Extract common code as helper function virNetlinkTalk, then simplify
the functions virNetlink[DumpLink|NewLink|DelLink|GetNeighbor].

Signed-off-by: Shi Lei <shi_lei@massclouds.com>
---
 src/util/virnetlink.c | 225 +++++++++++++++++-------------------------
 src/util/virnetlink.h |   4 +-
 2 files changed, 94 insertions(+), 135 deletions(-)

diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
index fdcb0dc0..2936a3ef 100644
--- a/src/util/virnetlink.c
+++ b/src/util/virnetlink.c
@@ -353,6 +353,52 @@ int virNetlinkCommand(struct nl_msg *nl_msg,
     return 0;
 }
 
+
+static int
+virNetlinkTalk(const char *ifname,
+               virNetlinkMsg *nl_msg,
+               uint32_t src_pid,
+               uint32_t dst_pid,
+               struct nlmsghdr **resp,
+               unsigned int *resp_len,
+               int *error,
+               virNetlinkTalkFallback fallback)
+{
+    if (virNetlinkCommand(nl_msg, resp, resp_len,
+                          src_pid, dst_pid, NETLINK_ROUTE, 0) < 0)
+        return -1;
+
+    if (*resp_len < NLMSG_LENGTH(0) || resp == NULL)
+        goto malformed_resp;
+
+    if ((*resp)->nlmsg_type == NLMSG_ERROR) {
+        struct nlmsgerr *err = (struct nlmsgerr *) NLMSG_DATA(resp);
+        if ((*resp)->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
+            goto malformed_resp;
+
+        if (-err->error == EOPNOTSUPP && fallback)
+            return fallback(ifname);
+
+        if (err->error < 0) {
+            if (error)
+                *error = err->error;
+            else
+                virReportSystemError(-err->error,
+                                     "%s", _("netlink error"));
+
+            return -1;
+        }
+    }
+
+    return 0;
+
+ malformed_resp:
+    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                   _("malformed netlink response message"));
+    return -1;
+}
+
+
 int
 virNetlinkDumpCommand(struct nl_msg *nl_msg,
                       virNetlinkDumpCallback callback,
@@ -396,6 +442,7 @@ virNetlinkDumpCommand(struct nl_msg *nl_msg,
     return 0;
 }
 
+
 /**
  * virNetlinkDumpLink:
  *
@@ -420,15 +467,14 @@ virNetlinkDumpLink(const char *ifname, int ifindex,
                    void **nlData, struct nlattr **tb,
                    uint32_t src_pid, uint32_t dst_pid)
 {
-    int rc = -1;
-    struct nlmsgerr *err;
     struct ifinfomsg ifinfo = {
         .ifi_family = AF_UNSPEC,
         .ifi_index  = ifindex
     };
-    unsigned int recvbuflen;
     g_autoptr(virNetlinkMsg) nl_msg = NULL;
     g_autofree struct nlmsghdr *resp = NULL;
+    unsigned int resp_len = 0;
+    int error = 0;
 
     if (ifname && ifindex <= 0 && virNetDevGetIndex(ifname, &ifindex) < 0)
         return -1;
@@ -459,46 +505,23 @@ virNetlinkDumpLink(const char *ifname, int ifindex,
     }
 # endif
 
-    if (virNetlinkCommand(nl_msg, &resp, &recvbuflen,
-                          src_pid, dst_pid, NETLINK_ROUTE, 0) < 0)
+    if (virNetlinkTalk(ifname, nl_msg, src_pid, dst_pid,
+                       &resp, &resp_len, &error, NULL) < 0) {
+        virReportSystemError(error,
+                             _("error dumping %s (%d) interface"),
+                             ifname, ifindex);
         return -1;
+    }
 
-    if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL)
-        goto malformed_resp;
-
-    switch (resp->nlmsg_type) {
-    case NLMSG_ERROR:
-        err = (struct nlmsgerr *)NLMSG_DATA(resp);
-        if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
-            goto malformed_resp;
-
-        if (err->error) {
-            virReportSystemError(-err->error,
-                                 _("error dumping %s (%d) interface"),
-                                 ifname, ifindex);
-            return -1;
-        }
-        break;
-
-    case GENL_ID_CTRL:
-    case NLMSG_DONE:
-        rc = nlmsg_parse(resp, sizeof(struct ifinfomsg),
-                         tb, IFLA_MAX, NULL);
-        if (rc < 0)
-            goto malformed_resp;
-        break;
-
-    default:
-        goto malformed_resp;
+    if ((resp->nlmsg_type != GENL_ID_CTRL && resp->nlmsg_type != NLMSG_DONE) ||
+        nlmsg_parse(resp, sizeof(struct ifinfomsg), tb, IFLA_MAX, NULL) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("malformed netlink response message"));
+        return -1;
     }
 
     *nlData = g_steal_pointer(&resp);
     return 0;
-
- malformed_resp:
-    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                   _("malformed netlink response message"));
-    return rc;
 }
 
 
@@ -523,13 +546,12 @@ virNetlinkNewLink(const char *ifname,
                   virNetlinkNewLinkDataPtr extra_args,
                   int *error)
 {
-    struct nlmsgerr *err;
     struct nlattr *linkinfo = NULL;
     struct nlattr *infodata = NULL;
-    unsigned int buflen;
     struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
     g_autoptr(virNetlinkMsg) nl_msg = NULL;
     g_autofree struct nlmsghdr *resp = NULL;
+    unsigned int resp_len = 0;
 
     *error = 0;
 
@@ -591,37 +613,17 @@ virNetlinkNewLink(const char *ifname,
         }
     }
 
-    if (virNetlinkCommand(nl_msg, &resp, &buflen, 0, 0, NETLINK_ROUTE, 0) < 0)
+    if (virNetlinkTalk(ifname, nl_msg, 0, 0,
+                       &resp, &resp_len, error, NULL) < 0)
         return -1;
 
-    if (buflen < NLMSG_LENGTH(0) || resp == NULL)
-        goto malformed_resp;
-
-    switch (resp->nlmsg_type) {
-    case NLMSG_ERROR:
-        err = (struct nlmsgerr *)NLMSG_DATA(resp);
-        if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
-            goto malformed_resp;
-
-        if (err->error < 0) {
-            *error = err->error;
-            return -1;
-        }
-        break;
-
-    case NLMSG_DONE:
-        break;
-
-    default:
-        goto malformed_resp;
+    if (resp->nlmsg_type != NLMSG_DONE) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("malformed netlink response message"));
+        return -1;
     }
 
     return 0;
-
- malformed_resp:
-    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                   _("malformed netlink response message"));
-    return -1;
 }
 
 
@@ -641,13 +643,13 @@ virNetlinkNewLink(const char *ifname,
  * Returns 0 on success, -1 on fatal error.
  */
 int
-virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback)
+virNetlinkDelLink(const char *ifname, virNetlinkTalkFallback fallback)
 {
-    struct nlmsgerr *err;
     struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
-    unsigned int recvbuflen;
     g_autoptr(virNetlinkMsg) nl_msg = NULL;
     g_autofree struct nlmsghdr *resp = NULL;
+    unsigned int resp_len = 0;
+    int error = 0;
 
     nl_msg = nlmsg_alloc_simple(RTM_DELLINK, NLM_F_REQUEST);
     if (!nl_msg) {
@@ -659,44 +661,21 @@ virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback)
 
     NETLINK_MSG_PUT(nl_msg, IFLA_IFNAME, (strlen(ifname) + 1), ifname);
 
-    if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0,
-                          NETLINK_ROUTE, 0) < 0) {
+    if (virNetlinkTalk(ifname, nl_msg, 0, 0,
+                       &resp, &resp_len, &error, fallback) < 0) {
+        virReportSystemError(error,
+                             _("error destroying network device %s"),
+                             ifname);
         return -1;
     }
 
-    if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL)
-        goto malformed_resp;
-
-    switch (resp->nlmsg_type) {
-    case NLMSG_ERROR:
-        err = (struct nlmsgerr *)NLMSG_DATA(resp);
-        if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
-            goto malformed_resp;
-
-        if (-err->error == EOPNOTSUPP && fallback)
-            return fallback(ifname);
-
-        if (err->error) {
-            virReportSystemError(-err->error,
-                                 _("error destroying network device %s"),
-                                 ifname);
-            return -1;
-        }
-        break;
-
-    case NLMSG_DONE:
-        break;
-
-    default:
-        goto malformed_resp;
+    if (resp->nlmsg_type != NLMSG_DONE) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("malformed netlink response message"));
+        return -1;
     }
 
     return 0;
