[libvirt] [PATCHv2 1/4] Introduce virNetlinkNewLink

[Erik Skultety]

On Wed, Sep 05, 2018 at 04:36:27PM +0800, Shi Lei wrote:

the subject should read:

util: netlink: Introduce virNetlinkNewLink helper

> This patch introduces virNetlinkNewLink which wraps newlink code
> using libnl.

... 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    | 120 +++++++++++++++++++++++++++++++++++++--
>  src/util/virnetlink.h    |  13 +++++
>  3 files changed, 129 insertions(+), 5 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index e2340d2..77c9b9e 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2447,6 +2447,7 @@ virNetlinkEventServiceStop;
>  virNetlinkEventServiceStopAll;
>  virNetlinkGetErrorCode;
>  virNetlinkGetNeighbor;
> +virNetlinkNewLink;
>  virNetlinkShutdown;
>  virNetlinkStartup;
>
> diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
> index 8f06446..32aa62d 100644
> --- a/src/util/virnetlink.c
> +++ b/src/util/virnetlink.c
> @@ -420,10 +420,8 @@ virNetlinkDumpLink(const char *ifname, int ifindex,
>      if (nlmsg_append(nl_msg,  &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0)
>          goto buffer_too_small;
>
> -    if (ifname) {
> -        if (nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0)
> -            goto buffer_too_small;
> -    }
> +    if (ifname && nla_put_string(nl_msg, IFLA_IFNAME, ifname) < 0)
> +        goto buffer_too_small;

unrelated to this patch...this kind of change should be a separate patch
somewhere at the beginning of the series...

>
>  # ifdef RTEXT_FILTER_VF
>      /* if this filter exists in the kernel's netlink implementation,
> @@ -488,6 +486,118 @@ virNetlinkDumpLink(const char *ifname, int ifindex,
>  }
>
>
> +/**
> + * virNetlinkNewLink:
> + *
> + * @ifname: Name of the link
> + * @type: The type of device, i.e., "bridge", "macvtap", "macvlan"

s/Name/name
s/The/the
s/of device/of the device/
s/i.e.,/i.e.

> + * @ifindex: The index for the 'link' device

I think ^this one should be part of the extra args, e.g. bridge doesn't use it

> + * @data: The extra args for creating the netlink interface

actually extra_args is a better name for the argument :)

> + * @error: for retrieving error code

s/for retrieving error code/netlink error code

> + *
> + * Create a network "link" (aka interface aka device) with the given
> + * args. This works for many different types of network devices,
> + * including macvtap and bridges.

A generic wrapper to create a network link.

> + *
> + * Returns 0 on success, -1 on fatal error.
> + */
> +int
> +virNetlinkNewLink(const char *ifname,
> +                  const char *type,
> +                  const int *ifindex,
> +                  virNetlinkNewLinkDataPtr data,
> +                  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 (!data)
> +        return -1;

Extra args should not be required, BridgeCreate doesn't make use of them. That
said, I think it's reasonable to make @ifname and @type mandatory and fail
whenever we don't get either of them:

if (!ifname && !type)
    virReportError(...);

> +
> +    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 (ifindex && nla_put_u32(nl_msg, IFLA_LINK, *ifindex) < 0)
> +        goto buffer_too_small;
> +
> +    if (data->mac && nla_put(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN, data->mac) < 0)
> +        goto buffer_too_small;

The order of the data in the msg payload doesn't matter, right? If that's true,
we could move everything related to @extra_args and *not* related to nested
containers right before actually calling into virNetlinkCommand. If the order
does actually matter, then you can disregard this comment.

> +
> +    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")) &&
> +        data->macvlan_mode && *data->macvlan_mode > 0) {

this check would get a bit uglier though:
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, *data->macvlan_mode) < 0)
> +            goto buffer_too_small;
> +
> +        nla_nest_end(nl_msg, infodata);
> +    }
> +
> +    nla_nest_end(nl_msg, linkinfo);

here you'd have:

if (extra_args) {
...
}

> +
> +    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 -1;
> +
> + buffer_too_small:
> +    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                   _("allocated netlink buffer is too small"));
> +    return -1;
> +}
> +
> +
>  /**
>   * virNetlinkDelLink:
>   *
> @@ -522,7 +632,7 @@ virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback)
>      if (nlmsg_append(nl_msg,  &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0)
>          goto buffer_too_small;
>
> -    if (nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0)
> +    if (ifname && nla_put_string(nl_msg, IFLA_IFNAME, ifname) < 0)
>          goto buffer_too_small;

...unrelated to this patch...

>
>      if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0,
> diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h
> index 1e1e616..8163a81 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 virMacAddr *mac;          /* The MAC address of the device */
> +    const uint32_t *macvlan_mode;   /* The mode of macvlan */

this would contain const int *ifindex too...

> +};
> +
> +int virNetlinkNewLink(const char *ifname,
> +                      const char *type,
> +                      const int *ifindex,
> +                      virNetlinkNewLinkDataPtr data,
> +                      int *error);
> +
>  typedef int (*virNetlinkDelLinkFallback)(const char *ifname);
>
>  int virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback);

The patch looks good, except for the few nitpicks I mentioned.

Erik