subject:"\[libvirt\] \[PATCH v1 16\/32\] util\: netdevmacvlan\: use VIR

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

2018-08-06 Thread Erik Skultety

On Fri, Aug 03, 2018 at 10:56:40PM +0530, Sukrit Bhatnagar wrote:
> On Fri, 3 Aug 2018 at 18:49, Erik Skultety  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 
> > > ---
> > >  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(>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

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

2018-08-03 Thread Sukrit Bhatnagar

On Fri, 3 Aug 2018 at 18:49, Erik Skultety  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 
> > ---
> >  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(>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,
> >  , , ) == 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 
>
>
> >  }
> >
> > --
> > 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

2018-08-03 Thread Erik Skultety

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 
> ---
>  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(>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,
>  , , ) == 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 


>  }
>
> --
> 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

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

2018-07-28 Thread Sukrit Bhatnagar

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 
---
 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(>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,
 , , ) == 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

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

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

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

4 matches

Site Navigation

Mail list logo

Footer information