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

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

Posted by Jim Fehlig 6 years, 3 months ago

Commit 8708ca01c added virNetDevSwitchdevFeature() to check if a network
device has Switchdev capabilities. virNetDevSwitchdevFeature() attempts
to retrieve the PCI device associated with the network device, ignoring
non-PCI devices. It does so via the following call chain

  virNetDevSwitchdevFeature()->virNetDevGetPCIDevice()->
  virPCIGetDeviceAddressFromSysfsLink()

For non-PCI network devices (qeth, Xen vif, etc),
virPCIGetDeviceAddressFromSysfsLink() will report an error when
virPCIDeviceAddressParse() fails. virPCIDeviceAddressParse() also
logs an error. After commit 8708ca01c there are now two errors reported
for each non-PCI network device even though the errors are harmless.

To avoid the errors, introduce virNetDevIsPCIDevice() and use it in
virNetDevGetPCIDevice() before attempting to retrieve the associated
PCI device. virNetDevIsPCIDevice() uses the 'subsystem' property of the
device to determine if it is PCI. See the sysfs rules in kernel
documentation for more details

https://www.kernel.org/doc/html/latest/admin-guide/sysfs-rules.html
---

V2: https://www.redhat.com/archives/libvir-list/2018-January/msg00233.html

Changes in V3:
Implement checking if netdev is PCI in virnetdev.c instead of virpci.c

Addressed other comments from eskultet

 src/util/virnetdev.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index eb2d119bf..baf4a71fe 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -22,6 +22,7 @@
 
 #include <config.h>
 
+#include "dirname.h"
 #include "virnetdev.h"
 #include "virnetlink.h"
 #include "virmacaddr.h"
@@ -1147,6 +1148,48 @@ virNetDevSysfsDeviceFile(char **pf_sysfs_device_link, const char *ifname,
     return 0;
 }
 
+/**
+ * Determine if the device path specified in devpath is a PCI Device
+ * by resolving the 'subsystem'-link in devpath and looking for
+ * 'pci' in the last component. For more information see the rules
+ * for accessing sysfs in the kernel docs
+ *
+ * https://www.kernel.org/doc/html/latest/admin-guide/sysfs-rules.html
+ *
+ * Returns true if devpath's susbsystem is pci, false otherwise.
+ */
+static bool
+virNetDevIsPCIDevice(const char *devpath)
+{
+    char *subsys_link = NULL;
+    char *abs_path = NULL;
+    char *subsys = NULL;
+    bool ret = false;
+
+    if (virAsprintf(&subsys_link, "%s/subsystem", devpath) < 0)
+        return false;
+
+    if (!virFileExists(subsys_link))
+        goto cleanup;
+
+    if (virFileIsLink(subsys_link) != 1)
+        goto cleanup;
+
+    if (virFileResolveLink(subsys_link, &abs_path) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Unable to resolve device subsystem symlink %s"),
+                       subsys_link);
+        goto cleanup;
+    }
+
+    subsys = last_component(abs_path);
+    ret = STRPREFIX(subsys, "pci");
+
+ cleanup:
+    VIR_FREE(subsys_link);
+    VIR_FREE(abs_path);
+    return ret;
+}
 
 static virPCIDevicePtr
 virNetDevGetPCIDevice(const char *devName)
@@ -1158,6 +1201,9 @@ virNetDevGetPCIDevice(const char *devName)
     if (virNetDevSysfsFile(&vfSysfsDevicePath, devName, "device") < 0)
         goto cleanup;
 
+    if (!virNetDevIsPCIDevice(vfSysfsDevicePath))
+        goto cleanup;
+
     vfPCIAddr = virPCIGetDeviceAddressFromSysfsLink(vfSysfsDevicePath);
     if (!vfPCIAddr)
         goto cleanup;
-- 
2.15.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Erik Skultety 6 years, 3 months ago

