Re: [libvirt] [PATCH v1 01/40] util: cgroup: modify virCgroupFree to take virCgroupPtr
On Tue, 24 Jul 2018 at 11:33, Erik Skultety wrote: > > On Mon, Jul 23, 2018 at 08:19:21PM +0530, Sukrit Bhatnagar wrote: > > On Mon, 23 Jul 2018 at 16:29, Erik Skultety 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 > > > > > > ... > > > > > > > @@ -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 libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 01/40] util: cgroup: modify virCgroupFree to take virCgroupPtr
On Mon, Jul 23, 2018 at 08:19:21PM +0530, Sukrit Bhatnagar wrote: > On Mon, 23 Jul 2018 at 16:29, Erik Skultety 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 > > > > ... > > > > > @@ -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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 01/40] util: cgroup: modify virCgroupFree to take virCgroupPtr
On Mon, 23 Jul 2018 at 16:29, Erik Skultety 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 > > ... > > > @@ -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. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 01/40] util: cgroup: modify virCgroupFree to take virCgroupPtr
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 ... > @@ -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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
