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

[Daniel P. Berrange]

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 :|