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

[Erik Skultety]

On Thu, Aug 02, 2018 at 11:03:16PM +0530, Sukrit Bhatnagar wrote:
> On Thu, 2 Aug 2018 at 19:33, Erik Skultety <eskultet at redhat.com> 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 at gmail.com>
> > > ---
> > >  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?

Why do you need a new Free wrapper? Naturally, you'd need a "single-token" type
name, something like virNetlinkNlMsg which you'd put into src/util/virnetlink.h
along with the VIR_DEFINE_AUTOPTR_FUNC(virNetlinkNlMsg, nlmsg_free) and there
you go, you can use VIR_AUTOPTR with it.

Erik