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