-
- malformed_resp:
-    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                   _("malformed netlink response message"));
-    return -1;
 }
 
 /**
@@ -712,18 +691,18 @@ virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback)
  *
  * Get neighbor table entry from netlink.
  *
- * Returns 0 on success, -1 on fatal error.
+ * Returns length of the raw data from netlink on success, -1 on fatal error.
  */
 int
 virNetlinkGetNeighbor(void **nlData, uint32_t src_pid, uint32_t dst_pid)
 {
-    struct nlmsgerr *err;
     struct ndmsg ndinfo = {
         .ndm_family = AF_UNSPEC,
     };
-    unsigned int recvbuflen;
     g_autoptr(virNetlinkMsg) nl_msg = NULL;
     g_autofree struct nlmsghdr *resp = NULL;
+    unsigned int resp_len = 0;
+    int error = 0;
 
     nl_msg = nlmsg_alloc_simple(RTM_GETNEIGH, NLM_F_DUMP | NLM_F_REQUEST);
     if (!nl_msg) {
@@ -733,40 +712,20 @@ virNetlinkGetNeighbor(void **nlData, uint32_t src_pid, uint32_t dst_pid)
 
     NETLINK_MSG_APPEND(nl_msg, sizeof(ndinfo), &ndinfo);
 
-    if (virNetlinkCommand(nl_msg, &resp, &recvbuflen,
-                          src_pid, dst_pid, NETLINK_ROUTE, 0) < 0)
+    if (virNetlinkTalk(NULL, nl_msg, src_pid, dst_pid,
+                       &resp, &resp_len, &error, NULL) < 0) {
+        virReportSystemError(error, "%s", _("error dumping"));
         return -1;
+    }
 
-    if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL)
-        goto malformed_resp;
-
-    switch (resp->nlmsg_type) {
-    case NLMSG_ERROR:
-        err = (struct nlmsgerr *)NLMSG_DATA(resp);
-        if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
-            goto malformed_resp;
-
-        if (err->error) {
-            virReportSystemError(-err->error,
-                                 "%s", _("error dumping"));
-            return -1;
-        }
-        break;
-
-    case RTM_NEWNEIGH:
-        break;
-
-    default:
-        goto malformed_resp;
+    if (resp->nlmsg_type != RTM_NEWNEIGH) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("malformed netlink response message"));
+        return -1;
     }
 
     *nlData = g_steal_pointer(&resp);
