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@xxxxxxxxxxxxxx>
> ---
> 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)
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
