Re: [libvirt] [PATCHv2 2/4] Add wrapper macros around nla_nest/nla_put to make code more readable

5 Sep 2018

On Wed, Sep 05, 2018 at 04:36:28PM +0800, Shi Lei wrote:
...
This patch adds wrapper macros around nla_nest*/nla_put* which apply to
virNetlinkNewLink and virNetDevSetVfConfig and virNetDevVPortProfileOpSetLink.
Signed-off-by: Shi Lei <shi_lei@massclouds.com>
---
 src/util/virnetdev.c             |  54 ++++++--------
 src/util/virnetdevvportprofile.c | 117 ++++++++++---------------------
 src/util/virnetlink.c            |  74 +++++++------------
 src/util/virnetlink.h            |  52 ++++++++++++++
 4 files changed, 134 insertions(+), 163 deletions(-)

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 8eac419..892a147 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1657,11 +1657,11 @@ virNetDevSetVfConfig(const char *ifname, int vf,
 {
     int rc = -1;
     char macstr[VIR_MAC_STRING_BUFLEN];
-    struct nlmsghdr *resp = NULL;
     struct nlmsgerr *err;
     unsigned int recvbuflen = 0;
-    struct nl_msg *nl_msg;
     struct nlattr *vfinfolist, *vfinfo;
+    VIR_AUTOPTR(virNetlinkMsg) nl_msg = NULL;
+    VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
changes related to AUTO{PTR,FREE} should completely separate, even from the
series itself.
...
struct ifinfomsg ifinfo = {
         .ifi_family = AF_UNSPEC,
         .ifi_index  = -1,
@@ -1676,47 +1676,41 @@ virNetDevSetVfConfig(const char *ifname, int vf,
         return rc;
     }
-    if (nlmsg_append(nl_msg,  &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0)
-        goto buffer_too_small;
-
-    if (ifname &&
-        nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0)
as I mentioned in my reply to the previous patch, I'd first replace ocurrences
like ^these with nla_put_string for consistency and also making reviewer's life
easier.
...
-        goto buffer_too_small;
-
+    if (nlmsg_append(nl_msg,  &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nlmsg_append error"));
+        return rc;
+    }
-    if (!(vfinfolist = nla_nest_start(nl_msg, IFLA_VFINFO_LIST)))
-        goto buffer_too_small;
+    NETLINK_MSG_PUT_STRING(nl_msg, IFLA_IFNAME, ifname);
-    if (!(vfinfo = nla_nest_start(nl_msg, IFLA_VF_INFO)))
-        goto buffer_too_small;
+    NETLINK_MSG_NEST_START(nl_msg, vfinfolist, IFLA_VFINFO_LIST);
+    NETLINK_MSG_NEST_START(nl_msg, vfinfo, IFLA_VF_INFO);
if (macaddr) {
         struct ifla_vf_mac ifla_vf_mac = {
-             .vf = vf,
-             .mac = { 0, },
+            .vf = vf,
+            .mac = { 0, },
         };
virMacAddrGetRaw(macaddr, ifla_vf_mac.mac);
-        if (nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac),
-                    &ifla_vf_mac) < 0)
-            goto buffer_too_small;
+        NETLINK_MSG_PUT(nl_msg, IFLA_VF_MAC,
+                        sizeof(ifla_vf_mac), &ifla_vf_mac);
     }
if (vlanid >= 0) {
         struct ifla_vf_vlan ifla_vf_vlan = {
-             .vf = vf,
-             .vlan = vlanid,
-             .qos = 0,
+            .vf = vf,
+            .vlan = vlanid,
+            .qos = 0,
         };
-        if (nla_put(nl_msg, IFLA_VF_VLAN, sizeof(ifla_vf_vlan),
-                    &ifla_vf_vlan) < 0)
-            goto buffer_too_small;
+        NETLINK_MSG_PUT(nl_msg, IFLA_VF_VLAN,
+                        sizeof(ifla_vf_vlan), &ifla_vf_vlan);
     }
-    nla_nest_end(nl_msg, vfinfo);
-    nla_nest_end(nl_msg, vfinfolist);
+    NETLINK_MSG_NEST_END(nl_msg, vfinfo);
+    NETLINK_MSG_NEST_END(nl_msg, vfinfolist);
if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0,
                           NETLINK_ROUTE, 0) < 0)
@@ -1767,20 +1761,12 @@ virNetDevSetVfConfig(const char *ifname, int vf,
               ifname, vf,
               macaddr ? virMacAddrFormat(macaddr, macstr) : "(unchanged)",
               vlanid, rc < 0 ? "Fail" : "Success");
-
-    nlmsg_free(nl_msg);
-    VIR_FREE(resp);
     return rc;
malformed_resp:
     virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                    _("malformed netlink response message"));
     goto cleanup;
