This patch introduces virNetlinkNewLink helper which wraps the common
libnl/netlink code to create a new link.

Signed-off-by: Shi Lei <shi_lei@massclouds.com>
---
 src/libvirt_private.syms |   1 +
 src/util/virnetlink.c    | 117 +++++++++++++++++++++++++++++++++++++++
 src/util/virnetlink.h    |  13 +++++
 3 files changed, 131 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 0fc5314..a0d229a 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2446,6 +2446,7 @@ virNetlinkEventServiceStop;
 virNetlinkEventServiceStopAll;
 virNetlinkGetErrorCode;
 virNetlinkGetNeighbor;
+virNetlinkNewLink;
 virNetlinkShutdown;
 virNetlinkStartup;
 
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
index 8f06446..d53cc73 100644
--- a/src/util/virnetlink.c
+++ b/src/util/virnetlink.c
@@ -488,6 +488,123 @@ virNetlinkDumpLink(const char *ifname, int ifindex,
 }
 
 
+/**
+ * virNetlinkNewLink:
+ *
+ * @ifname: name of the link
+ * @type: the type of the device, i.e. "bridge", "macvtap", "macvlan"
+ * @extra_args: the extra args for creating the netlink interface
+ * @error: netlink error code
+ *
+ * A generic wrapper to create a network link.
+ *
+ * Returns 0 on success, < 0 on fatal error.
+ */
+int
+virNetlinkNewLink(const char *ifname,
+                  const char *type,
+                  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 };
+    VIR_AUTOPTR(virNetlinkMsg) nl_msg = NULL;
+    VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
+
+    *error = 0;
+
+    if (!ifname || !type) {
+        virReportError(VIR_ERR_INVALID_ARG, "%s",
+                       _("neither of name and type can be NULL"));
+        return -EINVAL;
+    }
+
+    nl_msg = nlmsg_alloc_simple(RTM_NEWLINK,
+                                NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL);
+    if (!nl_msg) {
+        virReportOOMError();
+        return -ENOMEM;
+    }
+
+    if (nlmsg_append(nl_msg,  &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0)
+        goto buffer_too_small;
+
+    if (ifname && nla_put_string(nl_msg, IFLA_IFNAME, ifname) < 0)
+        goto buffer_too_small;
+
+    if (!(linkinfo = nla_nest_start(nl_msg, IFLA_LINKINFO)))
+        goto buffer_too_small;
+
+    if (type && nla_put_string(nl_msg, IFLA_INFO_KIND, type) < 0)
+        goto buffer_too_small;
+
+    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);
+    }
+
+    nla_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;
+    }
+
+    if (virNetlinkCommand(nl_msg, &resp, &buflen, 0, 0, NETLINK_ROUTE, 0) < 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;
+    }
+
+    return 0;
+
+ malformed_resp:
+    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;
+}
+
+
 /**
  * virNetlinkDelLink:
  *
diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h
index 1e1e616..09bab08 100644
--- a/src/util/virnetlink.h
+++ b/src/util/virnetlink.h
@@ -65,6 +65,19 @@ int virNetlinkDumpCommand(struct nl_msg *nl_msg,
                           unsigned int protocol, unsigned int groups,
                           void *opaque);
 
+typedef struct _virNetlinkNewLinkData virNetlinkNewLinkData;
+typedef virNetlinkNewLinkData *virNetlinkNewLinkDataPtr;
+struct _virNetlinkNewLinkData {
+    const int *ifindex;             /* The index for the 'link' device */
+    const virMacAddr *mac;          /* The MAC address of the device */
+    const uint32_t *macvlan_mode;   /* The mode of macvlan */
+};
+
+int virNetlinkNewLink(const char *ifname,
+                      const char *type,
+                      virNetlinkNewLinkDataPtr data,
+                      int *error);
+
 typedef int (*virNetlinkDelLinkFallback)(const char *ifname);
 
 int virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback);
-- 
2.17.1


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Sep 07, 2018 at 03:17:24PM +0800, Shi Lei wrote:
> This patch introduces virNetlinkNewLink helper which wraps the common
> libnl/netlink code to create a new link.
>
> Signed-off-by: Shi Lei <shi_lei@massclouds.com>
> ---
>  src/libvirt_private.syms |   1 +
>  src/util/virnetlink.c    | 117 +++++++++++++++++++++++++++++++++++++++
>  src/util/virnetlink.h    |  13 +++++
>  3 files changed, 131 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 0fc5314..a0d229a 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2446,6 +2446,7 @@ virNetlinkEventServiceStop;
>  virNetlinkEventServiceStopAll;
>  virNetlinkGetErrorCode;
>  virNetlinkGetNeighbor;
> +virNetlinkNewLink;
>  virNetlinkShutdown;
>  virNetlinkStartup;
>
> diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
> index 8f06446..d53cc73 100644
> --- a/src/util/virnetlink.c
> +++ b/src/util/virnetlink.c
> @@ -488,6 +488,123 @@ virNetlinkDumpLink(const char *ifname, int ifindex,
>  }
>
>
> +/**
> + * virNetlinkNewLink:
> + *
> + * @ifname: name of the link
> + * @type: the type of the device, i.e. "bridge", "macvtap", "macvlan"
> + * @extra_args: the extra args for creating the netlink interface
> + * @error: netlink error code
> + *
> + * A generic wrapper to create a network link.
> + *
> + * Returns 0 on success, < 0 on fatal error.

-1 on error, no need for errno, we mostly use that only for system errors and
these will most likely (most often) be internal errors, additionally, you were
not consistent with the returns, sometimes you returned a genuine errno and
sometimes -1, it should either be errno at all times or -1.

> + */
> +int
> +virNetlinkNewLink(const char *ifname,
> +                  const char *type,
> +                  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 };
> +    VIR_AUTOPTR(virNetlinkMsg) nl_msg = NULL;
> +    VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
> +
> +    *error = 0;

maybe worth a VIR_DEBUG too.

> +
> +    if (!ifname || !type) {
> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
> +                       _("neither of name and type can be NULL"));

_("both interface name and type must be non-NULL");

OR

_("either iterface name or type is missing")

> +        return -EINVAL;

            return -1;

