[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Add virNetlinkNewLink for simplifying virNetDev*Create



On Thu, Aug 23, 2018 at 12:15:08PM +0800, Shi Lei wrote:
> This patch adds virNetlinkNewLink for simplifying virNetDevMacVLanCreate
> and virNetDevBridgeCreate.
>
> Signed-off-by: Shi Lei <shi_lei massclouds com>
> ---

There are multiple changes happening in this patch so it will need to be split
into several patches, see below...

>  src/libvirt_private.syms    |   1 +
>  src/util/virnetdevbridge.c  |  73 +++--------------------
>  src/util/virnetdevmacvlan.c | 137 ++++++++++++++------------------------------
>  src/util/virnetlink.c       | 104 +++++++++++++++++++++++++++++++++
>  src/util/virnetlink.h       |   8 +++
>  5 files changed, 164 insertions(+), 159 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 47ea35f..23931ba 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2439,6 +2439,7 @@ virNetlinkEventServiceStop;
>  virNetlinkEventServiceStopAll;
>  virNetlinkGetErrorCode;
>  virNetlinkGetNeighbor;
> +virNetlinkNewLink;
>  virNetlinkShutdown;
>  virNetlinkStartup;
>
> diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
> index bc377b5..1f5b37e 100644
> --- a/src/util/virnetdevbridge.c
> +++ b/src/util/virnetdevbridge.c
> @@ -417,77 +417,22 @@ 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(NULL, brname, NULL, type, NULL, 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);
> -            }
> -# endif
> -
> -            virReportSystemError(-err->error,
> -                                 _("error creating bridge interface %s"),
> -                                 brname);
> -            return -1;
> +        if (error == -EOPNOTSUPP) {
> +            /* fallback to ioctl if netlink doesn't support creating bridges */
> +            return virNetDevBridgeCreateWithIoctl(brname);
>          }
> -        break;
> +# endif
> +        virReportSystemError(-error, _("error creating bridge interface %s"),
> +                             brname);
>
> -    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 2035b1f..1629add 100644
> --- a/src/util/virnetdevmacvlan.c
> +++ b/src/util/virnetdevmacvlan.c
> @@ -283,6 +283,37 @@ virNetDevMacVLanReleaseName(const char *name)
>  }
>
>
> +static int
> +virNetDevMacVLanCreateCallback(struct nl_msg *nl_msg, const void *opaque)
> +{
> +    const uint32_t *mode = (const uint32_t *) opaque;
> +    if (!nl_msg || !opaque) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("nl_msg %p or opaque %p is NULL"),
> +                       nl_msg, opaque);
> +        return -1;
> +    }
> +
> +    if (*mode > 0) {
> +        struct nlattr *info_data;
> +        if (!(info_data = nla_nest_start(nl_msg, IFLA_INFO_DATA)))
> +            goto buffer_too_small;
> +
> +        if (nla_put(nl_msg, IFLA_MACVLAN_MODE, sizeof(*mode), mode) < 0)
> +            goto buffer_too_small;
> +
> +        nla_nest_end(nl_msg, info_data);
> +    }
> +
> +    return 0;
> +
> + buffer_too_small:
> +    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                   _("allocated netlink buffer is too small"));
> +    return -1;
> +}

Having a callback just to add a nested data into nl_msg doesn't feel like the
right approach, I'd expect a callback to do more specific stuff, this should be
special-cased in virNetlinkNewLink.

