Re: [libvirt] [PATCHv3 2/3] util: netlink: Add wrapper macros to make virNetlinkNewLink more readable

On Fri, Sep 07, 2018 at 03:17:25PM +0800, Shi Lei wrote: > This patch adds wrapper macros around nla_nest_[start|end] and nla_put > which can make virNetlinkNewLink more readable. > > Signed-off-by: Shi Lei <shi_lei(a)massclouds.com&gt; > --- >  src/util/virnetlink.c | 43 +++++++++++++++---------------------------- >  src/util/virnetlink.h | 30 ++++++++++++++++++++++++++++++ >  2 files changed, 45 insertions(+), 28 deletions(-) > > diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c > index d53cc73..99ad003 100644 > --- a/src/util/virnetlink.c > +++ b/src/util/virnetlink.c > @@ -529,41 +529,33 @@ virNetlinkNewLink(const char *ifname, >          return -ENOMEM; >      } > > -    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", > +                       _("out of memory: nlmsg_append")); Let's drop this hunk, see my comment at the end of the mail.

> +        return -ENOMEM; return -1;

> +    } > > -    if (ifname && nla_put_string(nl_msg, IFLA_IFNAME, ifname) < 0) > -        goto buffer_too_small; > +    NETLINK_MSG_PUT(nl_msg, IFLA_IFNAME, (strlen(ifname)+1), ifname); > > -    if (!(linkinfo = nla_nest_start(nl_msg, IFLA_LINKINFO))) > -        goto buffer_too_small; > +    NETLINK_MSG_NEST_START(nl_msg, linkinfo, IFLA_LINKINFO); > > -    if (type && nla_put_string(nl_msg, IFLA_INFO_KIND, type) < 0) > -        goto buffer_too_small; > +    NETLINK_MSG_PUT(nl_msg, IFLA_INFO_KIND, (strlen(type)+1), type); > >      if ((STREQ(type, "macvtap") || STREQ(type, "macvlan")) && >           extra_args && >           extra_args->macvlan_mode && >           *extra_args->macvlan_mode > 0) { > -        if (!(infodata = nla_nest_start(nl_msg, IFLA_INFO_DATA))) > -            goto buffer_too_small; > - > -        if (nla_put_u32(nl_msg, IFLA_MACVLAN_MODE, *extra_args->macvlan_mode) < 0) > -            goto buffer_too_small; > - > -        nla_nest_end(nl_msg, infodata); > +        NETLINK_MSG_NEST_START(nl_msg, infodata, IFLA_INFO_DATA); > +        NETLINK_MSG_PUT(nl_msg, IFLA_MACVLAN_MODE, > +                        sizeof(uint32_t), extra_args->macvlan_mode); > +        NETLINK_MSG_NEST_END(nl_msg, infodata); >      } > > -    nla_nest_end(nl_msg, linkinfo); > +    NETLINK_MSG_NEST_END(nl_msg, linkinfo); > >      if (extra_args) { > -        if (extra_args->ifindex && > -            nla_put_u32(nl_msg, IFLA_LINK, *extra_args->ifindex) < 0) > -            goto buffer_too_small; > - > -        if (extra_args->mac && > -            nla_put(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN, extra_args->mac) < 0) > -            goto buffer_too_small; > +        NETLINK_MSG_PUT(nl_msg, IFLA_LINK, sizeof(uint32_t), extra_args->ifindex); ^needs to be line wrapped...

> +        NETLINK_MSG_PUT(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN, extra_args->mac); >      } > >      if (virNetlinkCommand(nl_msg, &resp, &buflen, 0, 0, NETLINK_ROUTE, 0) < 0) > @@ -597,11 +589,6 @@ virNetlinkNewLink(const char *ifname, >      virReportError(VIR_ERR_INTERNAL_ERROR, "%s", >                     _("malformed netlink response message")); >      return -EBADMSG; > - > - buffer_too_small: > -    virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > -                   _("allocated netlink buffer is too small")); > -    return -ENOMEM; >  } > > > diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h > index 09bab08..6f59d6e 100644 > --- a/src/util/virnetlink.h > +++ b/src/util/virnetlink.h > @@ -48,6 +48,36 @@ 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", \ > +                       _("out of memory: nla_nest_start [" #attrtype "]")); \ So I checked and buffer_too_small label is very common among util/virnet*.c modules and that's where we want to use these macros, so let's simply use goto buffer_too_small and we can then figure out, how to optimize that, i.e. have a single place where we define the error, so that we wouldn't have to repeat _("allocated netlink buffer too small").

