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

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.

Otherwise it the patch looks fine. Erik