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: Let's drop this hunk, see my comment at the end of the mail. return -1; ^needs to be line wrapped... 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 -1 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]" see my comment a few paragraphs above... 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)