[libvirt PATCH] util: don't log error if SRIOV PF has no associated netdev

[Laine Stump]

Ahem.. Hello? Is this thing on?? :-)

On 3/3/21 3:34 PM, Laine Stump wrote:
> ping
> 
> On 2/23/21 10:35 PM, Laine Stump wrote:
>> Some SRIOV PFs don't have a netdev associated with them (the spec
>> apparently doesn't require it). In most cases when libvirt is dealing
>> with an SRIOV VF, that VF must have a PF, and the PF *must* have an
>> associated netdev (the only way to set the MAC address of a VF is by
>> sending a netlink message to the netdev of that VF's PF). But there
>> are times when we don't need for the PF to have a netdev; in
>> particular, when we're just getting the Switchdev Features for a VF,
>> we don't need the PF netdev - the netdev of the VF (apparently) works
>> just as well.
>>
>> Commit 6452e2f5 (libvirt 5.1.0) *kind of* made libvirt work around PFs
>> with no netdevs in this case - if virNetDevGetPhysicalFunction
>> returned an error when setting up to retrieve Switchdev feature info,
>> it would ignore the error, and then check if the PF netdev name was
>> NULL and, if so it would reset the error object and continue on rather
>> than returning early with a failure. The problem is that by the time
>> this special handling occured, the error message about missing netdev
>> had already been logged, which was harmless to proper operation, but
>> confused the user.
>>
>> Fortunately there are only 2 users of virNetDevGetPhysicalFunction, so
>> it is easy to redefine it's API to state that a missing netdev name is
>> *not* an error - in that case it will still return success, but the
>> caller must be prepared for the PF netdev name to be NULL. After
>> making this change, we can modify the two callers to behave properly
>> with the new semantics (for one of the callers it *is* still an error,
>> so the error message is moved there, but for the other it is okay to
>> continue), and our spurious error messages are a thing of the past.
>>
>> Resolves: https://bugzilla.redhat.com/1924616
>> Fixes: 6452e2f5e1837bd753ee465e6607ed3c4f62b815
>> Signed-off-by: Laine Stump <laine at redhat.com>
>> ---
>>   src/util/virnetdev.c | 31 +++++++++++++++----------------
>>   1 file changed, 15 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
>> index 2b4c8b6280..7b766234ec 100644
>> --- a/src/util/virnetdev.c
>> +++ b/src/util/virnetdev.c
>> @@ -1339,7 +1339,8 @@ virNetDevGetVirtualFunctionIndex(const char 
>> *pfname, const char *vfname,
>>    *
>>    * @ifname : name of the physical function interface name
>>    * @pfname : Contains sriov physical function for interface ifname
>> - *           upon successful return
>> + *           upon successful return (might be NULL if the PF has no
>> + *           associated netdev. This is *not* an error)
>>    *
>>    * Returns 0 on success, -1 on failure
>>    *
>> @@ -1361,15 +1362,6 @@ virNetDevGetPhysicalFunction(const char 
>> *ifname, char **pfname)
>>           return -1;
>>       }
>> -    if (!*pfname) {
>> -        /* The SRIOV standard does not require VF netdevs to have
>> -         * the netdev assigned to a PF. */
>> -        virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                       _("The PF device for VF %s has no network 
>> device name"),
>> -                       ifname);
>> -        return -1;
>> -    }
>> -
>>       return 0;
>>   }
>> @@ -1442,6 +1434,17 @@ virNetDevGetVirtualFunctionInfo(const char 
>> *vfname, char **pfname,
>>       if (virNetDevGetPhysicalFunction(vfname, pfname) < 0)
>>           return -1;
>> +    if (!*pfname) {
>> +        /* The SRIOV standard does not require VF netdevs to have the
>> +         * netdev assigned to a PF, but our method of retrieving
>> +         * VFINFO does.
>> +         */
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("The PF device for VF %s has no network 
>> device name, cannot get virtual function info"),
>> +                       vfname);
>> +        return -1;
>> +    }
>> +
>>       if (virNetDevGetVirtualFunctionIndex(*pfname, vfname, vf) < 0)
>>           goto cleanup;
>> @@ -3204,12 +3207,8 @@ virNetDevSwitchdevFeature(const char *ifname,
>>       if ((is_vf = virNetDevIsVirtualFunction(ifname)) < 0)
>>           return ret;
>> -    if (is_vf == 1) {
>> -        /* Ignore error if PF does not have netdev assigned.
>> -         * In that case pfname == NULL. */
>> -        if (virNetDevGetPhysicalFunction(ifname, &pfname) < 0)
>> -            virResetLastError();
>> -    }
>> +    if (is_vf == 1 && virNetDevGetPhysicalFunction(ifname, &pfname) < 0)
>> +        return ret;
>>       pci_device_ptr = pfname ? virNetDevGetPCIDevice(pfname) :
>>                                 virNetDevGetPCIDevice(ifname);
>>
>