> +
> +
>  /**
>   * virNetDevMacVLanCreate:
>   *
> @@ -307,113 +338,29 @@ virNetDevMacVLanCreate(const char *ifname,
>                         uint32_t macvlan_mode,
>                         int *retry)
>  {

^This replacement would be the last patch in the series.

> -    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;
>      *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:
> +    if (virNetlinkNewLink(&ifindex, ifname, macaddress, type,
> +                          virNetDevMacVLanCreateCallback, &macvlan_mode,
> +                          &error) < 0) {
> +        char macstr[VIR_MAC_STRING_BUFLEN];
> +        if (error == -EEXIST)
>              *retry = 1;
> -            goto cleanup;
> -
> -        default:
> -            virReportSystemError(-err->error,
> +        else
> +            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;
>  }
>
>  /**
> diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
> index 8f06446..817e347 100644
> --- a/src/util/virnetlink.c
> +++ b/src/util/virnetlink.c
> @@ -489,6 +489,110 @@ virNetlinkDumpLink(const char *ifname, int ifindex,
>
>
>  /**
> + * virNetlinkNewLink:

Introduction of this function should come in a separate patch before you start
replacing the existing ones.

> + *
> + * @ifindex: The index for the 'link' device
> + * @ifname: The name of the link
> + * @mac: The MAC address of the device

Most of ^these attributes could be packed into a virNetlinkNewLinkData
structure (or something similarly named). You'll have to test check for
VIR_DOMAIN_NET_TYPE_DIRECT and VIR_NETDEV_MACVLAN_MODE_VEPA according to my
comments below inside <comment_block>...

> + * @type: The type of device, i.e., "bridge", "macvtap", "macvlan"
> + * @cb: The callback for filling in IFLA_INFO_DATA for this type

I don't think callback will be necessary

> + * @opaque: opaque for the callback
> + * @error: for retrieving error code

^see below for my comment on return value...

> + *
> + * 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.
> + *
> + * Returns 0 on success, -1 on fatal error.

Since this is a nice libvirt wrapper around the netlink communication
machinery, I think that returning -<netlink_error> in case of an error is
reasonable. Then, each of the callers of this API can handle the return values
as they please.

> + */
> +int
> +virNetlinkNewLink(const int *ifindex,
> +                  const char *ifname,
> +                  const virMacAddr *mac,
> +                  const char *type,
> +                  virNetlinkNewLinkCallback cb,
> +                  const void *opaque,
> +                  int *error)
> +{
> +    struct nlmsgerr *err;
> +    struct nlattr *linkinfo;
> +    unsigned int buflen;
> +    struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
> +    VIR_AUTOPTR(virNetlinkMsg) nl_msg = NULL;
> +    VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
> +
> +    *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;
> +

<comment_block>

> +    if (ifindex && nla_put_u32(nl_msg, IFLA_LINK, *ifindex) < 0)

pre-existing, but is there a particular reason why ^this has to be u32??

> +        goto buffer_too_small;
> +
> +    if (mac && nla_put(nl_msg, IFLA_ADDRESS, VIR_MAC_BUFLEN, mac) < 0)
> +        goto buffer_too_small;

Similarly, is there a reason why ^this is not necessary to be u32?

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

Since we're already touching the code, I would find it more readable if ^this
was written as linkinfo = nla_nest_start. The reason for that is that it's not
immediately visible where the matching counterpart to "nla_nest_end" is.

> +        goto buffer_too_small;
> +
> +    if (nla_put(nl_msg, IFLA_INFO_KIND, strlen(type), type) < 0)

I'm puzzled why ^this doesn't need strlen(foo)+1 instead of plain strlen(foo)

> +        goto buffer_too_small;

</comment_block>

We could actually create a wrapper macro around nla_put which would make the
block (begin-end) tiny more readable. See my notes above, as I have further
questions...Now, the macros would again be a separate patch to make the change
more gradual. Depending on the comments to my questions above, the macro would
either take the size of the type and always call nla_put or a specialized
nla_put_<string|u32|whatever> function. I'm CC'ing Laine to help answering my
questions inside <comment_block>.

Erik

> +
> +    if (cb && cb(nl_msg, opaque) < 0)
> +        return -1;

> +
> +    nla_nest_end(nl_msg, linkinfo);
> +
> +    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:
>   *
>   * @ifname:   Name of the link
> diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h
> index 1e1e616..195c7bb 100644
> --- a/src/util/virnetlink.h
> +++ b/src/util/virnetlink.h
> @@ -65,6 +65,14 @@ int virNetlinkDumpCommand(struct nl_msg *nl_msg,
>                            unsigned int protocol, unsigned int groups,
>                            void *opaque);
>
> +typedef int (*virNetlinkNewLinkCallback)(struct nl_msg *nl_msg,
> +                                         const void *opaque);
> +
> +int virNetlinkNewLink(const int *ifindex, const char *ifname,
> +                      const virMacAddr *macaddress, const char *type,
> +                      virNetlinkNewLinkCallback cb, const void *opaque,
> +                      int *error);
> +
>  typedef int (*virNetlinkDelLinkFallback)(const char *ifname);
>
>  int virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback);
> --
> 2.7.4
>
>
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]