> +    }
> +
> +    nl_msg = nlmsg_alloc_simple(RTM_NEWLINK,
> +                                NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL);
> +    if (!nl_msg) {
> +        virReportOOMError();
> +        return -ENOMEM;
> +    }
> +
> +    if (nlmsg_append(nl_msg,  &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0)
> +        goto buffer_too_small;
> +
> +    if (ifname && nla_put_string(nl_msg, IFLA_IFNAME, ifname) < 0)

A tiny nitpick ^here...since we settled down on having NETLINK_MSG_PUT wrapper
macro only, then I think we should not use nla_put_string, even though we're
going to replace it in the next patch, simply for consistency, IOW the
replacement should be nla_put for NETLINK_MSG_PUT.

> +        goto buffer_too_small;
> +
> +    if (!(linkinfo = nla_nest_start(nl_msg, IFLA_LINKINFO)))
> +        goto buffer_too_small;
> +
> +    if (type && nla_put_string(nl_msg, IFLA_INFO_KIND, type) < 0)

same as above...

> +        goto buffer_too_small;
> +
> +    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)

here too...

> +            goto buffer_too_small;
> +
> +        nla_nest_end(nl_msg, infodata);
> +    }
> +
> +    nla_nest_end(nl_msg, linkinfo);
> +
> +    if (extra_args) {
> +        if (extra_args->ifindex &&
> +            nla_put_u32(nl_msg, IFLA_LINK, *extra_args->ifindex) < 0)

and here as well....

> +            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;
> +    }
> +
> +    if (virNetlinkCommand(nl_msg, &resp, &buflen, 0, 0, NETLINK_ROUTE, 0) < 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;
> +    }
> +
> +    return 0;
> +
> + malformed_resp:
> +    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                   _("malformed netlink response message"));
> +    return -EBADMSG;

return -1

> +
> + buffer_too_small:
> +    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                   _("allocated netlink buffer is too small"));
> +    return -ENOMEM;

return -1

> +}
> +
> +
>  /**
>   * virNetlinkDelLink:
>   *
> diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h
> index 1e1e616..09bab08 100644
> --- a/src/util/virnetlink.h
> +++ b/src/util/virnetlink.h
> @@ -65,6 +65,19 @@ int virNetlinkDumpCommand(struct nl_msg *nl_msg,
>                            unsigned int protocol, unsigned int groups,
>                            void *opaque);
>
> +typedef struct _virNetlinkNewLinkData virNetlinkNewLinkData;
> +typedef virNetlinkNewLinkData *virNetlinkNewLinkDataPtr;
> +struct _virNetlinkNewLinkData {
> +    const int *ifindex;             /* The index for the 'link' device */
> +    const virMacAddr *mac;          /* The MAC address of the device */
> +    const uint32_t *macvlan_mode;   /* The mode of macvlan */
> +};
> +
> +int virNetlinkNewLink(const char *ifname,
> +                      const char *type,
> +                      virNetlinkNewLinkDataPtr data,
> +                      int *error);

I'd suggest using also ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNUL(2);

> +
>  typedef int (*virNetlinkDelLinkFallback)(const char *ifname);
>
>  int virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback);

Would you agree with squashing the following in before merging? (this was btw
failing on mingw, so I added a symbol for that too)

Erik

diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
index bc377b5921..ed2db27799 100644
--- a/src/util/virnetdevbridge.c
+++ b/src/util/virnetdevbridge.c
@@ -416,78 +416,23 @@ int
 virNetDevBridgeCreate(const char *brname)
 {
     /* use a netlink RTM_NEWLINK message to create the bridge */
-    const char *type = "bridge";
-    struct nlmsgerr *err;
-    struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
-    unsigned int recvbuflen;
-    struct nlattr *linkinfo;
-    VIR_AUTOPTR(virNetlinkMsg) nl_msg = NULL;
-    VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
+    int error = 0;

-    nl_msg = nlmsg_alloc_simple(RTM_NEWLINK,
-                                NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL);
-    if (!nl_msg) {
-        virReportOOMError();
-        return -1;
-    }
-
-    if (nlmsg_append(nl_msg,  &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0)
-        goto buffer_too_small;
-    if (nla_put(nl_msg, IFLA_IFNAME, strlen(brname)+1, brname) < 0)
-        goto buffer_too_small;
-    if (!(linkinfo = nla_nest_start(nl_msg, IFLA_LINKINFO)))
-        goto buffer_too_small;
-    if (nla_put(nl_msg, IFLA_INFO_KIND, strlen(type), type) < 0)
-        goto buffer_too_small;
-    nla_nest_end(nl_msg, linkinfo);
-
-    if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0,
-                          NETLINK_ROUTE, 0) < 0) {
-        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 < 0) {
+    if (virNetlinkNewLink(brname, "bridge", NULL, &error) < 0) {
 # if defined(HAVE_STRUCT_IFREQ) && defined(SIOCBRADDBR)
-            if (err->error == -EOPNOTSUPP) {
-                /* fallback to ioctl if netlink doesn't support creating
-                 * bridges
-                 */
-                return virNetDevBridgeCreateWithIoctl(brname);
-            }
+        if (error == -EOPNOTSUPP) {
+            /* fallback to ioctl if netlink doesn't support creating bridges */
+            return virNetDevBridgeCreateWithIoctl(brname);
+        }
 # endif
-
-            virReportSystemError(-err->error,
-                                 _("error creating bridge interface %s"),
+        if (error < 0)
+            virReportSystemError(-error, _("error creating bridge interface %s"),
                                  brname);
-            return -1;
-        }
-        break;

-    case NLMSG_DONE:
-        break;
-    default:
-        goto malformed_resp;
+        return -1;
     }

     return 0;
-
- malformed_resp:
-    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/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index 2035b1f021..a66ab59952 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -307,113 +307,33 @@ virNetDevMacVLanCreate(const char *ifname,
                        uint32_t macvlan_mode,
                        int *retry)
 {
-    int rc = -1;
-    struct nlmsgerr *err;
-    struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
-    int ifindex;
-    unsigned int recvbuflen;
-    struct nl_msg *nl_msg;
-    struct nlattr *linkinfo, *info_data;
-    char macstr[VIR_MAC_STRING_BUFLEN];
-    VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
-
-    if (virNetDevGetIndex(srcdev, &ifindex) < 0)
-        return -1;
+    int error = 0;
+    int ifindex = 0;
+    virNetlinkNewLinkData data = {
+        .macvlan_mode = &macvlan_mode,
+        .mac = macaddress,
+    };

     *retry = 0;

-    nl_msg = nlmsg_alloc_simple(RTM_NEWLINK,
-                                NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL);
-    if (!nl_msg) {
-        virReportOOMError();
+    if (virNetDevGetIndex(srcdev, &ifindex) < 0)
         return -1;
-    }

-    if (nlmsg_append(nl_msg,  &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0)
-        goto buffer_too_small;
-
-    if (nla_put_u32(nl_msg, IFLA_LINK, ifindex) < 0)
-        goto buffer_too_small;
-
-    if (nla_put(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN, macaddress) < 0)
-        goto buffer_too_small;
-
-    if (ifname &&
-        nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0)
-        goto buffer_too_small;
-
-    if (!(linkinfo = nla_nest_start(nl_msg, IFLA_LINKINFO)))
-        goto buffer_too_small;
-
-    if (nla_put(nl_msg, IFLA_INFO_KIND, strlen(type), type) < 0)
-        goto buffer_too_small;
-
-    if (macvlan_mode > 0) {
-        if (!(info_data = nla_nest_start(nl_msg, IFLA_INFO_DATA)))
-            goto buffer_too_small;
-
-        if (nla_put(nl_msg, IFLA_MACVLAN_MODE, sizeof(macvlan_mode),
-                    &macvlan_mode) < 0)
-            goto buffer_too_small;
-
-        nla_nest_end(nl_msg, info_data);
-    }
-
-    nla_nest_end(nl_msg, linkinfo);
-
-    if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0,
-                          NETLINK_ROUTE, 0) < 0) {
-        goto cleanup;
-    }
-
-    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;
-
-        switch (err->error) {
-
-        case 0:
-            break;
-
-        case -EEXIST:
+    data.ifindex = &ifindex;
+    if (virNetlinkNewLink(ifname, type, &data, &error) < 0) {
+        char macstr[VIR_MAC_STRING_BUFLEN];
+        if (error == -EEXIST)
             *retry = 1;
-            goto cleanup;
-
-        default:
-            virReportSystemError(-err->error,
+        else if (error < 0)
+            virReportSystemError(-error,
                                  _("error creating %s interface %s@%s (%s)"),
                                  type, ifname, srcdev,
                                  virMacAddrFormat(macaddress, macstr));
-            goto cleanup;
-        }
-        break;

-    case NLMSG_DONE:
-        break;
-
-    default:
-        goto malformed_resp;
+        return -1;
     }

-    rc = 0;
- cleanup:
-    nlmsg_free(nl_msg);
-    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;
+    return 0;
 }

 /**

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 09/10/2018 10:38 AM, Erik Skultety wrote:
> On Fri, Sep 07, 2018 at 03:17:24PM +0800, Shi Lei wrote:
>> This patch introduces virNetlinkNewLink helper which wraps the common
>> libnl/netlink code to create a new link.
>>
>> Signed-off-by: Shi Lei <shi_lei@massclouds.com>
>> ---
>>  src/libvirt_private.syms |   1 +
>>  src/util/virnetlink.c    | 117 +++++++++++++++++++++++++++++++++++++++
>>  src/util/virnetlink.h    |  13 +++++
>>  3 files changed, 131 insertions(+)
>>

I was looking too while Erik posted this... I have many of the same
comments, but a couple more...

>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 0fc5314..a0d229a 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2446,6 +2446,7 @@ virNetlinkEventServiceStop;
>>  virNetlinkEventServiceStopAll;
>>  virNetlinkGetErrorCode;
>>  virNetlinkGetNeighbor;
>> +virNetlinkNewLink;
>>  virNetlinkShutdown;
>>  virNetlinkStartup;
>>
>> diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
>> index 8f06446..d53cc73 100644
>> --- a/src/util/virnetlink.c
>> +++ b/src/util/virnetlink.c
>> @@ -488,6 +488,123 @@ virNetlinkDumpLink(const char *ifname, int ifindex,
>>  }
>>
>>
>> +/**
>> + * virNetlinkNewLink:
>> + *
>> + * @ifname: name of the link
>> + * @type: the type of the device, i.e. "bridge", "macvtap", "macvlan"
>> + * @extra_args: the extra args for creating the netlink interface
>> + * @error: netlink error code
>> + *
>> + * A generic wrapper to create a network link.
>> + *
>> + * Returns 0 on success, < 0 on fatal error.
> 
> -1 on error, no need for errno, we mostly use that only for system errors and
> these will most likely (most often) be internal errors, additionally, you were
> not consistent with the returns, sometimes you returned a genuine errno and
> sometimes -1, it should either be errno at all times or -1.
> 

I would say:

 * Returns 0 on success, -1 on error. Additionally, if the @error is
 * non-zero, then the failure occurred during virNetlinkCommand, but
 * no error message generated leaving it up to the caller to handle
 * the condition.

>> + */
>> +int
>> +virNetlinkNewLink(const char *ifname,
>> +                  const char *type,
>> +                  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 };
>> +    VIR_AUTOPTR(virNetlinkMsg) nl_msg = NULL;
>> +    VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
>> +
>> +    *error = 0;
> 
> maybe worth a VIR_DEBUG too.
> 
>> +
>> +    if (!ifname || !type) {
>> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
>> +                       _("neither of name and type can be NULL"));
> 
> _("both interface name and type must be non-NULL");
> 
> OR
> 
> _("either iterface name or type is missing")
> 
>> +        return -EINVAL;
> 
>             return -1;
> 
>> +    }
>> +
>> +    nl_msg = nlmsg_alloc_simple(RTM_NEWLINK,
>> +                                NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL);
>> +    if (!nl_msg) {
>> +        virReportOOMError();
>> +        return -ENOMEM;
>> +    }
>> +
>> +    if (nlmsg_append(nl_msg,  &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0)
>> +        goto buffer_too_small;
>> +
>> +    if (ifname && nla_put_string(nl_msg, IFLA_IFNAME, ifname) < 0)
> 
> A tiny nitpick ^here...since we settled down on having NETLINK_MSG_PUT wrapper
> macro only, then I think we should not use nla_put_string, even though we're
> going to replace it in the next patch, simply for consistency, IOW the
> replacement should be nla_put for NETLINK_MSG_PUT.
> 
>> +        goto buffer_too_small;
>> +
>> +    if (!(linkinfo = nla_nest_start(nl_msg, IFLA_LINKINFO)))
>> +        goto buffer_too_small;
>> +
>> +    if (type && nla_put_string(nl_msg, IFLA_INFO_KIND, type) < 0)
> 
> same as above...
> 
>> +        goto buffer_too_small;
>> +
>> +    if ((STREQ(type, "macvtap") || STREQ(type, "macvlan")) &&
>> +         extra_args &&
>> +         extra_args->macvlan_mode &&
>> +         *extra_args->macvlan_mode > 0) {

Why is @macvlan_mode a "const uint32_t *", doesn't need to be does it?

>> +        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)
> 
> here too...
> 
>> +            goto buffer_too_small;
>> +
>> +        nla_nest_end(nl_msg, infodata);
>> +    }
>> +
>> +    nla_nest_end(nl_msg, linkinfo);
>> +
>> +    if (extra_args) {
>> +        if (extra_args->ifindex &&
>> +            nla_put_u32(nl_msg, IFLA_LINK, *extra_args->ifindex) < 0)
> 
> and here as well....
> 

Similarly @ifindex doesn't seem to need to be a "const int *"

>> +            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;
>> +    }
>> +
>> +    if (virNetlinkCommand(nl_msg, &resp, &buflen, 0, 0, NETLINK_ROUTE, 0) < 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;
>> +    }
>> +
>> +    return 0;
>> +
>> + malformed_resp:
>> +    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                   _("malformed netlink response message"));
>> +    return -EBADMSG;
> 
> return -1
> 
>> +
>> + buffer_too_small:
>> +    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                   _("allocated netlink buffer is too small"));
>> +    return -ENOMEM;
> 
> return -1
> 
>> +}
>> +
>> +
>>  /**
>>   * virNetlinkDelLink:
>>   *
>> diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h
>> index 1e1e616..09bab08 100644
>> --- a/src/util/virnetlink.h
>> +++ b/src/util/virnetlink.h
>> @@ -65,6 +65,19 @@ int virNetlinkDumpCommand(struct nl_msg *nl_msg,
>>                            unsigned int protocol, unsigned int groups,
>>                            void *opaque);
>>
>> +typedef struct _virNetlinkNewLinkData virNetlinkNewLinkData;
>> +typedef virNetlinkNewLinkData *virNetlinkNewLinkDataPtr;
>> +struct _virNetlinkNewLinkData {
>> +    const int *ifindex;             /* The index for the 'link' device */
>> +    const virMacAddr *mac;          /* The MAC address of the device */
>> +    const uint32_t *macvlan_mode;   /* The mode of macvlan */
>> +};
>> +
>> +int virNetlinkNewLink(const char *ifname,
>> +                      const char *type,
>> +                      virNetlinkNewLinkDataPtr data,
>> +                      int *error);
> 
> I'd suggest using also ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNUL(2);
> 

