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

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