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 <eskul...@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

Reply via email to