[libvirt] [PATCHv3 1/3] util: netlink: Introduce virNetlinkNewLink helper

Shi Lei shi_lei at massclouds.com
Mon Sep 10 17:06:58 UTC 2018


On 2018-09-10 at 22:38, 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 at 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. 

Okay.

>
>> + */
>> +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. 

Yes. 

>
>> +
>> +    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") 
Okay. But ^ might be interface   :-)

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

Yes.

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

Okay.

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

Okay.

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

Okay.

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

Okay.

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

Okay.

>
>> +}
>> +
>> +
>>  /**
>>   * 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); 

Okay.

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

Okay. I agree.

Thanks,

Shi Lei

>
>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;
> }
>
> /**
>




More information about the libvir-list mailing list