FWIW: Those ONLY help if NULL is passed, but do not help if say ifname
or type were NULL.  Furthermore, they cause a coverity error because the
values are being checked.

John

>> +
>>  typedef int (*virNetlinkDelLinkFallback)(const char *ifname);
>>
>>  int virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback);
> 
> Would you agree with squashing the following in before merging? (this was btw
> failing on mingw, so I added a symbol for that too)
> 
> Erik
> 
> diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
> index bc377b5921..ed2db27799 100644
> --- a/src/util/virnetdevbridge.c
> +++ b/src/util/virnetdevbridge.c
> @@ -416,78 +416,23 @@ int
>  virNetDevBridgeCreate(const char *brname)
>  {
>      /* use a netlink RTM_NEWLINK message to create the bridge */
> -    const char *type = "bridge";
> -    struct nlmsgerr *err;
> -    struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
> -    unsigned int recvbuflen;
> -    struct nlattr *linkinfo;
> -    VIR_AUTOPTR(virNetlinkMsg) nl_msg = NULL;
> -    VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
> +    int error = 0;
> 
> -    nl_msg = nlmsg_alloc_simple(RTM_NEWLINK,
> -                                NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL);
> -    if (!nl_msg) {
> -        virReportOOMError();
> -        return -1;
> -    }
> -
> -    if (nlmsg_append(nl_msg,  &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0)
> -        goto buffer_too_small;
> -    if (nla_put(nl_msg, IFLA_IFNAME, strlen(brname)+1, brname) < 0)
> -        goto buffer_too_small;
> -    if (!(linkinfo = nla_nest_start(nl_msg, IFLA_LINKINFO)))
> -        goto buffer_too_small;
> -    if (nla_put(nl_msg, IFLA_INFO_KIND, strlen(type), type) < 0)
> -        goto buffer_too_small;
> -    nla_nest_end(nl_msg, linkinfo);
> -
> -    if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0,
> -                          NETLINK_ROUTE, 0) < 0) {
> -        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 < 0) {
> +    if (virNetlinkNewLink(brname, "bridge", NULL, &error) < 0) {
>  # if defined(HAVE_STRUCT_IFREQ) && defined(SIOCBRADDBR)
> -            if (err->error == -EOPNOTSUPP) {
> -                /* fallback to ioctl if netlink doesn't support creating
> -                 * bridges
> -                 */
> -                return virNetDevBridgeCreateWithIoctl(brname);
> -            }
> +        if (error == -EOPNOTSUPP) {
> +            /* fallback to ioctl if netlink doesn't support creating bridges */
> +            return virNetDevBridgeCreateWithIoctl(brname);
> +        }
>  # endif
> -
> -            virReportSystemError(-err->error,
> -                                 _("error creating bridge interface %s"),
> +        if (error < 0)
> +            virReportSystemError(-error, _("error creating bridge interface %s"),
>                                   brname);
> -            return -1;
> -        }
> -        break;
> 
> -    case NLMSG_DONE:
> -        break;
> -    default:
> -        goto malformed_resp;
> +        return -1;
>      }
> 
>      return 0;
> -
> - malformed_resp:
> -    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/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
> index 2035b1f021..a66ab59952 100644
> --- a/src/util/virnetdevmacvlan.c
> +++ b/src/util/virnetdevmacvlan.c
> @@ -307,113 +307,33 @@ virNetDevMacVLanCreate(const char *ifname,
>                         uint32_t macvlan_mode,
>                         int *retry)
>  {
> -    int rc = -1;
> -    struct nlmsgerr *err;
> -    struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
> -    int ifindex;
> -    unsigned int recvbuflen;
> -    struct nl_msg *nl_msg;
> -    struct nlattr *linkinfo, *info_data;
> -    char macstr[VIR_MAC_STRING_BUFLEN];
> -    VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
> -
> -    if (virNetDevGetIndex(srcdev, &ifindex) < 0)
> -        return -1;
> +    int error = 0;
> +    int ifindex = 0;
> +    virNetlinkNewLinkData data = {
> +        .macvlan_mode = &macvlan_mode,
> +        .mac = macaddress,
> +    };
> 
>      *retry = 0;
> 
> -    nl_msg = nlmsg_alloc_simple(RTM_NEWLINK,
> -                                NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL);
> -    if (!nl_msg) {
> -        virReportOOMError();
> +    if (virNetDevGetIndex(srcdev, &ifindex) < 0)
>          return -1;
> -    }
> 
> -    if (nlmsg_append(nl_msg,  &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0)
> -        goto buffer_too_small;
> -
> -    if (nla_put_u32(nl_msg, IFLA_LINK, ifindex) < 0)
> -        goto buffer_too_small;
> -
> -    if (nla_put(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN, macaddress) < 0)
> -        goto buffer_too_small;
> -
> -    if (ifname &&
> -        nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0)
> -        goto buffer_too_small;
> -
> -    if (!(linkinfo = nla_nest_start(nl_msg, IFLA_LINKINFO)))
> -        goto buffer_too_small;
> -
> -    if (nla_put(nl_msg, IFLA_INFO_KIND, strlen(type), type) < 0)
> -        goto buffer_too_small;
> -
> -    if (macvlan_mode > 0) {
> -        if (!(info_data = nla_nest_start(nl_msg, IFLA_INFO_DATA)))
> -            goto buffer_too_small;
> -
> -        if (nla_put(nl_msg, IFLA_MACVLAN_MODE, sizeof(macvlan_mode),
> -                    &macvlan_mode) < 0)
> -            goto buffer_too_small;
> -
> -        nla_nest_end(nl_msg, info_data);
> -    }
> -
> -    nla_nest_end(nl_msg, linkinfo);
> -
> -    if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0,
> -                          NETLINK_ROUTE, 0) < 0) {
> -        goto cleanup;
> -    }
> -
> -    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;
> -
> -        switch (err->error) {
> -
> -        case 0:
> -            break;
> -
> -        case -EEXIST:
> +    data.ifindex = &ifindex;
> +    if (virNetlinkNewLink(ifname, type, &data, &error) < 0) {
> +        char macstr[VIR_MAC_STRING_BUFLEN];
> +        if (error == -EEXIST)
>              *retry = 1;
> -            goto cleanup;
> -
> -        default:
> -            virReportSystemError(-err->error,
> +        else if (error < 0)
> +            virReportSystemError(-error,
>                                   _("error creating %s interface %s@%s (%s)"),
>                                   type, ifname, srcdev,
>                                   virMacAddrFormat(macaddress, macstr));
> -            goto cleanup;
> -        }
> -        break;
> 
> -    case NLMSG_DONE:
> -        break;
> -
> -    default:
> -        goto malformed_resp;
> +        return -1;
>      }
> 
> -    rc = 0;
> - cleanup:
> -    nlmsg_free(nl_msg);
> -    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;
> +    return 0;
>  }
> 
>  /**
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Sep 10, 2018 at 11:03:04AM -0400, John Ferlan wrote:
>
>
> On 09/10/2018 10:38 AM, Erik Skultety wrote:
> > On Fri, Sep 07, 2018 at 03:17:24PM +0800, Shi Lei wrote:
> >> This patch introduces virNetlinkNewLink helper which wraps the common
> >> libnl/netlink code to create a new link.
> >>
> >> Signed-off-by: Shi Lei <shi_lei@massclouds.com>
> >> ---
> >>  src/libvirt_private.syms |   1 +
> >>  src/util/virnetlink.c    | 117 +++++++++++++++++++++++++++++++++++++++
> >>  src/util/virnetlink.h    |  13 +++++
> >>  3 files changed, 131 insertions(+)
> >>
>
> I was looking too while Erik posted this... I have many of the same
> comments, but a couple more...
>
> >> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> >> index 0fc5314..a0d229a 100644
> >> --- a/src/libvirt_private.syms
> >> +++ b/src/libvirt_private.syms
> >> @@ -2446,6 +2446,7 @@ virNetlinkEventServiceStop;
> >>  virNetlinkEventServiceStopAll;
> >>  virNetlinkGetErrorCode;
> >>  virNetlinkGetNeighbor;
> >> +virNetlinkNewLink;
> >>  virNetlinkShutdown;
> >>  virNetlinkStartup;
> >>
> >> diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
> >> index 8f06446..d53cc73 100644
> >> --- a/src/util/virnetlink.c
> >> +++ b/src/util/virnetlink.c
> >> @@ -488,6 +488,123 @@ virNetlinkDumpLink(const char *ifname, int ifindex,
> >>  }
> >>
> >>
> >> +/**
> >> + * virNetlinkNewLink:
> >> + *
> >> + * @ifname: name of the link
> >> + * @type: the type of the device, i.e. "bridge", "macvtap", "macvlan"
> >> + * @extra_args: the extra args for creating the netlink interface
> >> + * @error: netlink error code
> >> + *
> >> + * A generic wrapper to create a network link.
> >> + *
> >> + * Returns 0 on success, < 0 on fatal error.
> >
> > -1 on error, no need for errno, we mostly use that only for system errors and
> > these will most likely (most often) be internal errors, additionally, you were
> > not consistent with the returns, sometimes you returned a genuine errno and
> > sometimes -1, it should either be errno at all times or -1.
> >
>
> I would say:
>
>  * Returns 0 on success, -1 on error. Additionally, if the @error is
>  * non-zero, then the failure occurred during virNetlinkCommand, but
>  * no error message generated leaving it up to the caller to handle
>  * the condition.

"is generated" I guess?

Anyway, I agree.

>
> >> + */
> >> +int
> >> +virNetlinkNewLink(const char *ifname,
> >> +                  const char *type,
> >> +                  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 };
> >> +    VIR_AUTOPTR(virNetlinkMsg) nl_msg = NULL;
> >> +    VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
> >> +
> >> +    *error = 0;
> >
> > maybe worth a VIR_DEBUG too.
> >
> >> +
> >> +    if (!ifname || !type) {
> >> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
> >> +                       _("neither of name and type can be NULL"));
> >
> > _("both interface name and type must be non-NULL");
> >
> > OR
> >
> > _("either iterface name or type is missing")
> >
> >> +        return -EINVAL;
> >
> >             return -1;
> >
> >> +    }
> >> +
> >> +    nl_msg = nlmsg_alloc_simple(RTM_NEWLINK,
> >> +                                NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL);
> >> +    if (!nl_msg) {
> >> +        virReportOOMError();
> >> +        return -ENOMEM;
> >> +    }
> >> +
> >> +    if (nlmsg_append(nl_msg,  &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0)
> >> +        goto buffer_too_small;
> >> +
> >> +    if (ifname && nla_put_string(nl_msg, IFLA_IFNAME, ifname) < 0)
> >
> > A tiny nitpick ^here...since we settled down on having NETLINK_MSG_PUT wrapper
> > macro only, then I think we should not use nla_put_string, even though we're
> > going to replace it in the next patch, simply for consistency, IOW the
> > replacement should be nla_put for NETLINK_MSG_PUT.
> >
> >> +        goto buffer_too_small;
> >> +
> >> +    if (!(linkinfo = nla_nest_start(nl_msg, IFLA_LINKINFO)))
> >> +        goto buffer_too_small;
> >> +
> >> +    if (type && nla_put_string(nl_msg, IFLA_INFO_KIND, type) < 0)
> >
> > same as above...
> >
> >> +        goto buffer_too_small;
> >> +
> >> +    if ((STREQ(type, "macvtap") || STREQ(type, "macvlan")) &&
> >> +         extra_args &&
> >> +         extra_args->macvlan_mode &&
> >> +         *extra_args->macvlan_mode > 0) {
>
> Why is @macvlan_mode a "const uint32_t *", doesn't need to be does it?
>
> >> +        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)
> >
> > here too...
> >
> >> +            goto buffer_too_small;
> >> +
> >> +        nla_nest_end(nl_msg, infodata);
> >> +    }
> >> +
> >> +    nla_nest_end(nl_msg, linkinfo);
> >> +
> >> +    if (extra_args) {
> >> +        if (extra_args->ifindex &&
> >> +            nla_put_u32(nl_msg, IFLA_LINK, *extra_args->ifindex) < 0)
> >
> > and here as well....
> >
>
> Similarly @ifindex doesn't seem to need to be a "const int *"

Are you referring to the const correctness or the fact it's a pointer? If it's
the former, then I don't see a problem, if it's the latter, then I simply
wanted a deterministic way of telling that an argument is set, in case values
0, -1, etc. had some meaning.

>
> >> +            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;
> >> +    }
> >> +
> >> +    if (virNetlinkCommand(nl_msg, &resp, &buflen, 0, 0, NETLINK_ROUTE, 0) < 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;
> >> +    }
> >> +
> >> +    return 0;
> >> +
> >> + malformed_resp:
> >> +    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >> +                   _("malformed netlink response message"));
> >> +    return -EBADMSG;
> >
> > return -1
> >
> >> +
> >> + buffer_too_small:
> >> +    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >> +                   _("allocated netlink buffer is too small"));
> >> +    return -ENOMEM;
> >
> > return -1
> >
> >> +}
> >> +
> >> +
> >>  /**
> >>   * virNetlinkDelLink:
> >>   *
> >> diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h
> >> index 1e1e616..09bab08 100644
> >> --- a/src/util/virnetlink.h
> >> +++ b/src/util/virnetlink.h
> >> @@ -65,6 +65,19 @@ int virNetlinkDumpCommand(struct nl_msg *nl_msg,
> >>                            unsigned int protocol, unsigned int groups,
> >>                            void *opaque);
> >>
> >> +typedef struct _virNetlinkNewLinkData virNetlinkNewLinkData;
> >> +typedef virNetlinkNewLinkData *virNetlinkNewLinkDataPtr;
> >> +struct _virNetlinkNewLinkData {
> >> +    const int *ifindex;             /* The index for the 'link' device */
> >> +    const virMacAddr *mac;          /* The MAC address of the device */
> >> +    const uint32_t *macvlan_mode;   /* The mode of macvlan */
> >> +};
> >> +
> >> +int virNetlinkNewLink(const char *ifname,
> >> +                      const char *type,
> >> +                      virNetlinkNewLinkDataPtr data,
> >> +                      int *error);
> >
> > I'd suggest using also ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNUL(2);
> >
>
> FWIW: Those ONLY help if NULL is passed, but do not help if say ifname

