[PATCHv3 4/4] netlink: Introduce a helper function to simplify netlink functions

Michal Privoznik mprivozn at redhat.com
Mon Jan 11 11:26:27 UTC 2021


On 1/11/21 3:23 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 at massclouds.com>
> ---
>   src/util/virnetlink.c | 232 ++++++++++++++++++------------------------
>   src/util/virnetlink.h |   4 +-
>   2 files changed, 101 insertions(+), 135 deletions(-)
> 
> diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
> index fdcb0dc0..650acff7 100644
> --- a/src/util/virnetlink.c
> +++ b/src/util/virnetlink.c
> @@ -353,6 +353,54 @@ int virNetlinkCommand(struct nl_msg *nl_msg,
>       return 0;
>   }
>   
> +

I guess we should document that NLMSG_ERROR and err->error == 0 is a 
valid case and no error. How about:

/**
  * virNetlinkTalk:
  * @ifname: name of the link
  * @nl_msg: pointer to netlink message
  * @src_pid: pid used for nl_pid of the local end of the netlink message
  *           (0 == "use getpid()")
  * @dst_pid: pid of destination nl_pid if the kernel
  *           is not the target of the netlink message but it is to be
  *           sent to another process (0 if sending to the kernel)
  * @resp: pointer to pointer where response buffer will be allocated
  * @resp_len: pointer to integer holding the size of the response buffer
  *            on return of the function
  * @error: pointer to store netlink error (-errno)
  * @fallback: pointer to an alternate function that will be called in case
  *            netlink fails with EOPNOTSUPP (any other error will simply be
  *            treated as an error)
  *
  * Simple wrapper around virNetlinkCommand(). The returned netlink message
  * is allocated at @resp. Please note that according to netlink(7) man 
page,
  * reply with type of NLMSG_ERROR and @error == 0 is an acknowledgment and
  * thus not an error.
  *
  * Returns: 0 on success,
  *         -1 otherwise (error reported if @error == NULL)
  */


> +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;
> +
> +        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;

So this sets @error to a negative number. [1]

> +            else
> +                virReportSystemError(-err->error, "%s", _("netlink error"));
> +
> +            return -1;
> +        }

And here I'd put:

         /* According to netlink(7) man page NLMSG_ERROR packet with error
          * field set to 0 is an acknowledgment packet and thus not an 
error. */


> +    }
> +
> +    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 +444,7 @@ virNetlinkDumpCommand(struct nl_msg *nl_msg,
>       return 0;
>   }
>   
> +
>   /**
>    * virNetlinkDumpLink:
>    *
> @@ -420,15 +469,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 +507,25 @@ 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,

1: and here it is used as if it was positive. We need -error. And in the 
rest of places too.

Anyway, I can fix that before pushing, so that you don't have to send 
another version. Do you agree if this is squashed in?

diff --git c/src/util/virnetlink.c w/src/util/virnetlink.c
index 650acff7d7..9bd7339054 100644
--- c/src/util/virnetlink.c
+++ w/src/util/virnetlink.c
@@ -354,6 +354,31 @@ int virNetlinkCommand(struct nl_msg *nl_msg,
  }


+/**
+ * virNetlinkTalk:
+ * @ifname: name of the link
+ * @nl_msg: pointer to netlink message
+ * @src_pid: pid used for nl_pid of the local end of the netlink message
+ *           (0 == "use getpid()")
+ * @dst_pid: pid of destination nl_pid if the kernel
+ *           is not the target of the netlink message but it is to be
+ *           sent to another process (0 if sending to the kernel)
+ * @resp: pointer to pointer where response buffer will be allocated
+ * @resp_len: pointer to integer holding the size of the response buffer
+ *            on return of the function
+ * @error: pointer to store netlink error (-errno)
+ * @fallback: pointer to an alternate function that will be called in case
+ *            netlink fails with EOPNOTSUPP (any other error will simply be
+ *            treated as an error)
+ *
+ * Simple wrapper around virNetlinkCommand(). The returned netlink message
+ * is allocated at @resp. Please note that according to netlink(7) man 
page,
+ * reply with type of NLMSG_ERROR and @error == 0 is an acknowledgment and
+ * thus not an error.
+ *
+ * Returns: 0 on success,
+ *         -1 otherwise (error reported if @error == NULL)
+ */
  static int
  virNetlinkTalk(const char *ifname,
                 virNetlinkMsg *nl_msg,
@@ -390,6 +415,9 @@ virNetlinkTalk(const char *ifname,

              return -1;
          }
+
+        /* According to netlink(7) man page NLMSG_ERROR packet with error
+         * field set to 0 is an acknowledgment packet and thus not an 
error. */
      }

      return 0;
@@ -509,7 +537,7 @@ virNetlinkDumpLink(const char *ifname, int ifindex,

      if (virNetlinkTalk(ifname, nl_msg, src_pid, dst_pid,
                         &resp, &resp_len, &error, NULL) < 0) {
-        virReportSystemError(error,
+        virReportSystemError(-error,
                               _("error dumping %s (%d) interface"),
                               ifname, ifindex);
          return -1;
@@ -668,7 +696,7 @@ virNetlinkDelLink(const char *ifname, 
virNetlinkTalkFallback fallback)

      if (virNetlinkTalk(ifname, nl_msg, 0, 0,
                         &resp, &resp_len, &error, fallback) < 0) {
-        virReportSystemError(error,
+        virReportSystemError(-error,
                               _("error destroying network device %s"),
                               ifname);
          return -1;
@@ -720,7 +748,7 @@ virNetlinkGetNeighbor(void **nlData, uint32_t 
src_pid, uint32_t dst_pid)

      if (virNetlinkTalk(NULL, nl_msg, src_pid, dst_pid,
                         &resp, &resp_len, &error, NULL) < 0) {
-        virReportSystemError(error, "%s", _("error dumping"));
+        virReportSystemError(-error, "%s", _("error dumping neighbor 
table"));
          return -1;
      }



Michal




More information about the libvir-list mailing list