On Tue, 24 Jul 2018 at 11:33, Erik Skultety <[email protected]> wrote:
>
> On Mon, Jul 23, 2018 at 08:19:21PM +0530, Sukrit Bhatnagar wrote:
> > On Mon, 23 Jul 2018 at 16:29, Erik Skultety <[email protected]> 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 <[email protected]>
> > >
> > > ...
> > >
> > > > @@ -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;
--
libvir-list mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/libvir-list
