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. Erik -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
