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

[Richard Weinberger]

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.

>> +
>> +            VIR_FREE(entry);
>> +        }
>> +        closedir(dh);
>> +
>> +        if (chown(base, uid, gid) < 0)
>> +            VIR_WARN(_("cannot chown '%s' to (%u, %u): %s"), entry, uid, gid,
>> +            strerror(errno));
> 
> Likewise. Also it uses NULL 'entry' var rather than 'base' in the
> message args.
> 
>> +
>> +        VIR_FREE(base);
>> +    }
>> +
>> +    return 0;
>> +}
> 
> I'm going to send a followup with these fixes for review, which also
> centralizes the error cleanup code.

Okay!

Thanks,
//richard