Re: [libvirt PATCH v8 1/3] Set VF MAC and VLAN ID in two different operations

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

 



On 2/1/22 09:28, Dmitrii Shcherbakov wrote:
> This has a benefit of being able to handle error codes for those
> operations separately which is useful when drivers allow setting a MAC
> address but do not allow setting a VLAN (which is the case with some
> SmartNIC DPUs).
> 
> Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov@xxxxxxxxxxxxx>
> ---
>  src/libvirt_private.syms |   7 ++
>  src/util/virnetdev.c     | 226 ++++++++++++++++++++++++++------------
>  src/util/virnetdevpriv.h |  44 ++++++++
>  tests/virnetdevtest.c    | 230 ++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 434 insertions(+), 73 deletions(-)
>  create mode 100644 src/util/virnetdevpriv.h
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index bc6fa191bf..e6493c2176 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2847,6 +2847,13 @@ virNetDevOpenvswitchSetTimeout;
>  virNetDevOpenvswitchUpdateVlan;
>  
>  
> +# util/virnetdevpriv.h
> +virNetDevSendVfSetLinkRequest;
> +virNetDevSetVfConfig;
> +virNetDevSetVfMac;
> +virNetDevSetVfVlan;
> +
> +
>  # util/virnetdevtap.h
>  virNetDevTapAttachBridge;
>  virNetDevTapCreate;
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index d93b2c6a83..043225d31f 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -19,7 +19,9 @@
>  #include <config.h>
>  #include <math.h>
>  
> -#include "virnetdev.h"
> +#define LIBVIRT_VIRNETDEVPRIV_H_ALLOW
> +
> +#include "virnetdevpriv.h"
>  #include "viralloc.h"
>  #include "virnetlink.h"
>  #include "virmacaddr.h"
> @@ -1509,16 +1511,12 @@ static struct nla_policy ifla_vfstats_policy[IFLA_VF_STATS_MAX+1] = {
>      [IFLA_VF_STATS_MULTICAST]   = { .type = NLA_U64 },
>  };
>  
> -
> -static int
> -virNetDevSetVfConfig(const char *ifname, int vf,
> -                     const virMacAddr *macaddr, int vlanid,
> -                     bool *allowRetry)
> +int
> +virNetDevSendVfSetLinkRequest(const char *ifname, int vfInfoType,
> +                              const void *payload, const size_t payloadLen)

Here and everywhere else, please put one argument per line.

