1. Patchew
  2. Libvirt
  3. [libvirt] [PATCHv3 0/3] Add virNetlinkNewLink helper for simplifying virNetDev*Create
  4. View patch

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

Shi Lei posted 3 patches 6 years, 2 months ago
There is a newer version of this series
[libvirt] [PATCHv3 2/3] util: netlink: Add wrapper macros to make virNetlinkNewLink more readable
Posted by Shi Lei 6 years, 2 months ago 
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"));
+        return -ENOMEM;
+    }
 
-    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);
+        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 "]")); \
+        return -ENOMEM; \
+    } \
+} 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]"
+ */
+# 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; \
+    } \
+} while(0)
+
+
 int virNetlinkStartup(void);
 void virNetlinkShutdown(void);
 
-- 
2.17.1


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv3 2/3] util: netlink: Add wrapper macros to make virNetlinkNewLink more readable
Posted by Erik Skultety 6 years, 2 months ago 
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.

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.