-
- buffer_too_small:
-    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                   _("allocated netlink buffer is too small"));
-    goto cleanup;
 }
static int
diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c
index 3ebf757..8574571 100644
--- a/src/util/virnetdevvportprofile.c
+++ b/src/util/virnetdevvportprofile.c
@@ -641,8 +641,6 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex,
                                int32_t vf,
                                uint8_t op)
 {
-    int rc = -1;
-    struct nlmsghdr *resp = NULL;
     struct nlmsgerr *err;
     struct ifinfomsg ifinfo = {
         .ifi_family = AF_UNSPEC,
@@ -651,12 +649,14 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex,
     unsigned int recvbuflen = 0;
     int src_pid = 0;
     uint32_t dst_pid = 0;
-    struct nl_msg *nl_msg;
     struct nlattr *vfports = NULL, *vfport;
     char macStr[VIR_MAC_STRING_BUFLEN];
     char hostUUIDStr[VIR_UUID_STRING_BUFLEN];
     char instanceUUIDStr[VIR_UUID_STRING_BUFLEN];
     const char *opName;
+    int vfport_type = IFLA_PORT_SELF;
+    VIR_AUTOPTR(virNetlinkMsg) nl_msg = NULL;
+    VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
again the VIR_AUTO{PTR,FREE} stuff...
...
switch (op) {
     case PORT_REQUEST_PREASSOCIATE:
@@ -691,24 +691,21 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex,
     nl_msg = nlmsg_alloc_simple(RTM_SETLINK, NLM_F_REQUEST);
     if (!nl_msg) {
         virReportOOMError();
-        return rc;
+        return -1;
     }
-    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", _("nlmsg_append error"));
+        return -1;
+    }
-    if (ifname &&
-        nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0)
-        goto buffer_too_small;
+    NETLINK_MSG_PUT_STRING(nl_msg, IFLA_IFNAME, ifname);
if (macaddr || vlanid >= 0) {
         struct nlattr *vfinfolist, *vfinfo;
-        if (!(vfinfolist = nla_nest_start(nl_msg, IFLA_VFINFO_LIST)))
-            goto buffer_too_small;
-
-        if (!(vfinfo = nla_nest_start(nl_msg, IFLA_VF_INFO)))
-            goto buffer_too_small;
+        NETLINK_MSG_NEST_START(nl_msg, vfinfolist, IFLA_VFINFO_LIST);
+        NETLINK_MSG_NEST_START(nl_msg, vfinfo, IFLA_VF_INFO);
if (macaddr) {
             struct ifla_vf_mac ifla_vf_mac = {
@@ -718,9 +715,8 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex,
virMacAddrGetRaw(macaddr, ifla_vf_mac.mac);
-            if (nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac),
-                        &ifla_vf_mac) < 0)
-                goto buffer_too_small;
+            NETLINK_MSG_PUT(nl_msg, IFLA_VF_MAC,
+                            sizeof(ifla_vf_mac), &ifla_vf_mac);
         }
if (vlanid >= 0) {
@@ -730,77 +726,49 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex,
                 .qos = 0,
             };
-            if (nla_put(nl_msg, IFLA_VF_VLAN, sizeof(ifla_vf_vlan),
-                        &ifla_vf_vlan) < 0)
-                goto buffer_too_small;
+            NETLINK_MSG_PUT(nl_msg, IFLA_VF_VLAN,
+                            sizeof(ifla_vf_vlan), &ifla_vf_vlan);
         }
-        nla_nest_end(nl_msg, vfinfo);
-        nla_nest_end(nl_msg, vfinfolist);
-    }
-
-    if (vf == PORT_SELF_VF && nltarget_kernel) {
-        if (!(vfport = nla_nest_start(nl_msg, IFLA_PORT_SELF)))
-            goto buffer_too_small;
-    } else {
-        if (!(vfports = nla_nest_start(nl_msg, IFLA_VF_PORTS)))
-            goto buffer_too_small;
-
-        /* begin nesting vfports */
-        if (!(vfport = nla_nest_start(nl_msg, IFLA_VF_PORT)))
-            goto buffer_too_small;
-    }
at this point it got quite difficult to keep track of all the replacements, so
I'd suggest making the replacements one at a time, first one macro, then
another, etc.

