[libvirt] [PATCH v1 01/40] util: cgroup: modify virCgroupFree to take virCgroupPtr

Sukrit Bhatnagar skrtbhtngr at gmail.com
Tue Jul 24 16:50:41 UTC 2018


On Tue, 24 Jul 2018 at 11:33, Erik Skultety <eskultet at redhat.com> wrote:
>
> On Mon, Jul 23, 2018 at 08:19:21PM +0530, Sukrit Bhatnagar wrote:
> > On Mon, 23 Jul 2018 at 16:29, Erik Skultety <eskultet at redhat.com> wrote:
> > >
> > > On Sat, Jul 21, 2018 at 05:36:33PM +0530, Sukrit Bhatnagar wrote:
> > > > Modify virCgroupFree function signature to take a value of type
> > > > virCgroupPtr instead of virCgroupPtr * as the parameter.
> > > >
> > > > Change the argument type in all calls to virCgroupFree function
> > > > from virCgroupPtr * to virCgroupPtr.
> > >
> > > ^This sentence doesn't add any useful information. Instead, the commit message
> > > should add information about why we're performing this change, i.e. in order to
> > > enable usage of VIR_AUTOPTR with cgroup module or something alike.
> > > Also, this patch is oddly placed, IMHO it should come before patch 8, where the
> > > other work on cgroup module is done.
> > >
> > > With that:
> > > Reviewed-by: Erik Skultety <eskultet at redhat.com>
> > >
> > > ...
> > >
> > > > @@ -1379,8 +1379,8 @@ virCgroupNewPartition(const char *path,
> > > >      ret = 0;
> > > >   cleanup:
> > > >      if (ret != 0)
> > > > -        virCgroupFree(group);
> > > > -    virCgroupFree(&parent);
> > > > +        virCgroupFree(*group);
> > > > +    virCgroupFree(parent);
> > >
> > > Since you're already touching the code, I'd appreciate another "adjustment"
> > > patch where occurrences like ^this will be replaced by a VIR_STEAL_PTR, IOW
> > > where we're passing a reference to a pointer in order to change the original
> > > pointer, we should use a VIR_STEAL_PTR just before the cleanup section, so that
> > > we don't need an if (ret != 0) or if (ret < 0) check only to conditionally do
> > > some cleanup. Feel free to let me know if none of what I just wrote is clear.
> >
> > I am assuming that you are referring to `group` variable. If so, then I cannot
> > apply cleanup attribute to function parameters and `group` is one of them.
>
> I didn't mean using a cleanup attribute. What I meant was making the necessary
> adjustments in order to get rid of the 'if(ret != 0)' check, since you're
> already touching the code I thought why not going one step further...


You mean something like this?

    VIR_AUTOPTR(virCgroup) ptr = NULL;
    ...
    return 0;
 cleanup:
    VIR_STEAL_PTR(ptr, *group);
    return -1;




More information about the libvir-list mailing list