On 2018-09-10 at 22:39, Erik Skultety wrote:
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@massclouds.com> --- 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.
Okay. I have read it.
+ return -ENOMEM;
return -1;
Okay.
+ }
- 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...
Yes.
+ 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").
Okay.
+ return -ENOMEM; \
return -1
Okay.
+ } \ +} 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]"
Okay.
+ */ +# 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...
Okay.
+ return -ENOMEM; \
return -1;
Okay.
Erik
I'm suggesting to squash the following (let me know if you agree):
Okay. I 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)