-    return recvbuflen;
-
- malformed_resp:
-    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                   _("malformed netlink response message"));
-    return -1;
+    return resp_len;
 }
 
 int
@@ -1300,7 +1259,7 @@ virNetlinkDumpLink(const char *ifname G_GNUC_UNUSED,
 
 int
 virNetlinkDelLink(const char *ifname G_GNUC_UNUSED,
-                  virNetlinkDelLinkFallback fallback G_GNUC_UNUSED)
+                  virNetlinkTalkFallback fallback G_GNUC_UNUSED)
 {
     virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported));
     return -1;
diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h
index 966d6db3..cab685fe 100644
--- a/src/util/virnetlink.h
+++ b/src/util/virnetlink.h
@@ -69,9 +69,9 @@ int virNetlinkNewLink(const char *ifname,
                       virNetlinkNewLinkDataPtr data,
                       int *error);
 
-typedef int (*virNetlinkDelLinkFallback)(const char *ifname);
+typedef int (*virNetlinkTalkFallback)(const char *ifname);
 
-int virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback);
+int virNetlinkDelLink(const char *ifname, virNetlinkTalkFallback fallback);
 
 int virNetlinkGetErrorCode(struct nlmsghdr *resp, unsigned int recvbuflen);
 
-- 
2.25.1

