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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list