Re: [libvirt] [PATCH v2] lxc: Add virCgroupSetOwner()

Am 24.02.2014 13:20, schrieb Daniel P. Berrange: > On Fri, Feb 14, 2014 at 02:25:55PM +0100, Richard Weinberger wrote: >> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c >> index a6d60c5..4bef0db 100644 >> --- a/src/util/vircgroup.c >> +++ b/src/util/vircgroup.c >> @@ -3253,6 +3253,66 @@ cleanup: >> } >> >> >> +int virCgroupSetOwner(virCgroupPtr cgroup, >> + uid_t uid, >> + gid_t gid, >> + int controllers) >> +{ >> + size_t i; >> + >> + for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { >> + char *base, *entry; >> + DIR *dh; >> + struct dirent *de; >> + >> + if (!((1 << i) & controllers)) >> + continue; >> + >> + if (!cgroup->controllers[i].mountPoint) >> + continue; >> + >> + if (virAsprintf(&base, "%s%s", cgroup->controllers[i].mountPoint, >> + cgroup->controllers[i].placement) < 0) { >> + return -1; >> + } > > Indentation of 'return' is too deep Do you have something like a checkpatch.pl? ;=)

>> + dh = opendir(base); >> + if (!dh) { >> + VIR_ERROR(_("Unable to open %s: %s"), base, strerror(errno)); >> + VIR_FREE(base); >> + return -1; >> + } > > This should use virReportSystemError. To avoid further confusion. When to use VIR_ERROR() and when virReportSystemError()?

>> + >> + while ((de = readdir(dh)) != NULL) { >> + if (STREQ(de->d_name, ".") || >> + STREQ(de->d_name, "..")) >> + continue; >> + >> + if (virAsprintf(&entry, "%s/%s", base, de->d_name) < 0) { >> + VIR_FREE(base); >> + closedir(dh); >> + return -1; >> + } >> + >> + if (chown(entry, uid, gid) < 0) >> + VIR_WARN(_("cannot chown '%s' to (%u, %u): %s"), entry, uid, gid, >> + strerror(errno)); > > This should use virReportSystemError too, and propagate the error. Do you we really want to propagate this error? IMHO a failing chown() is not a fatal error.