1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH v1 00/32] use GNU C's cleanup attribute in src/util (batch III)
  4. View patch

[libvirt] [PATCH v1 16/32] util: netdevmacvlan: use VIR_AUTOPTR for aggregate types

Sukrit Bhatnagar posted 32 patches 7 years, 6 months ago
There is a newer version of this series
[libvirt] [PATCH v1 16/32] util: netdevmacvlan: 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/virnetdevmacvlan.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index a2ed65c..d01e5ef 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -898,19 +898,20 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname,
                                              virNetDevVPortProfilePtr virtPortProfile,
                                              virNetDevVPortProfileOp vmOp)
 {
-    virNetlinkCallbackDataPtr calld = NULL;
+    VIR_AUTOPTR(virNetlinkCallbackData) calld = NULL;
+    virNetlinkCallbackDataPtr temp ATTRIBUTE_UNUSED = NULL;
 
     if (virtPortProfile && virNetlinkEventServiceIsRunning(NETLINK_ROUTE)) {
         if (VIR_ALLOC(calld) < 0)
-            goto error;
+            return -1;
         if (VIR_STRDUP(calld->cr_ifname, ifname) < 0)
-            goto error;
+            return -1;
         if (VIR_ALLOC(calld->virtPortProfile) < 0)
-            goto error;
+            return -1;
         memcpy(calld->virtPortProfile, virtPortProfile, sizeof(*virtPortProfile));
         virMacAddrSet(&calld->macaddress, macaddress);
         if (VIR_STRDUP(calld->linkdev, linkdev) < 0)
-            goto error;
+            return -1;
         memcpy(calld->vmuuid, vmuuid, sizeof(calld->vmuuid));
 
         calld->vmOp = vmOp;
@@ -918,14 +919,12 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname,
         if (virNetlinkEventAddClient(virNetDevMacVLanVPortProfileCallback,
                                      virNetDevMacVLanVPortProfileDestroyCallback,
                                      calld, macaddress, NETLINK_ROUTE) < 0)
-            goto error;
+            return -1;
     }
 
+    VIR_STEAL_PTR(temp, calld);
+
     return 0;
-
- error:
-    virNetlinkCallbackDataFree(calld);
-    return -1;
 }
 
 
@@ -1184,9 +1183,9 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname,
     }
 
     if (mode == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) {
-        virMacAddrPtr MAC = NULL;
-        virMacAddrPtr adminMAC = NULL;
-        virNetDevVlanPtr vlan = NULL;
+        VIR_AUTOPTR(virMacAddr) MAC = NULL;
+        VIR_AUTOPTR(virMacAddr) adminMAC = NULL;
+        VIR_AUTOPTR(virNetDevVlan) vlan = NULL;
 
         if ((virNetDevReadNetConfig(linkdev, -1, stateDir,
                                     &adminMAC, &vlan, &MAC) == 0) &&
@@ -1194,9 +1193,6 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname,
 
             ignore_value(virNetDevSetNetConfig(linkdev, -1,
                                                adminMAC, vlan, MAC, !!vlan));
-            VIR_FREE(MAC);
-            VIR_FREE(adminMAC);
-            virNetDevVlanFree(vlan);
         }
     }
 
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 16/32] util: netdevmacvlan: use VIR_AUTOPTR for aggregate types
Posted by Erik Skultety 7 years, 6 months ago 
On Sat, Jul 28, 2018 at 11:31:31PM +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/virnetdevmacvlan.c | 28 ++++++++++++----------------
>  1 file changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
> index a2ed65c..d01e5ef 100644
> --- a/src/util/virnetdevmacvlan.c
> +++ b/src/util/virnetdevmacvlan.c
> @@ -898,19 +898,20 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname,
>                                               virNetDevVPortProfilePtr virtPortProfile,
>                                               virNetDevVPortProfileOp vmOp)
>  {
> -    virNetlinkCallbackDataPtr calld = NULL;
> +    VIR_AUTOPTR(virNetlinkCallbackData) calld = NULL;
> +    virNetlinkCallbackDataPtr temp ATTRIBUTE_UNUSED = NULL;
>
>      if (virtPortProfile && virNetlinkEventServiceIsRunning(NETLINK_ROUTE)) {
>          if (VIR_ALLOC(calld) < 0)
> -            goto error;
> +            return -1;
>          if (VIR_STRDUP(calld->cr_ifname, ifname) < 0)
> -            goto error;
> +            return -1;
>          if (VIR_ALLOC(calld->virtPortProfile) < 0)
> -            goto error;
> +            return -1;
>          memcpy(calld->virtPortProfile, virtPortProfile, sizeof(*virtPortProfile));
>          virMacAddrSet(&calld->macaddress, macaddress);
>          if (VIR_STRDUP(calld->linkdev, linkdev) < 0)
> -            goto error;
> +            return -1;
>          memcpy(calld->vmuuid, vmuuid, sizeof(calld->vmuuid));
>
>          calld->vmOp = vmOp;
> @@ -918,14 +919,12 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname,
>          if (virNetlinkEventAddClient(virNetDevMacVLanVPortProfileCallback,
>                                       virNetDevMacVLanVPortProfileDestroyCallback,
>                                       calld, macaddress, NETLINK_ROUTE) < 0)
> -            goto error;
> +            return -1;
>      }
>
> +    VIR_STEAL_PTR(temp, calld);
> +
>      return 0;
> -
> - error:
> -    virNetlinkCallbackDataFree(calld);
> -    return -1;
>  }

