Re: [libvirt] [PATCHv2 2/4] Add wrapper macros around nla_nest*/nla_put* to make code more readable

On Wed, Sep 05, 2018 at 04:36:28PM +0800, Shi Lei wrote: > This patch adds wrapper macros around nla_nest*/nla_put* which apply to > virNetlinkNewLink and virNetDevSetVfConfig and virNetDevVPortProfileOpSetLink. > > Signed-off-by: Shi Lei <shi_lei(a)massclouds.com&gt; > --- >  src/util/virnetdev.c             |  54 ++++++-------- >  src/util/virnetdevvportprofile.c | 117 ++++++++++--------------------- >  src/util/virnetlink.c            |  74 +++++++------------ >  src/util/virnetlink.h            |  52 ++++++++++++++ >  4 files changed, 134 insertions(+), 163 deletions(-) > > diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c > index 8eac419..892a147 100644 > --- a/src/util/virnetdev.c > +++ b/src/util/virnetdev.c > @@ -1657,11 +1657,11 @@ virNetDevSetVfConfig(const char *ifname, int vf, >  { >      int rc = -1; >      char macstr[VIR_MAC_STRING_BUFLEN]; > -    struct nlmsghdr *resp = NULL; >      struct nlmsgerr *err; >      unsigned int recvbuflen = 0; > -    struct nl_msg *nl_msg; >      struct nlattr *vfinfolist, *vfinfo; > +    VIR_AUTOPTR(virNetlinkMsg) nl_msg = NULL; > +    VIR_AUTOFREE(struct nlmsghdr *) resp = NULL; changes related to AUTO{PTR,FREE} should completely separate, even from the series itself.

>      struct ifinfomsg ifinfo = { >          .ifi_family = AF_UNSPEC, >          .ifi_index  = -1, > @@ -1676,47 +1676,41 @@ virNetDevSetVfConfig(const char *ifname, int vf, >          return rc; >      } > > -    if (nlmsg_append(nl_msg,  &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) > -        goto buffer_too_small; > - > -    if (ifname && > -        nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0) as I mentioned in my reply to the previous patch, I'd first replace ocurrences like ^these with nla_put_string for consistency and also making reviewer's life easier.

> -        goto buffer_too_small; > - > +    if (nlmsg_append(nl_msg,  &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) { > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nlmsg_append error")); > +        return rc; > +    } > > -    if (!(vfinfolist = nla_nest_start(nl_msg, IFLA_VFINFO_LIST))) > -        goto buffer_too_small; > +    NETLINK_MSG_PUT_STRING(nl_msg, IFLA_IFNAME, ifname); > > -    if (!(vfinfo = nla_nest_start(nl_msg, IFLA_VF_INFO))) > -        goto buffer_too_small; > +    NETLINK_MSG_NEST_START(nl_msg, vfinfolist, IFLA_VFINFO_LIST); > +    NETLINK_MSG_NEST_START(nl_msg, vfinfo, IFLA_VF_INFO); > >      if (macaddr) { >          struct ifla_vf_mac ifla_vf_mac = { > -             .vf = vf, > -             .mac = { 0, }, > +            .vf = vf, > +            .mac = { 0, }, >          }; > >          virMacAddrGetRaw(macaddr, ifla_vf_mac.mac); > > -        if (nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac), > -                    &ifla_vf_mac) < 0) > -            goto buffer_too_small; > +        NETLINK_MSG_PUT(nl_msg, IFLA_VF_MAC, > +                        sizeof(ifla_vf_mac), &ifla_vf_mac); >      } > >      if (vlanid >= 0) { >          struct ifla_vf_vlan ifla_vf_vlan = { > -             .vf = vf, > -             .vlan = vlanid, > -             .qos = 0, > +            .vf = vf, > +            .vlan = vlanid, > +            .qos = 0, >          }; > > -        if (nla_put(nl_msg, IFLA_VF_VLAN, sizeof(ifla_vf_vlan), > -                    &ifla_vf_vlan) < 0) > -            goto buffer_too_small; > +        NETLINK_MSG_PUT(nl_msg, IFLA_VF_VLAN, > +                        sizeof(ifla_vf_vlan), &ifla_vf_vlan); >      } > > -    nla_nest_end(nl_msg, vfinfo); > -    nla_nest_end(nl_msg, vfinfolist); > +    NETLINK_MSG_NEST_END(nl_msg, vfinfo); > +    NETLINK_MSG_NEST_END(nl_msg, vfinfolist); > >      if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0, >                            NETLINK_ROUTE, 0) < 0) > @@ -1767,20 +1761,12 @@ virNetDevSetVfConfig(const char *ifname, int vf, >                ifname, vf, >                macaddr ? virMacAddrFormat(macaddr, macstr) : "(unchanged)", >                vlanid, rc < 0 ? "Fail" : "Success"); > - > -    nlmsg_free(nl_msg); > -    VIR_FREE(resp); >      return rc; > >   malformed_resp: >      virReportError(VIR_ERR_INTERNAL_ERROR, "%s", >                     _("malformed netlink response message")); >      goto cleanup; > - > - buffer_too_small: > -    virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > -                   _("allocated netlink buffer is too small")); > -    goto cleanup; >  } > >  static int > diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c > index 3ebf757..8574571 100644 > --- a/src/util/virnetdevvportprofile.c > +++ b/src/util/virnetdevvportprofile.c > @@ -641,8 +641,6 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex, >                                 int32_t vf, >                                 uint8_t op) >  { > -    int rc = -1; > -    struct nlmsghdr *resp = NULL; >      struct nlmsgerr *err; >      struct ifinfomsg ifinfo = { >          .ifi_family = AF_UNSPEC, > @@ -651,12 +649,14 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex, >      unsigned int recvbuflen = 0; >      int src_pid = 0; >      uint32_t dst_pid = 0; > -    struct nl_msg *nl_msg; >      struct nlattr *vfports = NULL, *vfport; >      char macStr[VIR_MAC_STRING_BUFLEN]; >      char hostUUIDStr[VIR_UUID_STRING_BUFLEN]; >      char instanceUUIDStr[VIR_UUID_STRING_BUFLEN]; >      const char *opName; > +    int vfport_type = IFLA_PORT_SELF; > +    VIR_AUTOPTR(virNetlinkMsg) nl_msg = NULL; > +    VIR_AUTOFREE(struct nlmsghdr *) resp = NULL; again the VIR_AUTO{PTR,FREE} stuff...

> >      switch (op) { >      case PORT_REQUEST_PREASSOCIATE: > @@ -691,24 +691,21 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex, >      nl_msg = nlmsg_alloc_simple(RTM_SETLINK, NLM_F_REQUEST); >      if (!nl_msg) { >          virReportOOMError(); > -        return rc; > +        return -1; >      } > > -    if (nlmsg_append(nl_msg,  &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) > -        goto buffer_too_small; > +    if (nlmsg_append(nl_msg,  &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) { > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nlmsg_append error")); > +        return -1; > +    } > > -    if (ifname && > -        nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0) > -        goto buffer_too_small; > +    NETLINK_MSG_PUT_STRING(nl_msg, IFLA_IFNAME, ifname); > >      if (macaddr || vlanid >= 0) { >          struct nlattr *vfinfolist, *vfinfo; > > -        if (!(vfinfolist = nla_nest_start(nl_msg, IFLA_VFINFO_LIST))) > -            goto buffer_too_small; > - > -        if (!(vfinfo = nla_nest_start(nl_msg, IFLA_VF_INFO))) > -            goto buffer_too_small; > +        NETLINK_MSG_NEST_START(nl_msg, vfinfolist, IFLA_VFINFO_LIST); > +        NETLINK_MSG_NEST_START(nl_msg, vfinfo, IFLA_VF_INFO); > >          if (macaddr) { >              struct ifla_vf_mac ifla_vf_mac = { > @@ -718,9 +715,8 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex, > >              virMacAddrGetRaw(macaddr, ifla_vf_mac.mac); > > -            if (nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac), > -                        &ifla_vf_mac) < 0) > -                goto buffer_too_small; > +            NETLINK_MSG_PUT(nl_msg, IFLA_VF_MAC, > +                            sizeof(ifla_vf_mac), &ifla_vf_mac); >          } > >          if (vlanid >= 0) { > @@ -730,77 +726,49 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex, >                  .qos = 0, >              }; > > -            if (nla_put(nl_msg, IFLA_VF_VLAN, sizeof(ifla_vf_vlan), > -                        &ifla_vf_vlan) < 0) > -                goto buffer_too_small; > +            NETLINK_MSG_PUT(nl_msg, IFLA_VF_VLAN, > +                            sizeof(ifla_vf_vlan), &ifla_vf_vlan); >          } > > -        nla_nest_end(nl_msg, vfinfo); > -        nla_nest_end(nl_msg, vfinfolist); > -    } > - > -    if (vf == PORT_SELF_VF && nltarget_kernel) { > -        if (!(vfport = nla_nest_start(nl_msg, IFLA_PORT_SELF))) > -            goto buffer_too_small; > -    } else { > -        if (!(vfports = nla_nest_start(nl_msg, IFLA_VF_PORTS))) > -            goto buffer_too_small; > - > -        /* begin nesting vfports */ > -        if (!(vfport = nla_nest_start(nl_msg, IFLA_VF_PORT))) > -            goto buffer_too_small; > -    } at this point it got quite difficult to keep track of all the replacements, so I'd suggest making the replacements one at a time, first one macro, then another, etc.

... >  /** > diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h > index 8163a81..2e64570 100644 > --- a/src/util/virnetlink.h > +++ b/src/util/virnetlink.h > @@ -48,6 +48,58 @@ struct nlmsghdr; > >  # endif /* __linux__ */ > > + > +# define NETLINK_MSG_NEST_START(msg, container, attrtype) \ > +do { \ > +    container = nla_nest_start(msg, attrtype); \ > +    if (!container) { \ > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \ > +                       _("nla_nest_start error [" #attrtype "]")); \ > +        return -1; \ > +    } \ > +} while(0) > + > +# define NETLINK_MSG_NEST_END(msg, container) \ > +do { nla_nest_end(msg, container); } while(0) > + > +/* > + * When data is an rvalue, compiler will report error as below: > + * "the address of [...] will always evaluate as 'true' [-Werror=address]" > + * Add a dummy(as an lvalue) to solve it. Okay, let's try rewording this in a more compact manner below... > + */ > +# define NETLINK_MSG_PUT(msg, attrtype, datalen, data) \ > +do { \ how about: /* @data may not always be a pointer type, therefore making compilers complain * about the check below, so let's use an intermediary pointer type */

> +    const void *dummy = data; \ let's rename @dummy to dataptr to make it more explicit

> +    if (dummy && nla_put(msg, attrtype, datalen, data) < 0) { \ > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \ > +                       _("nla_put error [" #attrtype "]")); \ > +        return -1; \ > +    } \ > +} while(0) > + > +# define NETLINK_MSG_PUT_STRING(msg, attrtype, str) \ > +do { \ > +    const void *dummy = str; \ > +    if (dummy && nla_put_string(msg, attrtype, str) < 0) { \ > +        virReportError(VIR_ERR_INTERNAL_ERROR, \ > +                       _("nla_put_string error [" #attrtype "]: '%s'"), \ > +                       str); \ I don't think that changing the error is necessary, all of the variants essentially call nla_put and that one either fails with -EINVAL (not our case, since we always provide a valid datalen) or -ENOMEM, so the original error stating that the buffer wasn't large enough is sufficient.

> +        return -1; \ > +    } \ > +} while(0) I'm not entirely sold on these MSG_PUT_X macros, but I don't have a reasonable argument against either, so let's leave them in. Alternatively, we could have #define NETLINK_MSG_PUT_STRING(msg, attrtype, str) \     NETLINK_MSG_PUT(msg, attrtype, strlen(str) + 1, str) and that would make things more clean, afterall that is what all the nla_put_foo will do anyway... Erik

> + > +# define NETLINK_MSG_PUT_U32PTR(msg, attrtype, ptr) \ > +do { \ > +    const void *dummy = ptr; \ > +    if (dummy && nla_put_u32(msg, attrtype, *ptr) < 0) { \ > +        virReportError(VIR_ERR_INTERNAL_ERROR, \ > +                       _("nla_put_u32 error [" #attrtype "]: '%u'"), \ > +                       *ptr); \ > +        return -1; \ > +    } \ > +} while(0) > + > + >  int virNetlinkStartup(void); >  void virNetlinkShutdown(void); > > -- > 2.17.1 > > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list