Right, for everything else, we have the NULL check at the beginning.

> or type were NULL.  Furthermore, they cause a coverity error because the
> values are being checked.

Huh, I didn't know ^that, note taken, thanks.

Erik

>
> John
>
> >> +
> >>  typedef int (*virNetlinkDelLinkFallback)(const char *ifname);
> >>
> >>  int virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback);
> >
> > Would you agree with squashing the following in before merging? (this was btw
> > failing on mingw, so I added a symbol for that too)
> >
> > Erik
> >
> > diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
> > index bc377b5921..ed2db27799 100644
> > --- a/src/util/virnetdevbridge.c
> > +++ b/src/util/virnetdevbridge.c
> > @@ -416,78 +416,23 @@ int
> >  virNetDevBridgeCreate(const char *brname)
> >  {
> >      /* use a netlink RTM_NEWLINK message to create the bridge */
> > -    const char *type = "bridge";
> > -    struct nlmsgerr *err;
> > -    struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
> > -    unsigned int recvbuflen;
> > -    struct nlattr *linkinfo;
> > -    VIR_AUTOPTR(virNetlinkMsg) nl_msg = NULL;
> > -    VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
> > +    int error = 0;
> >
> > -    nl_msg = nlmsg_alloc_simple(RTM_NEWLINK,
> > -                                NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL);
> > -    if (!nl_msg) {
> > -        virReportOOMError();
> > -        return -1;
> > -    }
> > -
> > -    if (nlmsg_append(nl_msg,  &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0)
> > -        goto buffer_too_small;
> > -    if (nla_put(nl_msg, IFLA_IFNAME, strlen(brname)+1, brname) < 0)
> > -        goto buffer_too_small;
> > -    if (!(linkinfo = nla_nest_start(nl_msg, IFLA_LINKINFO)))
> > -        goto buffer_too_small;
> > -    if (nla_put(nl_msg, IFLA_INFO_KIND, strlen(type), type) < 0)
> > -        goto buffer_too_small;
> > -    nla_nest_end(nl_msg, linkinfo);
> > -
> > -    if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0,
> > -                          NETLINK_ROUTE, 0) < 0) {
> > -        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 < 0) {
> > +    if (virNetlinkNewLink(brname, "bridge", NULL, &error) < 0) {
> >  # if defined(HAVE_STRUCT_IFREQ) && defined(SIOCBRADDBR)
> > -            if (err->error == -EOPNOTSUPP) {
> > -                /* fallback to ioctl if netlink doesn't support creating
> > -                 * bridges
> > -                 */
> > -                return virNetDevBridgeCreateWithIoctl(brname);
> > -            }
> > +        if (error == -EOPNOTSUPP) {
> > +            /* fallback to ioctl if netlink doesn't support creating bridges */
> > +            return virNetDevBridgeCreateWithIoctl(brname);
> > +        }
> >  # endif
> > -
> > -            virReportSystemError(-err->error,
> > -                                 _("error creating bridge interface %s"),
> > +        if (error < 0)
> > +            virReportSystemError(-error, _("error creating bridge interface %s"),
> >                                   brname);
> > -            return -1;
> > -        }
> > -        break;
> >
> > -    case NLMSG_DONE:
> > -        break;
> > -    default:
> > -        goto malformed_resp;
> > +        return -1;
> >      }
> >
> >      return 0;
> > -
> > - malformed_resp:
> > -    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/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
> > index 2035b1f021..a66ab59952 100644
> > --- a/src/util/virnetdevmacvlan.c
> > +++ b/src/util/virnetdevmacvlan.c
> > @@ -307,113 +307,33 @@ virNetDevMacVLanCreate(const char *ifname,
> >                         uint32_t macvlan_mode,
> >                         int *retry)
> >  {
> > -    int rc = -1;
> > -    struct nlmsgerr *err;
> > -    struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
> > -    int ifindex;
> > -    unsigned int recvbuflen;
> > -    struct nl_msg *nl_msg;
> > -    struct nlattr *linkinfo, *info_data;
> > -    char macstr[VIR_MAC_STRING_BUFLEN];
> > -    VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
> > -
> > -    if (virNetDevGetIndex(srcdev, &ifindex) < 0)
> > -        return -1;
> > +    int error = 0;
> > +    int ifindex = 0;
> > +    virNetlinkNewLinkData data = {
> > +        .macvlan_mode = &macvlan_mode,
> > +        .mac = macaddress,
> > +    };
> >
> >      *retry = 0;
> >
> > -    nl_msg = nlmsg_alloc_simple(RTM_NEWLINK,
> > -                                NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL);
> > -    if (!nl_msg) {
> > -        virReportOOMError();
> > +    if (virNetDevGetIndex(srcdev, &ifindex) < 0)
> >          return -1;
> > -    }
> >
> > -    if (nlmsg_append(nl_msg,  &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0)
> > -        goto buffer_too_small;
> > -
> > -    if (nla_put_u32(nl_msg, IFLA_LINK, ifindex) < 0)
> > -        goto buffer_too_small;
> > -
> > -    if (nla_put(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN, macaddress) < 0)
> > -        goto buffer_too_small;
> > -
> > -    if (ifname &&
> > -        nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0)
> > -        goto buffer_too_small;
> > -
> > -    if (!(linkinfo = nla_nest_start(nl_msg, IFLA_LINKINFO)))
> > -        goto buffer_too_small;
> > -
> > -    if (nla_put(nl_msg, IFLA_INFO_KIND, strlen(type), type) < 0)
> > -        goto buffer_too_small;
> > -
> > -    if (macvlan_mode > 0) {
> > -        if (!(info_data = nla_nest_start(nl_msg, IFLA_INFO_DATA)))
> > -            goto buffer_too_small;
> > -
> > -        if (nla_put(nl_msg, IFLA_MACVLAN_MODE, sizeof(macvlan_mode),
> > -                    &macvlan_mode) < 0)
> > -            goto buffer_too_small;
> > -
> > -        nla_nest_end(nl_msg, info_data);
> > -    }
> > -
> > -    nla_nest_end(nl_msg, linkinfo);
> > -
> > -    if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0,
> > -                          NETLINK_ROUTE, 0) < 0) {
> > -        goto cleanup;
> > -    }
> > -
> > -    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;
> > -
> > -        switch (err->error) {
> > -
> > -        case 0:
> > -            break;
> > -
> > -        case -EEXIST:
> > +    data.ifindex = &ifindex;
> > +    if (virNetlinkNewLink(ifname, type, &data, &error) < 0) {
> > +        char macstr[VIR_MAC_STRING_BUFLEN];
> > +        if (error == -EEXIST)
> >              *retry = 1;
> > -            goto cleanup;
> > -
> > -        default:
> > -            virReportSystemError(-err->error,
> > +        else if (error < 0)
> > +            virReportSystemError(-error,
> >                                   _("error creating %s interface %s@%s (%s)"),
> >                                   type, ifname, srcdev,
> >                                   virMacAddrFormat(macaddress, macstr));
> > -            goto cleanup;
> > -        }
> > -        break;
> >
> > -    case NLMSG_DONE:
> > -        break;
> > -
> > -    default:
> > -        goto malformed_resp;
> > +        return -1;
> >      }
> >
> > -    rc = 0;
> > - cleanup:
> > -    nlmsg_free(nl_msg);
> > -    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;
> > +    return 0;
> >  }
> >
> >  /**
> >
> > --
> > libvir-list mailing list
> > libvir-list@redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
> >

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[...]

>>
>> I would say:
>>
>>  * Returns 0 on success, -1 on error. Additionally, if the @error is
>>  * non-zero, then the failure occurred during virNetlinkCommand, but
>>  * no error message generated leaving it up to the caller to handle
>>  * the condition.
> 
> "is generated" I guess?
> 
> Anyway, I agree.
> 

right... fingers don't always comply with mind ;-)


>>

[...]

>>>> +    if ((STREQ(type, "macvtap") || STREQ(type, "macvlan")) &&
>>>> +         extra_args &&
>>>> +         extra_args->macvlan_mode &&
>>>> +         *extra_args->macvlan_mode > 0) {
>>
>> Why is @macvlan_mode a "const uint32_t *", doesn't need to be does it?
>>
>>>> +        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)
>>>
>>> here too...
>>>
>>>> +            goto buffer_too_small;
>>>> +
>>>> +        nla_nest_end(nl_msg, infodata);
>>>> +    }
>>>> +
>>>> +    nla_nest_end(nl_msg, linkinfo);
>>>> +
>>>> +    if (extra_args) {
>>>> +        if (extra_args->ifindex &&
>>>> +            nla_put_u32(nl_msg, IFLA_LINK, *extra_args->ifindex) < 0)
>>>
>>> and here as well....
>>>
>>
>> Similarly @ifindex doesn't seem to need to be a "const int *"
> 
> Are you referring to the const correctness or the fact it's a pointer? If it's
> the former, then I don't see a problem, if it's the latter, then I simply
> wanted a deterministic way of telling that an argument is set, in case values
> 0, -1, etc. had some meaning.
> 

latter that it's a pointer.  The way it looks to me is that the value
could be changed in the function, but I understand the point of 0 (not
supplied) vs. NULL...

So fair enough reason to keep as const int *...

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Sep 10, 2018 at 01:17:43PM -0400, John Ferlan wrote:
> [...]
>
> >>
> >> I would say:
> >>
> >>  * Returns 0 on success, -1 on error. Additionally, if the @error is
> >>  * non-zero, then the failure occurred during virNetlinkCommand, but
> >>  * no error message generated leaving it up to the caller to handle
> >>  * the condition.
> >
> > "is generated" I guess?
> >
> > Anyway, I agree.
> >
>
> right... fingers don't always comply with mind ;-)
>
>
> >>
>
> [...]
>
> >>>> +    if ((STREQ(type, "macvtap") || STREQ(type, "macvlan")) &&
> >>>> +         extra_args &&
> >>>> +         extra_args->macvlan_mode &&
> >>>> +         *extra_args->macvlan_mode > 0) {
> >>
> >> Why is @macvlan_mode a "const uint32_t *", doesn't need to be does it?
> >>
> >>>> +        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)
> >>>
> >>> here too...
> >>>
> >>>> +            goto buffer_too_small;
> >>>> +
> >>>> +        nla_nest_end(nl_msg, infodata);
> >>>> +    }
> >>>> +
> >>>> +    nla_nest_end(nl_msg, linkinfo);
> >>>> +
> >>>> +    if (extra_args) {
> >>>> +        if (extra_args->ifindex &&
> >>>> +            nla_put_u32(nl_msg, IFLA_LINK, *extra_args->ifindex) < 0)
> >>>
> >>> and here as well....
> >>>
> >>
> >> Similarly @ifindex doesn't seem to need to be a "const int *"
> >
> > Are you referring to the const correctness or the fact it's a pointer? If it's
> > the former, then I don't see a problem, if it's the latter, then I simply
> > wanted a deterministic way of telling that an argument is set, in case values
> > 0, -1, etc. had some meaning.
> >
>
> latter that it's a pointer.  The way it looks to me is that the value
> could be changed in the function, but I understand the point of 0 (not
> supplied) vs. NULL...
>
> So fair enough reason to keep as const int *...

Okay, so do you have any other comments or shall I proceed with adding the
function commentary you suggested in your response + removing the
ATTRIBUTE_NONNULLs and merging the patch set with my proposed adjustment diffs?

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 09/11/2018 02:29 AM, Erik Skultety wrote:
> On Mon, Sep 10, 2018 at 01:17:43PM -0400, John Ferlan wrote:
>> [...]
>>
>>>>
>>>> I would say:
>>>>
>>>>  * Returns 0 on success, -1 on error. Additionally, if the @error is
>>>>  * non-zero, then the failure occurred during virNetlinkCommand, but
>>>>  * no error message generated leaving it up to the caller to handle
>>>>  * the condition.
>>>
>>> "is generated" I guess?
>>>
>>> Anyway, I agree.
>>>
>>
>> right... fingers don't always comply with mind ;-)
>>
>>
>>>>
>>
>> [...]
>>
>>>>>> +    if ((STREQ(type, "macvtap") || STREQ(type, "macvlan")) &&
>>>>>> +         extra_args &&
>>>>>> +         extra_args->macvlan_mode &&
>>>>>> +         *extra_args->macvlan_mode > 0) {
>>>>
>>>> Why is @macvlan_mode a "const uint32_t *", doesn't need to be does it?
>>>>
>>>>>> +        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)
>>>>>
>>>>> here too...
>>>>>
>>>>>> +            goto buffer_too_small;
>>>>>> +
>>>>>> +        nla_nest_end(nl_msg, infodata);
>>>>>> +    }
>>>>>> +
>>>>>> +    nla_nest_end(nl_msg, linkinfo);
>>>>>> +
>>>>>> +    if (extra_args) {
>>>>>> +        if (extra_args->ifindex &&
>>>>>> +            nla_put_u32(nl_msg, IFLA_LINK, *extra_args->ifindex) < 0)
>>>>>
>>>>> and here as well....
>>>>>
>>>>
>>>> Similarly @ifindex doesn't seem to need to be a "const int *"
>>>
>>> Are you referring to the const correctness or the fact it's a pointer? If it's
>>> the former, then I don't see a problem, if it's the latter, then I simply
>>> wanted a deterministic way of telling that an argument is set, in case values
>>> 0, -1, etc. had some meaning.
>>>
>>
>> latter that it's a pointer.  The way it looks to me is that the value
>> could be changed in the function, but I understand the point of 0 (not
>> supplied) vs. NULL...
>>
>> So fair enough reason to keep as const int *...
> 
> Okay, so do you have any other comments or shall I proceed with adding the
> function commentary you suggested in your response + removing the
> ATTRIBUTE_NONNULLs and merging the patch set with my proposed adjustment diffs?
> 

No other comments from me.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list