> +        return -ENOMEM; \ return -1

> +    } \ > +} while(0) > + > +# define NETLINK_MSG_NEST_END(msg, container) \ > +do { nla_nest_end(msg, container); } while(0) > + > +/* > + * @data may be like &foo sometimes, therefore making compilers complain > + * about the check below, so let's use an intermediary pointer type > + * "the address of [...] will always evaluate as 'true' [-Werror=address]" we need to use an intermediary pointer to @data as compilers may sometimes complain about @data not being a pointer type: "error: the address of 'foo' will always evaluate as 'true' [-Werror=address]"

> + */ > +# define NETLINK_MSG_PUT(msg, attrtype, datalen, data) \ > +do { \ > +    const void *dataptr = data; \ > +    if (dataptr && nla_put(msg, attrtype, datalen, dataptr) < 0) { \ > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \ > +                       _("out of memory: nla_put [" #attrtype "]")); \ see my comment a few paragraphs above...

> +        return -ENOMEM; \ return -1;

Erik I'm suggesting to squash the following (let me know if you agree):

diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 4e63edcfe5..eb03df4bda 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -537,7 +537,6 @@ virNetlinkNewLink(const char *ifname,      NETLINK_MSG_PUT(nl_msg, IFLA_IFNAME, (strlen(ifname) + 1), ifname);      NETLINK_MSG_NEST_START(nl_msg, linkinfo, IFLA_LINKINFO); -      NETLINK_MSG_PUT(nl_msg, IFLA_INFO_KIND, (strlen(type) + 1), type);      if ((STREQ(type, "macvtap") || STREQ(type, "macvlan")) && @@ -553,7 +552,8 @@ virNetlinkNewLink(const char *ifname,      NETLINK_MSG_NEST_END(nl_msg, linkinfo);      if (extra_args) { -        NETLINK_MSG_PUT(nl_msg, IFLA_LINK, sizeof(uint32_t), extra_args->ifindex); +        NETLINK_MSG_PUT(nl_msg, IFLA_LINK, +                        sizeof(uint32_t), extra_args->ifindex);          NETLINK_MSG_PUT(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN, extra_args->mac);      } @@ -588,6 +588,11 @@ virNetlinkNewLink(const char *ifname,      virReportError(VIR_ERR_INTERNAL_ERROR, "%s",                     _("malformed netlink response message"));      return -1; + + buffer_too_small: +    virReportError(VIR_ERR_INTERNAL_ERROR, "%s", +                   _("allocated netlink buffer is too small")); +    return -1; } diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 8bc33d0cd3..552a3a8fe1 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -48,33 +48,26 @@ 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", \ -                       _("out of memory: nla_nest_start [" #attrtype "]")); \ -        return -ENOMEM; \ -    } \ +    if (!container) \ +        goto buffer_too_small; \ } while(0) # define NETLINK_MSG_NEST_END(msg, container) \ do { nla_nest_end(msg, container); } while(0) /* - * @data may be like &foo sometimes, therefore making compilers complain - * about the check below, so let's use an intermediary pointer type - * "the address of [...] will always evaluate as 'true' [-Werror=address]" + * we need to use an intermediary pointer to @data as compilers may sometimes + * complain about @data not being a pointer type: + * error: the address of 'foo' will always evaluate as 'true' [-Werror=address]   */ # define NETLINK_MSG_PUT(msg, attrtype, datalen, data) \ do { \      const void *dataptr = data; \ -    if (dataptr && nla_put(msg, attrtype, datalen, dataptr) < 0) { \ -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \ -                       _("out of memory: nla_put [" #attrtype "]")); \ -        return -ENOMEM; \ -    } \ +    if (dataptr && nla_put(msg, attrtype, datalen, dataptr) < 0) \ +        goto buffer_too_small; \ } while(0)