^This is stretching the VIR_AUTO* concept too much, there's no apparent gain
here and should be left as is.


>
>
> @@ -1184,9 +1183,9 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname,
>      }
>
>      if (mode == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) {
> -        virMacAddrPtr MAC = NULL;
> -        virMacAddrPtr adminMAC = NULL;
> -        virNetDevVlanPtr vlan = NULL;
> +        VIR_AUTOPTR(virMacAddr) MAC = NULL;
> +        VIR_AUTOPTR(virMacAddr) adminMAC = NULL;
> +        VIR_AUTOPTR(virNetDevVlan) vlan = NULL;
>
>          if ((virNetDevReadNetConfig(linkdev, -1, stateDir,
>                                      &adminMAC, &vlan, &MAC) == 0) &&
> @@ -1194,9 +1193,6 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname,
>
>              ignore_value(virNetDevSetNetConfig(linkdev, -1,
>                                                 adminMAC, vlan, MAC, !!vlan));
> -            VIR_FREE(MAC);
> -            VIR_FREE(adminMAC);
> -            virNetDevVlanFree(vlan);
>          }

To ^this hunk:
Reviewed-by: Erik Skultety <eskultet@redhat.com>


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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 16/32] util: netdevmacvlan: use VIR_AUTOPTR for aggregate types
Posted by Sukrit Bhatnagar 7 years, 6 months ago 
On Fri, 3 Aug 2018 at 18:49, Erik Skultety <eskultet@redhat.com> wrote:
>
> On Sat, Jul 28, 2018 at 11:31:31PM +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/virnetdevmacvlan.c | 28 ++++++++++++----------------
> >  1 file changed, 12 insertions(+), 16 deletions(-)
> >
> > diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
> > index a2ed65c..d01e5ef 100644
> > --- a/src/util/virnetdevmacvlan.c
> > +++ b/src/util/virnetdevmacvlan.c
> > @@ -898,19 +898,20 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname,
> >                                               virNetDevVPortProfilePtr virtPortProfile,
> >                                               virNetDevVPortProfileOp vmOp)
> >  {
> > -    virNetlinkCallbackDataPtr calld = NULL;
> > +    VIR_AUTOPTR(virNetlinkCallbackData) calld = NULL;
> > +    virNetlinkCallbackDataPtr temp ATTRIBUTE_UNUSED = NULL;
> >
> >      if (virtPortProfile && virNetlinkEventServiceIsRunning(NETLINK_ROUTE)) {
> >          if (VIR_ALLOC(calld) < 0)
> > -            goto error;
> > +            return -1;
> >          if (VIR_STRDUP(calld->cr_ifname, ifname) < 0)
> > -            goto error;
> > +            return -1;
> >          if (VIR_ALLOC(calld->virtPortProfile) < 0)
> > -            goto error;
> > +            return -1;
> >          memcpy(calld->virtPortProfile, virtPortProfile, sizeof(*virtPortProfile));
> >          virMacAddrSet(&calld->macaddress, macaddress);
> >          if (VIR_STRDUP(calld->linkdev, linkdev) < 0)
> > -            goto error;
> > +            return -1;
> >          memcpy(calld->vmuuid, vmuuid, sizeof(calld->vmuuid));
> >
> >          calld->vmOp = vmOp;
> > @@ -918,14 +919,12 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname,
> >          if (virNetlinkEventAddClient(virNetDevMacVLanVPortProfileCallback,
> >                                       virNetDevMacVLanVPortProfileDestroyCallback,
> >                                       calld, macaddress, NETLINK_ROUTE) < 0)
> > -            goto error;
> > +            return -1;
> >      }
> >
> > +    VIR_STEAL_PTR(temp, calld);
> > +
> >      return 0;
> > -
> > - error:
> > -    virNetlinkCallbackDataFree(calld);
> > -    return -1;
> >  }
>
> ^This is stretching the VIR_AUTO* concept too much, there's no apparent gain
> here and should be left as is.

But by doing this, are getting rid of goto jumps and error section.
That was one of the goals, right?

>
> >
> >
> > @@ -1184,9 +1183,9 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname,
> >      }
> >
> >      if (mode == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) {
> > -        virMacAddrPtr MAC = NULL;
> > -        virMacAddrPtr adminMAC = NULL;
> > -        virNetDevVlanPtr vlan = NULL;
> > +        VIR_AUTOPTR(virMacAddr) MAC = NULL;
> > +        VIR_AUTOPTR(virMacAddr) adminMAC = NULL;
> > +        VIR_AUTOPTR(virNetDevVlan) vlan = NULL;
> >
> >          if ((virNetDevReadNetConfig(linkdev, -1, stateDir,
> >                                      &adminMAC, &vlan, &MAC) == 0) &&
> > @@ -1194,9 +1193,6 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname,
> >
> >              ignore_value(virNetDevSetNetConfig(linkdev, -1,
> >                                                 adminMAC, vlan, MAC, !!vlan));
> > -            VIR_FREE(MAC);
> > -            VIR_FREE(adminMAC);
> > -            virNetDevVlanFree(vlan);
> >          }
>
> To ^this hunk:
> Reviewed-by: Erik Skultety <eskultet@redhat.com>
>
>
> >      }
> >
> > --
> > 1.8.3.1
> >
> > --
> > libvir-list mailing list
> > libvir-list@redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 16/32] util: netdevmacvlan: use VIR_AUTOPTR for aggregate types
Posted by Erik Skultety 7 years, 6 months ago 
On Fri, Aug 03, 2018 at 10:56:40PM +0530, Sukrit Bhatnagar wrote:
> On Fri, 3 Aug 2018 at 18:49, Erik Skultety <eskultet@redhat.com> wrote:
> >
> > On Sat, Jul 28, 2018 at 11:31:31PM +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/virnetdevmacvlan.c | 28 ++++++++++++----------------
> > >  1 file changed, 12 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
> > > index a2ed65c..d01e5ef 100644
> > > --- a/src/util/virnetdevmacvlan.c
> > > +++ b/src/util/virnetdevmacvlan.c
> > > @@ -898,19 +898,20 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname,
> > >                                               virNetDevVPortProfilePtr virtPortProfile,
> > >                                               virNetDevVPortProfileOp vmOp)
> > >  {
> > > -    virNetlinkCallbackDataPtr calld = NULL;
> > > +    VIR_AUTOPTR(virNetlinkCallbackData) calld = NULL;
> > > +    virNetlinkCallbackDataPtr temp ATTRIBUTE_UNUSED = NULL;
> > >
> > >      if (virtPortProfile && virNetlinkEventServiceIsRunning(NETLINK_ROUTE)) {
> > >          if (VIR_ALLOC(calld) < 0)
> > > -            goto error;
> > > +            return -1;
> > >          if (VIR_STRDUP(calld->cr_ifname, ifname) < 0)
> > > -            goto error;
> > > +            return -1;
> > >          if (VIR_ALLOC(calld->virtPortProfile) < 0)
> > > -            goto error;
> > > +            return -1;
> > >          memcpy(calld->virtPortProfile, virtPortProfile, sizeof(*virtPortProfile));
> > >          virMacAddrSet(&calld->macaddress, macaddress);
> > >          if (VIR_STRDUP(calld->linkdev, linkdev) < 0)
> > > -            goto error;
> > > +            return -1;
> > >          memcpy(calld->vmuuid, vmuuid, sizeof(calld->vmuuid));
> > >
> > >          calld->vmOp = vmOp;
> > > @@ -918,14 +919,12 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname,
> > >          if (virNetlinkEventAddClient(virNetDevMacVLanVPortProfileCallback,
> > >                                       virNetDevMacVLanVPortProfileDestroyCallback,
> > >                                       calld, macaddress, NETLINK_ROUTE) < 0)
> > > -            goto error;
> > > +            return -1;
> > >      }
> > >
> > > +    VIR_STEAL_PTR(temp, calld);
> > > +
> > >      return 0;
> > > -
> > > - error:
> > > -    virNetlinkCallbackDataFree(calld);
> > > -    return -1;
> > >  }
> >
> > ^This is stretching the VIR_AUTO* concept too much, there's no apparent gain
> > here and should be left as is.
>
> But by doing this, are getting rid of goto jumps and error section.
> That was one of the goals, right?

Yes, it was the goal, but we can't do it blindly everywhere just for the sake
of replacement, for this specific case, we didn't really gain anything, because
we traded one error label containing a single *Free call for one extra
virNetlinkCallbackDataPtr variable just to be able to do a VIR_STEAL_PTR
(which again, should be mainly used for caller-provided pointers),
virNetlinkCallbackDataPtr typedefs and VIR_DEFINE_AUTOPTR_FUNC for a single use
case.

Erik

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.