[libvirt] [PATCH 18/19] util: if setting admin MAC to 00:00:00:00:00:00 fails, try 02:00:00:00:00:00

[Michal Privoznik]

On 03/10/2017 09:35 PM, Laine Stump wrote:
> Some PF drivers allow setting the admin MAC (that is the MAC address
> that the VF will be initialized to the next time the VF's driver is
> loaded) to 00:00:00:00:00:00, and some don't. Multiple drivers
> initialize the admin MACs to all 0, but don't allow setting it to that
> very same value. It has been an uphill battle convincing the driver
> people that it's reasonable to expect The argument that's used is
> that an all 0 device MAC address on a device is invalid; however, from
> an outsider's point of view, when the admin MAC is set to 0 at the
> time the VF driver is loaded, the VF's MAC is *not* set to 0, but to a
> random non-0 value. But that's beside the point - even if I could
> convince one or two SRIOV driver maintainers to permit setting the
> admin MAC to 0, there are still several other drivers.
>
> So rather than fighting that losing battle, this patch checks for a
> failure to set the admin MAC due to an all 0 value, and retries it
> with 02:00:00:00:00:00. That won't result in a random value being set
> in the VF MAC at next VF driver init, but that's okay, because we
> always want to set a specific value anyway. Rather, the "almost 0"
> setting makes it easy to visually detect from the output of "ip link
> show" which VFs are currently in use and which are free.
> ---
>  src/util/virnetdev.c | 42 ++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 2b1cebc..6cf0463 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -1387,6 +1387,14 @@ virNetDevSysfsFile(char **pf_sysfs_device_link ATTRIBUTE_UNUSED,
>  #if defined(__linux__) && defined(HAVE_LIBNL) && defined(IFLA_VF_MAX)
>
>
> +static virMacAddr zeroMAC = { 0 };
> +
> +/* if a net driver doesn't allow setting MAC to all 0, try setting
> + * to this (the only bit that is set is the "locally administered" bit")
> + */
> +static virMacAddr altZeroMAC = { .addr = { 0x02, 0, 0, 0, 0, 0, } };
> +
> +
>  static struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
>      [IFLA_VF_MAC]       = { .type = NLA_UNSPEC,
>                              .maxlen = sizeof(struct ifla_vf_mac) },
> @@ -1397,7 +1405,8 @@ static struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
>
>  static int
>  virNetDevSetVfConfig(const char *ifname, int vf,
> -                     const virMacAddr *macaddr, int vlanid)
> +                     const virMacAddr *macaddr, int vlanid,
> +                     bool *allowRetry)
>  {
>      int rc = -1;
>      struct nlmsghdr *resp = NULL;
> @@ -1474,7 +1483,15 @@ virNetDevSetVfConfig(const char *ifname, int vf,
>          if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
>              goto malformed_resp;
>
> -        if (err->error) {
> +        /* 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 &&

No, please no. if (err->error == -EINVAL ...) is way better.

> +            macaddr && !virMacAddrCmp(macaddr, &zeroMAC)) {
> +            goto cleanup;
> +        } else if (err->error) {
> +            /* other errors are permanent */
>              char macstr[VIR_MAC_STRING_BUFLEN];
>
>              virReportSystemError(-err->error,

ACK with that fixed.

Michal