>  {
> -    int rc = -1;
> -    char macstr[VIR_MAC_STRING_BUFLEN];
>      g_autofree struct nlmsghdr *resp = NULL;
> -    struct nlmsgerr *err;
> +    struct nlmsgerr *err = NULL;
>      unsigned int recvbuflen = 0;
>      struct nl_msg *nl_msg;
>      struct nlattr *vfinfolist, *vfinfo;
> @@ -1526,9 +1524,11 @@ virNetDevSetVfConfig(const char *ifname, int vf,
>          .ifi_family = AF_UNSPEC,
>          .ifi_index  = -1,
>      };
> +    int rc = 0;

We usually initialize return values to -1 so that they don't have to be
overwritten in each error path.

>  
> -    if (!macaddr && vlanid < 0)
> +    if (payload == NULL || payloadLen == 0) {
>          return -1;
> +    }

This seems rather excessive. I know you just copied whatever was in the
original code, but neither of callers passes NULL/0.

>  
>      nl_msg = virNetlinkMsgNew(RTM_SETLINK, NLM_F_REQUEST);
>  
> @@ -1546,30 +1546,8 @@ virNetDevSetVfConfig(const char *ifname, int vf,
>      if (!(vfinfo = nla_nest_start(nl_msg, IFLA_VF_INFO)))
>          goto buffer_too_small;
>  
> -    if (macaddr) {
> -        struct ifla_vf_mac ifla_vf_mac = {
> -             .vf = vf,
> -             .mac = { 0, },
> -        };
> -
> -        virMacAddrGetRaw(macaddr, ifla_vf_mac.mac);
> -
> -        if (nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac),
> -                    &ifla_vf_mac) < 0)
> -            goto buffer_too_small;
> -    }
> -
> -    if (vlanid >= 0) {
> -        struct ifla_vf_vlan ifla_vf_vlan = {
> -             .vf = vf,
> -             .vlan = vlanid,
> -             .qos = 0,
> -        };
> -
> -        if (nla_put(nl_msg, IFLA_VF_VLAN, sizeof(ifla_vf_vlan),
> -                    &ifla_vf_vlan) < 0)
> -            goto buffer_too_small;
> -    }
> +    if (nla_put(nl_msg, vfInfoType, payloadLen, payload) < 0)
> +        goto buffer_too_small;
>  
>      nla_nest_end(nl_msg, vfinfo);
>      nla_nest_end(nl_msg, vfinfolist);
> @@ -1582,48 +1560,20 @@ virNetDevSetVfConfig(const char *ifname, int vf,
>          goto malformed_resp;
>  
>      switch (resp->nlmsg_type) {
> -    case NLMSG_ERROR:
> -        err = (struct nlmsgerr *)NLMSG_DATA(resp);
> -        if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
> +        case NLMSG_ERROR:
> +            err = (struct nlmsgerr *)NLMSG_DATA(resp);
> +            if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
> +                goto malformed_resp;
> +            rc = err->error;
> +            break;
> +        case NLMSG_DONE:
> +            rc = 0;
> +            break;
> +        default:
>              goto malformed_resp;

No need for formatting change.

> -
> -        /* if allowRetry is true and the error was EINVAL, then
> -         * silently return a failure so the caller can retry with a
> -         * different MAC address
> -         */
> -        if (err->error == -EINVAL && *allowRetry &&
> -            macaddr && !virMacAddrCmp(macaddr, &zeroMAC)) {
> -            goto cleanup;
> -        } else if (err->error) {
> -            /* other errors are permanent */
> -            virReportSystemError(-err->error,
> -                                 _("Cannot set interface MAC/vlanid to %s/%d "
> -                                   "for ifname %s vf %d"),
> -                                 (macaddr
> -                                  ? virMacAddrFormat(macaddr, macstr)
> -                                  : "(unchanged)"),
> -                                 vlanid,
> -                                 ifname ? ifname : "(unspecified)",
> -                                 vf);
> -            *allowRetry = false; /* no use retrying */
> -            goto cleanup;
> -        }
> -        break;
> -
> -    case NLMSG_DONE:
> -        break;
> -
> -    default:
> -        goto malformed_resp;
>      }
>  
> -    rc = 0;
>   cleanup:
> -    VIR_DEBUG("RTM_SETLINK %s vf %d MAC=%s vlanid=%d - %s",
> -              ifname, vf,
> -              macaddr ? virMacAddrFormat(macaddr, macstr) : "(unchanged)",
> -              vlanid, rc < 0 ? "Fail" : "Success");
> -
>      nlmsg_free(nl_msg);
>      return rc;
>  
> @@ -1638,6 +1588,100 @@ virNetDevSetVfConfig(const char *ifname, int vf,
>      goto cleanup;
>  }
>  
> +int
> +virNetDevSetVfVlan(const char *ifname, int vf, int vlanid)
> +{
> +    int rc = 0;
> +    int requestError = 0;
> +
> +    struct ifla_vf_vlan ifla_vf_vlan = {
> +         .vf = vf,
> +         .vlan = vlanid,
> +         .qos = 0,
> +    };

Alignment.

> +
> +    /* VLAN ids 0 and 4095 are reserved per 802.1Q but are valid values. */
> +    if ((vlanid < 0 || vlanid > 4095)) {
> +        return -ERANGE;

Here, we need to report an error because we do so in the other error
path. The rule of thumb is that either all or none error paths report an
error.

> +    }
> +
> +    requestError = virNetDevSendVfSetLinkRequest(ifname, IFLA_VF_VLAN,
> +                                                 &ifla_vf_vlan, sizeof(ifla_vf_vlan));
> +
> +    if (requestError) {
> +        virReportSystemError(-requestError,
> +                             _("Cannot set interface vlanid to %d "
> +                               "for ifname %s vf %d"),

Pre-existing, but error message are exempt from 80-chars long line rule
so that they can be easily 'git-grep'-ed. But since you're touching this
code it's worth fixing.

> +                             vlanid, ifname ? ifname : "(unspecified)", vf);
> +        rc = requestError;

So there's no difference between @rc and @requestError. They both have
the same value in the end.

> +    }
> +    VIR_DEBUG("RTM_SETLINK %s vf %d vlanid=%d - %s",
> +              ifname, vf,
> +              vlanid, rc < 0 ? "Fail" : "Success");
> +    return rc;
> +}
> +
> +int
> +virNetDevSetVfMac(const char *ifname, int vf,
> +                  const virMacAddr *macaddr,
> +                  bool *allowRetry)
> +{


Basically all comments from above apply here too.

> +    int rc = 0;
> +    int requestError = 0;
> +    char macstr[VIR_MAC_STRING_BUFLEN];
> +
> +    struct ifla_vf_mac ifla_vf_mac = {
> +         .vf = vf,
> +         .mac = { 0, },
> +    };
> +
> +    if (macaddr == NULL || allowRetry == NULL)
> +        return -EINVAL;
> +
> +    virMacAddrGetRaw(macaddr, ifla_vf_mac.mac);
> +
> +    requestError = virNetDevSendVfSetLinkRequest(ifname, IFLA_VF_MAC,
> +                                                 &ifla_vf_mac, sizeof(ifla_vf_mac));
> +    /* if allowRetry is true and the error was EINVAL, then
> +     * silently return a failure so the caller can retry with a
> +     * different MAC address. */
> +    if (requestError == -EINVAL && *allowRetry && !virMacAddrCmp(macaddr, &zeroMAC)) {
> +        rc = requestError;
> +    } else if (requestError) {
> +        /* other errors are permanent */
> +        virReportSystemError(-requestError,
> +                             _("Cannot set interface MAC to %s "
> +                               "for ifname %s vf %d"),
> +                             (macaddr
> +                              ? virMacAddrFormat(macaddr, macstr)
> +                              : "(unchanged)"),
> +                             ifname ? ifname : "(unspecified)",
> +                             vf);
> +        *allowRetry = false; /* no use retrying */
> +        rc = requestError;
> +    } else {
> +        rc = 0;
> +    }
> +    VIR_DEBUG("RTM_SETLINK %s vf %d MAC=%s - %s",
> +              ifname, vf,
> +              macaddr ? virMacAddrFormat(macaddr, macstr) : "(unchanged)",
> +              rc < 0 ? "Fail" : "Success");
> +    return rc;
> +}
> +
> +int
> +virNetDevSetVfConfig(const char *ifname, int vf,
> +                     const virMacAddr *macaddr, int vlanid,
> +                     bool *allowRetry)
> +{
> +    int rc = 0;
> +    if ((rc = virNetDevSetVfMac(ifname, vf, macaddr, allowRetry)) < 0)
> +        return rc;
> +    if ((rc = virNetDevSetVfVlan(ifname, vf, vlanid)) < 0)
> +        return rc;
> +    return rc;
> +}
> +
>  /**
>   * virNetDevParseVfInfo:
>   * Get the VF interface information from kernel by netlink, To make netlink
> @@ -2391,6 +2435,44 @@ virNetDevVFInterfaceStats(virPCIDeviceAddress *vfAddr G_GNUC_UNUSED,
>      return -1;
>  }
>  
> +int
> +virNetDevSendVfSetLinkRequest(const char *ifname G_GNUC_UNUSED, int vfInfoType G_GNUC_UNUSED,
> +                              const void *payload G_GNUC_UNUSED,
> +                              const size_t payloadLen G_GNUC_UNUSED)
> +{
> +    virReportSystemError(ENOSYS, "%s",
> +                         _("Unable to send a VF SETLINK request on this platform"));
> +    return -1;

I'm inclined to return -ENOSYS rather than -1 because the real
implementation would return a real -errno. I know it doesn't matter
really because neither of callers looks at errno, they merely check for
<0 but consistency is paramount. Thus if one variant of function returns
-errno then the other should too.

> +}
> +
> +int
> +virNetDevSetVfVlan(const char *ifname G_GNUC_UNUSED, int vf G_GNUC_UNUSED, int vlanid G_GNUC_UNUSED)
> +{
> +    virReportSystemError(ENOSYS, "%s",
> +                         _("Unable to set a VF VLAN on this platform"));
> +    return -1;
> +}
> +
> +int
> +virNetDevSetVfMac(const char *ifname G_GNUC_UNUSED, int vf G_GNUC_UNUSED,
> +                  const virMacAddr *macaddr G_GNUC_UNUSED,
> +                  bool *allowRetry G_GNUC_UNUSED)
> +{
> +    virReportSystemError(ENOSYS, "%s",
> +                         _("Unable to set a VF MAC on this platform"));
> +    return -1;
> +}
> +
> +int
> +virNetDevSetVfConfig(const char *ifname G_GNUC_UNUSED, int vf G_GNUC_UNUSED,
> +                     const virMacAddr *macaddr G_GNUC_UNUSED, int vlanid G_GNUC_UNUSED,
> +                     bool *allowRetry G_GNUC_UNUSED)
> +{
> +    virReportSystemError(ENOSYS, "%s",
> +                         _("Unable to set a VF config on this platform"));
> +    return -1;
> +}
> +
>  
>  #endif /* defined(WITH_LIBNL) */
>  
> diff --git a/src/util/virnetdevpriv.h b/src/util/virnetdevpriv.h
> new file mode 100644
> index 0000000000..7990bf5269
> --- /dev/null
> +++ b/src/util/virnetdevpriv.h
> @@ -0,0 +1,44 @@
> +/*
> + * virnetdevpriv.h: private virnetdev header for unit testing
> + *
> + * Copyright (C) 2021 Canonical Ltd.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library;  If not, see
> + * <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef LIBVIRT_VIRNETDEVPRIV_H_ALLOW
> +# error "virnetdevpriv.h may only be included by virnetdev.c or test suites"
> +#endif /* LIBVIRT_VIRNETDEVPRIV_H_ALLOW */
> +
> +#pragma once
> +
> +#include "virnetdev.h"
> +
> +int
> +virNetDevSendVfSetLinkRequest(const char *ifname, int vfInfoType,
> +                              const void *payload, const size_t payloadLen);
> +
> +int
> +virNetDevSetVfVlan(const char *ifname, int vf, int vlanid);
> +
> +int
> +virNetDevSetVfMac(const char *ifname, int vf,
> +                  const virMacAddr *macaddr,
> +                  bool *allowRetry);
> +
> +int
> +virNetDevSetVfConfig(const char *ifname, int vf,
> +                     const virMacAddr *macaddr, int vlanid,
> +                     bool *allowRetry);
> diff --git a/tests/virnetdevtest.c b/tests/virnetdevtest.c
> index aadbeb1ef4..c8dba05c31 100644
> --- a/tests/virnetdevtest.c
> +++ b/tests/virnetdevtest.c

Bonus points for extensive tests.

Michal




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux