[libvirt] [PATCH v1 25/40] util: usb: use VIR_AUTOPTR for aggregate types

Sukrit Bhatnagar posted 40 patches 7 years, 6 months ago
There is a newer version of this series
[libvirt] [PATCH v1 25/40] util: usb: use VIR_AUTOPTR for aggregate types
Posted by Sukrit Bhatnagar 7 years, 6 months ago
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.

Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com>
---
 src/util/virusb.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/util/virusb.c b/src/util/virusb.c
index c14683f..cfeac51 100644
--- a/src/util/virusb.c
+++ b/src/util/virusb.c
@@ -508,8 +508,7 @@ void
 virUSBDeviceListDel(virUSBDeviceListPtr list,
                     virUSBDevicePtr dev)
 {
-    virUSBDevicePtr ret = virUSBDeviceListSteal(list, dev);
-    virUSBDeviceFree(ret);
+    VIR_AUTOPTR(virUSBDevice) ret = virUSBDeviceListSteal(list, dev);
 }
 
 virUSBDevicePtr
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 25/40] util: usb: use VIR_AUTOPTR for aggregate types
Posted by Erik Skultety 7 years, 6 months ago
On Sat, Jul 21, 2018 at 05:36:57PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOPTR macro for declaring aggregate pointer variables,
> majority of the calls to *Free functions can be dropped, which
> in turn leads to getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com>
> ---
>  src/util/virusb.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/src/util/virusb.c b/src/util/virusb.c
> index c14683f..cfeac51 100644
> --- a/src/util/virusb.c
> +++ b/src/util/virusb.c
> @@ -508,8 +508,7 @@ void
>  virUSBDeviceListDel(virUSBDeviceListPtr list,
>                      virUSBDevicePtr dev)
>  {
> -    virUSBDevicePtr ret = virUSBDeviceListSteal(list, dev);
> -    virUSBDeviceFree(ret);
> +    VIR_AUTOPTR(virUSBDevice) ret = virUSBDeviceListSteal(list, dev);
>  }
>

Technically, there's also a virUSBDevicePtr instance in virUSBDeviceSearch that
could be converted to VIR_AUTOPTR, but virUSBDeviceListAdd would have to take a
double pointer to @dev instead of a single pointer. A bit more background
info - the current issue is that virUSBDeviceListAdd calls our
VIR_APPEND_ELEMENT helper which does clear the original pointer which we could
utilize here, but not while passing a single pointer.
Not a deal breaker, though, it's just a nice to have, since you're already
working in this area, because I don't suppose we'd make such a change any time
soon after your assignment is over.

(regardless)
Reviewed-by: Erik Skultety <eskultet@redhat.com>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 25/40] util: usb: use VIR_AUTOPTR for aggregate types
Posted by Sukrit Bhatnagar 7 years, 6 months ago
On Wed, 25 Jul 2018 at 15:03, Erik Skultety <eskultet@redhat.com> wrote:
>
> On Sat, Jul 21, 2018 at 05:36:57PM +0530, Sukrit Bhatnagar wrote:
> > By making use of GNU C's cleanup attribute handled by the
> > VIR_AUTOPTR macro for declaring aggregate pointer variables,
> > majority of the calls to *Free functions can be dropped, which
> > in turn leads to getting rid of most of our cleanup sections.
> >
> > Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com>
> > ---
> >  src/util/virusb.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/src/util/virusb.c b/src/util/virusb.c
> > index c14683f..cfeac51 100644
> > --- a/src/util/virusb.c
> > +++ b/src/util/virusb.c
> > @@ -508,8 +508,7 @@ void
> >  virUSBDeviceListDel(virUSBDeviceListPtr list,
> >                      virUSBDevicePtr dev)
> >  {
> > -    virUSBDevicePtr ret = virUSBDeviceListSteal(list, dev);
> > -    virUSBDeviceFree(ret);
> > +    VIR_AUTOPTR(virUSBDevice) ret = virUSBDeviceListSteal(list, dev);
> >  }
> >
>
> Technically, there's also a virUSBDevicePtr instance in virUSBDeviceSearch that
> could be converted to VIR_AUTOPTR, but virUSBDeviceListAdd would have to take a
> double pointer to @dev instead of a single pointer. A bit more background
> info - the current issue is that virUSBDeviceListAdd calls our
> VIR_APPEND_ELEMENT helper which does clear the original pointer which we could
> utilize here, but not while passing a single pointer.
> Not a deal breaker, though, it's just a nice to have, since you're already
> working in this area, because I don't suppose we'd make such a change any time
> soon after your assignment is over.
>
> (regardless)
> Reviewed-by: Erik Skultety <eskultet@redhat.com>

There are many such functions:
virMediatedDeviceListAdd
virSCSIDeviceListAdd
virPCIDeviceListAdd

Making those changes would take a while and it is not directly related
to our cleanup. So, I'll do the necessary after the two cleanup macros
are used in all files. Is that ok?

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 25/40] util: usb: use VIR_AUTOPTR for aggregate types
Posted by Erik Skultety 7 years, 6 months ago
On Thu, Jul 26, 2018 at 12:32:41AM +0530, Sukrit Bhatnagar wrote:
> On Wed, 25 Jul 2018 at 15:03, Erik Skultety <eskultet@redhat.com> wrote:
> >
> > On Sat, Jul 21, 2018 at 05:36:57PM +0530, Sukrit Bhatnagar wrote:
> > > By making use of GNU C's cleanup attribute handled by the
> > > VIR_AUTOPTR macro for declaring aggregate pointer variables,
> > > majority of the calls to *Free functions can be dropped, which
> > > in turn leads to getting rid of most of our cleanup sections.
> > >
> > > Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com>
> > > ---
> > >  src/util/virusb.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/src/util/virusb.c b/src/util/virusb.c
> > > index c14683f..cfeac51 100644
> > > --- a/src/util/virusb.c
> > > +++ b/src/util/virusb.c
> > > @@ -508,8 +508,7 @@ void
> > >  virUSBDeviceListDel(virUSBDeviceListPtr list,
> > >                      virUSBDevicePtr dev)
> > >  {
> > > -    virUSBDevicePtr ret = virUSBDeviceListSteal(list, dev);
> > > -    virUSBDeviceFree(ret);
> > > +    VIR_AUTOPTR(virUSBDevice) ret = virUSBDeviceListSteal(list, dev);
> > >  }
> > >
> >
> > Technically, there's also a virUSBDevicePtr instance in virUSBDeviceSearch that
> > could be converted to VIR_AUTOPTR, but virUSBDeviceListAdd would have to take a
> > double pointer to @dev instead of a single pointer. A bit more background
> > info - the current issue is that virUSBDeviceListAdd calls our
> > VIR_APPEND_ELEMENT helper which does clear the original pointer which we could
> > utilize here, but not while passing a single pointer.
> > Not a deal breaker, though, it's just a nice to have, since you're already
> > working in this area, because I don't suppose we'd make such a change any time
> > soon after your assignment is over.
> >
> > (regardless)
> > Reviewed-by: Erik Skultety <eskultet@redhat.com>
>
> There are many such functions:
> virMediatedDeviceListAdd
> virSCSIDeviceListAdd
> virPCIDeviceListAdd
>
> Making those changes would take a while and it is not directly related
> to our cleanup. So, I'll do the necessary after the two cleanup macros
> are used in all files. Is that ok?

Yeah, as I said, it's just "nice-to-have", I understand we are short of time,
so don't bother for now.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 25/40] util: usb: use VIR_AUTOPTR for aggregate types
Posted by Sukrit Bhatnagar 7 years, 6 months ago
On Thu, 26 Jul 2018 at 00:32, Sukrit Bhatnagar <skrtbhtngr@gmail.com> wrote:
>
> On Wed, 25 Jul 2018 at 15:03, Erik Skultety <eskultet@redhat.com> wrote:
> >
> > On Sat, Jul 21, 2018 at 05:36:57PM +0530, Sukrit Bhatnagar wrote:
> > > By making use of GNU C's cleanup attribute handled by the
> > > VIR_AUTOPTR macro for declaring aggregate pointer variables,
> > > majority of the calls to *Free functions can be dropped, which
> > > in turn leads to getting rid of most of our cleanup sections.
> > >
> > > Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com>
> > > ---
> > >  src/util/virusb.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/src/util/virusb.c b/src/util/virusb.c
> > > index c14683f..cfeac51 100644
> > > --- a/src/util/virusb.c
> > > +++ b/src/util/virusb.c
> > > @@ -508,8 +508,7 @@ void
> > >  virUSBDeviceListDel(virUSBDeviceListPtr list,
> > >                      virUSBDevicePtr dev)
> > >  {
> > > -    virUSBDevicePtr ret = virUSBDeviceListSteal(list, dev);
> > > -    virUSBDeviceFree(ret);
> > > +    VIR_AUTOPTR(virUSBDevice) ret = virUSBDeviceListSteal(list, dev);
> > >  }
> > >
> >
> > Technically, there's also a virUSBDevicePtr instance in virUSBDeviceSearch that
> > could be converted to VIR_AUTOPTR, but virUSBDeviceListAdd would have to take a
> > double pointer to @dev instead of a single pointer. A bit more background
> > info - the current issue is that virUSBDeviceListAdd calls our
> > VIR_APPEND_ELEMENT helper which does clear the original pointer which we could
> > utilize here, but not while passing a single pointer.
> > Not a deal breaker, though, it's just a nice to have, since you're already
> > working in this area, because I don't suppose we'd make such a change any time
> > soon after your assignment is over.
> >
> > (regardless)
> > Reviewed-by: Erik Skultety <eskultet@redhat.com>
>
> There are many such functions:
> virMediatedDeviceListAdd
> virSCSIDeviceListAdd
> virPCIDeviceListAdd
>
> Making those changes would take a while and it is not directly related
> to our cleanup. So, I'll do the necessary after the two cleanup macros
> are used in all files. Is that ok?

For now, I am changing virUSBDeviceListAdd and the related occurrences.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list