On 1/6/21 3:40 AM, Shi Lei wrote:
> Extract common code as helper function virNetlinkTalk, then simplify
> the functions virNetlink[DumpLink|NewLink|DelLink|GetNeighbor].
> 
> Signed-off-by: Shi Lei <shi_lei@massclouds.com>
> ---
>   src/util/virnetlink.c | 225 +++++++++++++++++-------------------------
>   src/util/virnetlink.h |   4 +-
>   2 files changed, 94 insertions(+), 135 deletions(-)
> 
> diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
> index fdcb0dc0..2936a3ef 100644
> --- a/src/util/virnetlink.c
> +++ b/src/util/virnetlink.c
> @@ -353,6 +353,52 @@ int virNetlinkCommand(struct nl_msg *nl_msg,
>       return 0;
>   }
>   
> +
> +static int
> +virNetlinkTalk(const char *ifname,
> +               virNetlinkMsg *nl_msg,
> +               uint32_t src_pid,
> +               uint32_t dst_pid,
> +               struct nlmsghdr **resp,
> +               unsigned int *resp_len,
> +               int *error,
> +               virNetlinkTalkFallback fallback)
> +{
> +    if (virNetlinkCommand(nl_msg, resp, resp_len,
> +                          src_pid, dst_pid, NETLINK_ROUTE, 0) < 0)
> +        return -1;
> +
> +    if (*resp_len < NLMSG_LENGTH(0) || resp == NULL)

This needs to be *resp == NULL, because now we're passing a pointer to a 
pointer.

> +        goto malformed_resp;
> +
> +    if ((*resp)->nlmsg_type == NLMSG_ERROR) {
> +        struct nlmsgerr *err = (struct nlmsgerr *) NLMSG_DATA(resp);

And this needs to be NLMSG_DATA(*resp);
Also, might be worth putting an empty line here to create two blocks: 
one with variable declaration and the other with the code.


> +        if ((*resp)->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
> +            goto malformed_resp;
> +
> +        if (-err->error == EOPNOTSUPP && fallback)
> +            return fallback(ifname);
> +
> +        if (err->error < 0) {
> +            if (error)
> +                *error = err->error;
> +            else
> +                virReportSystemError(-err->error,
> +                                     "%s", _("netlink error"));

Since this is a two line body, it should be wrapped in curly braces 
(according to our coding style) which means that both bodies should have 
them (we don't really like one body having them while the other not).

> +
> +            return -1;
> +        }
> +    }
> +
> +    return 0;
> +
> + malformed_resp:
> +    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                   _("malformed netlink response message"));
> +    return -1;
> +}
> +
> +
>   int
>   virNetlinkDumpCommand(struct nl_msg *nl_msg,
>                         virNetlinkDumpCallback callback,
> @@ -396,6 +442,7 @@ virNetlinkDumpCommand(struct nl_msg *nl_msg,
>       return 0;
>   }
>   
> +
>   /**
>    * virNetlinkDumpLink:
>    *
> @@ -420,15 +467,14 @@ virNetlinkDumpLink(const char *ifname, int ifindex,
>                      void **nlData, struct nlattr **tb,
>                      uint32_t src_pid, uint32_t dst_pid)
>   {
> -    int rc = -1;
> -    struct nlmsgerr *err;
>       struct ifinfomsg ifinfo = {
>           .ifi_family = AF_UNSPEC,
>           .ifi_index  = ifindex
>       };
> -    unsigned int recvbuflen;
>       g_autoptr(virNetlinkMsg) nl_msg = NULL;
>       g_autofree struct nlmsghdr *resp = NULL;
> +    unsigned int resp_len = 0;
> +    int error = 0;
>   
>       if (ifname && ifindex <= 0 && virNetDevGetIndex(ifname, &ifindex) < 0)
>           return -1;
> @@ -459,46 +505,23 @@ virNetlinkDumpLink(const char *ifname, int ifindex,
>       }
>   # endif
>   
> -    if (virNetlinkCommand(nl_msg, &resp, &recvbuflen,
> -                          src_pid, dst_pid, NETLINK_ROUTE, 0) < 0)
> +    if (virNetlinkTalk(ifname, nl_msg, src_pid, dst_pid,
> +                       &resp, &resp_len, &error, NULL) < 0) {
> +        virReportSystemError(error,
> +                             _("error dumping %s (%d) interface"),
> +                             ifname, ifindex);
>           return -1;
> +    }
>   
> -    if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL)
> -        goto malformed_resp;
> -
> -    switch (resp->nlmsg_type) {
> -    case NLMSG_ERROR:
> -        err = (struct nlmsgerr *)NLMSG_DATA(resp);
> -        if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
> -            goto malformed_resp;
> -
> -        if (err->error) {
> -            virReportSystemError(-err->error,
> -                                 _("error dumping %s (%d) interface"),
> -                                 ifname, ifindex);
> -            return -1;
> -        }
> -        break;
> -
> -    case GENL_ID_CTRL:
> -    case NLMSG_DONE:
> -        rc = nlmsg_parse(resp, sizeof(struct ifinfomsg),
> -                         tb, IFLA_MAX, NULL);
> -        if (rc < 0)
> -            goto malformed_resp;
> -        break;
> -
> -    default:
> -        goto malformed_resp;
> +    if ((resp->nlmsg_type != GENL_ID_CTRL && resp->nlmsg_type != NLMSG_DONE) ||
> +        nlmsg_parse(resp, sizeof(struct ifinfomsg), tb, IFLA_MAX, NULL) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("malformed netlink response message"));
> +        return -1;
>       }
>   
>       *nlData = g_steal_pointer(&resp);
>       return 0;
> -
> - malformed_resp:
> -    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                   _("malformed netlink response message"));
> -    return rc;
>   }
>   
>   
> @@ -523,13 +546,12 @@ virNetlinkNewLink(const char *ifname,
>                     virNetlinkNewLinkDataPtr extra_args,
>                     int *error)
>   {
> -    struct nlmsgerr *err;
>       struct nlattr *linkinfo = NULL;
>       struct nlattr *infodata = NULL;
> -    unsigned int buflen;
>       struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
>       g_autoptr(virNetlinkMsg) nl_msg = NULL;
>       g_autofree struct nlmsghdr *resp = NULL;
> +    unsigned int resp_len = 0;
>   
>       *error = 0;
>   
> @@ -591,37 +613,17 @@ virNetlinkNewLink(const char *ifname,
>           }
>       }
>   
> -    if (virNetlinkCommand(nl_msg, &resp, &buflen, 0, 0, NETLINK_ROUTE, 0) < 0)
> +    if (virNetlinkTalk(ifname, nl_msg, 0, 0,
> +                       &resp, &resp_len, error, NULL) < 0)
>           return -1;
>   
> -    if (buflen < NLMSG_LENGTH(0) || resp == NULL)
> -        goto malformed_resp;
> -
> -    switch (resp->nlmsg_type) {
> -    case NLMSG_ERROR:
> -        err = (struct nlmsgerr *)NLMSG_DATA(resp);
> -        if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
> -            goto malformed_resp;
> -
> -        if (err->error < 0) {
> -            *error = err->error;
> -            return -1;
> -        }
> -        break;
> -
> -    case NLMSG_DONE:
> -        break;
> -
> -    default:
> -        goto malformed_resp;
> +    if (resp->nlmsg_type != NLMSG_DONE) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("malformed netlink response message"));
> +        return -1;
>       }

This is not correct IMO. The way that control flows through this 
function before your patch is (I'm trying to create virbr0 using 'virsh 
net-start default'):

1) virNetlinkCommand() suceeds,
2) resp->nlmsg_type == NLMSG_ERROR even though the bridge was created,
3) err->error is 0 hence we hit 'break' and fall out of the switch and ..

>   
>       return 0;

.. return 0. With your patch, I get the "malrofmed netlink response 
message" error. Reading netlink(7) manpage lets more light into this:

   For reliable transfer the sender can request an acknowledgement from
   the receiver by setting the NLM_F_ACK flag. An acknowledgment is an
   NLMSG_ERROR packet with the error field set to 0.

Which is exactly what I'm seeing, but what I don't understand is why we 
are getting this ack message since NLM_F_ACK flag was not set.

Michal