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

On Mon, 23 Jul 2018 at 16:29, Erik Skultety <eskultet(a)redhat.com&gt; 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&gt; > > ... > > > @@ -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.