Re: [libvirt] [PATCH v1 01/40] util: cgroup: modify virCgroupFree to take virCgroupPtr
On Tue, 24 Jul 2018 at 11:33, Erik Skultety <eskultet(a)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(a)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(a)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;