...
...
/**
diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h
index 8163a81..2e64570 100644
--- a/src/util/virnetlink.h
+++ b/src/util/virnetlink.h
@@ -48,6 +48,58 @@ 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", \
+                       _("nla_nest_start error [" #attrtype "]")); \
+        return -1; \
+    } \
+} while(0)
+
+# define NETLINK_MSG_NEST_END(msg, container) \
+do { nla_nest_end(msg, container); } while(0)
+
+/*
+ * When data is an rvalue, compiler will report error as below:
+ * "the address of [...] will always evaluate as 'true' [-Werror=address]"
+ * Add a dummy(as an lvalue) to solve it.
Okay, let's try rewording this in a more compact manner below...
...
+ */
+# define NETLINK_MSG_PUT(msg, attrtype, datalen, data) \
+do { \
how about:
/* @data may not always be a pointer type, therefore making compilers complain
 * about the check below, so let's use an intermediary pointer type */
...
+    const void *dummy = data; \
let's rename @dummy to dataptr to make it more explicit
...
+    if (dummy && nla_put(msg, attrtype, datalen, data) < 0) { \
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \
+                       _("nla_put error [" #attrtype "]")); \
+        return -1; \
+    } \
+} while(0)
+
+# define NETLINK_MSG_PUT_STRING(msg, attrtype, str) \
+do { \
+    const void *dummy = str; \
+    if (dummy && nla_put_string(msg, attrtype, str) < 0) { \
+        virReportError(VIR_ERR_INTERNAL_ERROR, \
+                       _("nla_put_string error [" #attrtype "]: '%s'"), \
+                       str); \
I don't think that changing the error is necessary, all of the variants
essentially call nla_put and that one either fails with -EINVAL (not our case,
since we always provide a valid datalen) or -ENOMEM, so the original error
stating that the buffer wasn't large enough is sufficient.
...
+        return -1; \
+    } \
+} while(0)
I'm not entirely sold on these MSG_PUT_X macros, but I don't have a reasonable
argument against either, so let's leave them in. Alternatively, we could have

#define NETLINK_MSG_PUT_STRING(msg, attrtype, str) \
    NETLINK_MSG_PUT(msg, attrtype, strlen(str) + 1, str)

and that would make things more clean, afterall that is what all the
nla_put_foo will do anyway...

Erik
...
+
+# define NETLINK_MSG_PUT_U32PTR(msg, attrtype, ptr) \
+do { \
+    const void *dummy = ptr; \
+    if (dummy && nla_put_u32(msg, attrtype, *ptr) < 0) { \
+        virReportError(VIR_ERR_INTERNAL_ERROR, \
+                       _("nla_put_u32 error [" #attrtype "]: '%u'"), \
+                       *ptr); \
+        return -1; \
+    } \
+} 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] [PATCHv2 2/4] Add wrapper macros around nla_nest*/nla_put* to make code more readable

Erik Skultety

Re: [libvirt] [PATCHv2 2/4] Add wrapper macros around nla_nest/nla_put to make code more readable