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

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(a)redhat.com&gt; (but I'd wait with pushing until after the release)