On Mon, Feb 24, 2014 at 01:25:04PM +0100, Richard Weinberger wrote:
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? ;=)
Just my eyes in this case ;-P
+ 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()?
VIR_ERROR merely puts a message in the logs. It doesn't propagate anything back to the client making the API call. The virReport*Error functions actually send an error back to the client app. You basically always want virReport*Error - there's almost no cases where VIR_ERROR is the right thing todo.
+ + 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.
I don't see a valid reason why chown would fail in normal usage, so I think it should be fatal. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|