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

[Laine Stump]

On 03/17/2017 09:32 AM, Michal Privoznik wrote:
> 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.

Yeah, sure. I just copy-pasta'd that from somewhere else. Consider it done.

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