Re: [libvirt] [PATCH v1 03/32] util: netdevbridge: use VIR_AUTOFREE instead of VIR_FREE for scalar types

On Thu, 2 Aug 2018 at 19:33, Erik Skultety <eskultet(a)redhat.com&gt; wrote: > > On Sat, Jul 28, 2018 at 11:31:18PM +0530, Sukrit Bhatnagar wrote: > > By making use of GNU C's cleanup attribute handled by the > > VIR_AUTOFREE macro for declaring scalar variables, majority > > of the VIR_FREE calls can be dropped, which in turn leads to > > getting rid of most of our cleanup sections. > > > > Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com&gt; > > --- > > src/util/virnetdevbridge.c | 45 +++++++++++++++------------------------------ > > 1 file changed, 15 insertions(+), 30 deletions(-) > > > > diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c > > index e46ac35..bf30d7c 100644 > > --- a/src/util/virnetdevbridge.c > > +++ b/src/util/virnetdevbridge.c > > @@ -126,8 +126,7 @@ static int virNetDevBridgeSet(const char *brname, > > int fd, /* control socket */ > > struct ifreq *ifr) /* pre-filled bridge name */ > > { > > - char *path = NULL; > > - int ret = -1; > > + VIR_AUTOFREE(char *) path = NULL; > > > > if (virAsprintf(&path, SYSFS_NET_DIR "%s/bridge/%s", brname, paramname) < 0) > > return -1; > > @@ -138,7 +137,7 @@ static int virNetDevBridgeSet(const char *brname, > > if (virFileWriteStr(path, valuestr, 0) < 0) { > > virReportSystemError(errno, > > _("Unable to set bridge %s %s"), brname, paramname); > > - goto cleanup; > > + return -1; > > } > > } else { > > unsigned long paramid; > > @@ -149,21 +148,18 @@ static int virNetDevBridgeSet(const char *brname, > > } else { > > virReportSystemError(EINVAL, > > _("Unable to set bridge %s %s"), brname, paramname); > > - goto cleanup; > > + return -1; > > } > > unsigned long args[] = { paramid, value, 0, 0 }; > > ifr->ifr_data = (char*)&args; > > if (ioctl(fd, SIOCDEVPRIVATE, ifr) < 0) { > > virReportSystemError(errno, > > _("Unable to set bridge %s %s"), brname, paramname); > > - goto cleanup; > > + return -1; > > } > > } > > > > - ret = 0; > > - cleanup: > > - VIR_FREE(path); > > - return ret; > > + return 0; > > } > > > > > > @@ -171,7 +167,7 @@ static int virNetDevBridgeGet(const char *brname, > > const char *paramname, /* sysfs param name */ > > unsigned long *value) /* current value */ > > { > > - char *path = NULL; > > + VIR_AUTOFREE(char *) path = NULL; > > Referring to my response to previous patch, I'll move ^this at the end of the > "declare" block (there are a few identical spots across the patch). > > ... > > > malformed_resp: > > @@ -1069,7 +1055,7 @@ virNetDevBridgeFDBAddDel(const virMacAddr *mac, const char *ifname, > > unsigned int flags, bool isAdd) > > { > > int ret = -1; > > - struct nlmsghdr *resp = NULL; > > + VIR_AUTOFREE(struct nlmsghdr *) resp = NULL; > > struct nlmsgerr *err; > > unsigned int recvbuflen; > > struct nl_msg *nl_msg; > > So, I believe ^this external type can easily be turned into an autoclean > variant. > > > @@ -1142,7 +1128,6 @@ virNetDevBridgeFDBAddDel(const virMacAddr *mac, const char *ifname, > > ret = 0; > > cleanup: > > nlmsg_free(nl_msg); > > So that ^this would be done automatically. We would need to pass the nlmsg_free function in the cleanup attribute somehow. So, shall we not use the VIR_AUTO macros and declare it with cleanup attribute explicitly, or create a new type and a new Free wrapper for it to use with out macros?