[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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

> +
> +        dh = opendir(base);
> +        if (!dh) {
> +            VIR_ERROR(_("Unable to open %s: %s"), base, strerror(errno));
> +            VIR_FREE(base);
> +            return -1;
> +        }

This should use 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.

> +
> +            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.

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]