On Wed, Jan 17, 2018 at 10:46:32AM -0700, Jim Fehlig wrote:
> Commit 8708ca01c added virNetDevSwitchdevFeature() to check if a network
> device has Switchdev capabilities. virNetDevSwitchdevFeature() attempts
> to retrieve the PCI device associated with the network device, ignoring
> non-PCI devices. It does so via the following call chain
>
>   virNetDevSwitchdevFeature()->virNetDevGetPCIDevice()->
>   virPCIGetDeviceAddressFromSysfsLink()
>
> For non-PCI network devices (qeth, Xen vif, etc),
> virPCIGetDeviceAddressFromSysfsLink() will report an error when
> virPCIDeviceAddressParse() fails. virPCIDeviceAddressParse() also
> logs an error. After commit 8708ca01c there are now two errors reported
> for each non-PCI network device even though the errors are harmless.
>
> To avoid the errors, introduce virNetDevIsPCIDevice() and use it in
> virNetDevGetPCIDevice() before attempting to retrieve the associated
> PCI device. virNetDevIsPCIDevice() uses the 'subsystem' property of the
> device to determine if it is PCI. See the sysfs rules in kernel
> documentation for more details
>
> https://www.kernel.org/doc/html/latest/admin-guide/sysfs-rules.html
> ---
>
> V2: https://www.redhat.com/archives/libvir-list/2018-January/msg00233.html
>
> Changes in V3:
> Implement checking if netdev is PCI in virnetdev.c instead of virpci.c
>
> Addressed other comments from eskultet
>
>  src/util/virnetdev.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index eb2d119bf..baf4a71fe 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -22,6 +22,7 @@
>
>  #include <config.h>
>
> +#include "dirname.h"
>  #include "virnetdev.h"
>  #include "virnetlink.h"
>  #include "virmacaddr.h"
> @@ -1147,6 +1148,48 @@ virNetDevSysfsDeviceFile(char **pf_sysfs_device_link, const char *ifname,
>      return 0;
>  }
>
> +/**
> + * Determine if the device path specified in devpath is a PCI Device
> + * by resolving the 'subsystem'-link in devpath and looking for
> + * 'pci' in the last component. For more information see the rules
> + * for accessing sysfs in the kernel docs
> + *
> + * https://www.kernel.org/doc/html/latest/admin-guide/sysfs-rules.html
> + *
> + * Returns true if devpath's susbsystem is pci, false otherwise.
> + */
> +static bool
> +virNetDevIsPCIDevice(const char *devpath)
> +{
> +    char *subsys_link = NULL;
> +    char *abs_path = NULL;
> +    char *subsys = NULL;
> +    bool ret = false;
> +
> +    if (virAsprintf(&subsys_link, "%s/subsystem", devpath) < 0)
> +        return false;
> +
> +    if (!virFileExists(subsys_link))
> +        goto cleanup;
> +
> +    if (virFileIsLink(subsys_link) != 1)
> +        goto cleanup;
> +

You don't really need ^this check, do you? Once the path exists,
virFileResolveLink is going to handle it gracefully.

Reviewed-by: Erik Skultety <eskultet@redhat.com> (but I'd wait with pushing
until after the release)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Jim Fehlig 6 years, 3 months ago

On 01/18/2018 02:21 AM, Erik Skultety wrote:
> On Wed, Jan 17, 2018 at 10:46:32AM -0700, Jim Fehlig wrote:
>> Commit 8708ca01c added virNetDevSwitchdevFeature() to check if a network
>> device has Switchdev capabilities. virNetDevSwitchdevFeature() attempts
>> to retrieve the PCI device associated with the network device, ignoring
>> non-PCI devices. It does so via the following call chain
>>
>>    virNetDevSwitchdevFeature()->virNetDevGetPCIDevice()->
>>    virPCIGetDeviceAddressFromSysfsLink()
>>
>> For non-PCI network devices (qeth, Xen vif, etc),
>> virPCIGetDeviceAddressFromSysfsLink() will report an error when
>> virPCIDeviceAddressParse() fails. virPCIDeviceAddressParse() also
>> logs an error. After commit 8708ca01c there are now two errors reported
>> for each non-PCI network device even though the errors are harmless.
>>
>> To avoid the errors, introduce virNetDevIsPCIDevice() and use it in
>> virNetDevGetPCIDevice() before attempting to retrieve the associated
>> PCI device. virNetDevIsPCIDevice() uses the 'subsystem' property of the
>> device to determine if it is PCI. See the sysfs rules in kernel
>> documentation for more details
>>
>> https://www.kernel.org/doc/html/latest/admin-guide/sysfs-rules.html
>> ---
>>
>> V2: https://www.redhat.com/archives/libvir-list/2018-January/msg00233.html
>>
>> Changes in V3:
>> Implement checking if netdev is PCI in virnetdev.c instead of virpci.c
>>
>> Addressed other comments from eskultet
>>
>>   src/util/virnetdev.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 46 insertions(+)
>>
>> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
>> index eb2d119bf..baf4a71fe 100644
>> --- a/src/util/virnetdev.c
>> +++ b/src/util/virnetdev.c
>> @@ -22,6 +22,7 @@
>>
>>   #include <config.h>
>>
>> +#include "dirname.h"
>>   #include "virnetdev.h"
>>   #include "virnetlink.h"
>>   #include "virmacaddr.h"
>> @@ -1147,6 +1148,48 @@ virNetDevSysfsDeviceFile(char **pf_sysfs_device_link, const char *ifname,
>>       return 0;
>>   }
>>
>> +/**
>> + * Determine if the device path specified in devpath is a PCI Device
>> + * by resolving the 'subsystem'-link in devpath and looking for
>> + * 'pci' in the last component. For more information see the rules
>> + * for accessing sysfs in the kernel docs
>> + *
>> + * https://www.kernel.org/doc/html/latest/admin-guide/sysfs-rules.html
>> + *
>> + * Returns true if devpath's susbsystem is pci, false otherwise.
>> + */
>> +static bool
>> +virNetDevIsPCIDevice(const char *devpath)
>> +{
>> +    char *subsys_link = NULL;
>> +    char *abs_path = NULL;
>> +    char *subsys = NULL;
>> +    bool ret = false;
>> +
>> +    if (virAsprintf(&subsys_link, "%s/subsystem", devpath) < 0)
>> +        return false;
>> +
>> +    if (!virFileExists(subsys_link))
>> +        goto cleanup;
>> +
>> +    if (virFileIsLink(subsys_link) != 1)
>> +        goto cleanup;
>> +
> 
> You don't really need ^this check, do you? Once the path exists,
> virFileResolveLink is going to handle it gracefully.

I removed the check.

> 
> Reviewed-by: Erik Skultety <eskultet@redhat.com> (but I'd wait with pushing
> until after the release)

Pushed now that the release is out. And agreed waiting until after the release 
was best since there are other callers of virNetDevGetPCIDevice() that could be 
affected - although I did check those and they seem fine.

Regards,
Jim

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list