On Mon, Jul 23, 2018 at 08:19:21PM +0530, Sukrit Bhatnagar wrote: > On Mon, 23 Jul 2018 at 16:29, Erik Skultety <eskultet@xxxxxxxxxx> 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@xxxxxxxxxx> > > > > ... > > > > > @@ -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... Erik -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
