[libvirt] [PATCH] nodedev: Fix failing to parse PCI address for non-PCI network devices

[Jim Fehlig]

On 01/03/2018 01:40 AM, Erik Skultety wrote:
> On Fri, Dec 22, 2017 at 01:05:26PM +0800, Fei Li wrote:
>> Commit 8708ca01c added virNetDevSwitchdevFeature to check whether
>> the NIC had Switchdev capabilities; however this causes errors for
>> network devices whose address is not in PCI format, like qeth device
>> whose address is 0.0.0800, when calling virPCIDeviceAddressParse.
>>
>> Change virReportError to VIR_DEBUG to avoid these two error messages
>> occuring when starting the libvirtd service, just leave their callers
>> to handle the return values respectively.
>>
>> Signed-off-by: Fei Li <fli at suse.com>
>> ---
>>   src/util/virpci.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/util/virpci.c b/src/util/virpci.c
>> index fe57bef32..880d7baba 100644
>> --- a/src/util/virpci.c
>> +++ b/src/util/virpci.c
>> @@ -2561,7 +2561,7 @@ logStrToLong_ui(char const *s,
>>
>>       ret = virStrToLong_ui(s, end_ptr, base, result);
>>       if (ret != 0)
>> -        VIR_ERROR(_("Failed to convert '%s' to unsigned int"), s);
>> +        VIR_DEBUG("Failed to convert '%s' to unsigned int", s);
>>       return ret;
>>   }
>>
>> @@ -2638,9 +2638,7 @@ virPCIGetDeviceAddressFromSysfsLink(const char *device_link)
>>           goto out;
>>
>>       if (virPCIDeviceAddressParse(config_address, bdf) < 0) {
>> -        virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                       _("Failed to parse PCI config address '%s'"),
>> -                       config_address);
>> +        VIR_DEBUG("Failed to parse PCI config address '%s'", config_address);
>>           VIR_FREE(bdf);
>>           goto out;
>>       }
> 
> I am not familiar with switchdev feature, but this is certainly not the correct
> fix, errors do have their meaning and by simply mangling the log level of
> messages to suit your use case is not the way.

I'll suggested this change when Fei and I discussed this bug off list. I'm far 
from an expert on this code as well, but I made the suggestions since some 
callers of virPCIGetDeviceAddressFromSysfsLink() report their own errors when it 
returns NULL and others treat a NULL return value as non-fatal. E.g. the 
virNetDevSwitchdevFeature()->virNetDevGetPCIDevice()->virPCIGetDeviceAddressFromSysfsLink() 
call chain.

In addition, virPCIGetDeviceAddressFromSysfsLink() already has

     if (!virFileExists(device_link)) {
         VIR_DEBUG("'%s' does not exist", device_link);
         return NULL;
     }

If changing error reporting to debug was acceptable, Fei's patch would need 
amended to change one more instance of virReportError().

> Again, I'm far from an expert
> in this area, but IMHO you should special case handling of devices supporting
> this feature by, e.g. using the VIR_NET_DEV_FEAT_SWITCHDEV enum or create
> another one if suitable/appropriate in this case and skip parsing the PCI
> address completely.

IIUC, we are trying to determine if the interface supports 
VIR_NET_DEV_FEAT_SWITCHDEV in virNetDevSwitchdevFeature(), and part of that is 
checking if the interface is backed by a PCI device. But like you, I'm not 
familiar with this code and may be missing something. Adding the author of the 
feature to cc for suggestions.

Regards,
Jim