[libvirt] [PATCH] nodedev: update mdev_types caps before dumpxml
Wu Zongyong (1): nodedev: update mdev_types caps before dumpxml src/node_device/node_device_linux_sysfs.c | 42 +++++++++++++++++++++++++++++++ src/node_device/node_device_udev.c | 11 ++++++-- src/node_device/node_device_udev.h | 8 ++++++ 3 files changed, 59 insertions(+), 2 deletions(-) -- 1.9.1
In current implemention, mdev_types caps keep constant all the time. But, it is possible that a device capable of mdev_types sometime(for example:bind to proper driver) and incapable of mdev_types at other times(for example: unbind from its driver). We should keep the info of xml dumped out consistent with real status of the device. --- src/node_device/node_device_linux_sysfs.c | 42 +++++++++++++++++++++++++++++++ src/node_device/node_device_udev.c | 11 ++++++-- src/node_device/node_device_udev.h | 8 ++++++ 3 files changed, 59 insertions(+), 2 deletions(-) diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c index 6f438e5..2dc01bf 100644 --- a/src/node_device/node_device_linux_sysfs.c +++ b/src/node_device/node_device_linux_sysfs.c @@ -29,6 +29,7 @@ #include "dirname.h" #include "node_device_driver.h" #include "node_device_hal.h" +#include "node_device_udev.h" #include "node_device_linux_sysfs.h" #include "virerror.h" #include "viralloc.h" @@ -175,6 +176,45 @@ nodeDeviceSysfsGetPCIIOMMUGroupCaps(virNodeDevCapPCIDevPtr pci_dev) return ret; } +static int +nodeDeviceSysfsGetPCIMdevTypesCaps(const char *sysfsPath, + virNodeDevCapPCIDevPtr pci_dev) +{ + size_t i; + int ret = -1; + struct udev *udev = NULL; + struct udev_device *device = NULL; + + /* this could be a refresh, so clear out the old data */ + for (i = 0; i < pci_dev->nmdev_types; i++) + VIR_FREE(pci_dev->mdev_types[i]); + VIR_FREE(pci_dev->mdev_types); + pci_dev->nmdev_types = 0; + + udev = udev_new(); + if (!udev) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to create udev context")); + goto cleanup; + } + device = udev_device_new_from_syspath(udev, sysfsPath); + if (!device) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to create udev device from path %s"), + sysfsPath); + goto cleanup; + } + + if (udevPCIGetMdevTypesCap(device, pci_dev) < 0) + goto cleanup; + + ret = 0; + cleanup: + udev_device_unref(device); + udev_unref(udev); + return ret; +} + /* nodeDeviceSysfsGetPCIRelatedCaps() get info that is stored in sysfs * about devices related to this device, i.e. things that can change @@ -190,6 +230,8 @@ nodeDeviceSysfsGetPCIRelatedDevCaps(const char *sysfsPath, return -1; if (nodeDeviceSysfsGetPCIIOMMUGroupCaps(pci_dev) < 0) return -1; + if (nodeDeviceSysfsGetPCIMdevTypesCaps(sysfsPath, pci_dev) < 0) + return -1; return 0; } diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 1e1b717..256bb66 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -432,7 +432,7 @@ udevFillMdevType(struct udev_device *device, } -static int +int udevPCIGetMdevTypesCap(struct udev_device *device, virNodeDevCapPCIDevPtr pcidata) { @@ -599,8 +599,15 @@ udevProcessPCI(struct udev_device *device, /* check whether the device is mediated devices framework capable, if so, * process it + * + * UDEV doesn't report attributes under subdirectories by default but is + * able to query them if the path to the attribute is relative to the + * device's base path, e.g. /sys/devices/../0000:00:01.0/ is the device's + * base path as udev reports it, but we're interested in attributes under + * /sys/devices/../0000:00:01.0/mdev_supported_types/<type>/. So, we need to + * scan the subdirectories ourselves. */ - if (udevPCIGetMdevTypesCap(device, pci_dev) < 0) + if (udevPCIGetMdevTypesCap(udev_device_get_syspath(device), pci_dev) < 0) goto cleanup; ret = 0; diff --git a/src/node_device/node_device_udev.h b/src/node_device/node_device_udev.h index f15e520..9f78289 100644 --- a/src/node_device/node_device_udev.h +++ b/src/node_device/node_device_udev.h @@ -19,6 +19,8 @@ * * Author: Dave Allan <dallan@redhat.com> */ +#ifndef __VIR_NODE_DEVICE_UDEV_H__ +#define __VIR_NODE_DEVICE_UDEV_H__ #include <libudev.h> #include <stdint.h> @@ -26,3 +28,9 @@ #define SYSFS_DATA_SIZE 4096 #define DMI_DEVPATH "/sys/devices/virtual/dmi/id" #define DMI_DEVPATH_FALLBACK "/sys/class/dmi/id" + +int +udevPCIGetMdevTypesCap(struct udev_device *device, virNodeDevCapPCIDevPtr pcidata); + + +#endif /* __VIR_NODE_DEVICE_UDEV_H__ */ -- 1.9.1
On Tue, Dec 26, 2017 at 07:55:52PM +0800, Wu Zongyong wrote:
In current implemention, mdev_types caps keep constant all the time. But, it is possible that a device capable of mdev_types sometime(for example:bind to proper driver) and incapable of mdev_types at other times(for example: unbind from its driver). We should keep the info of xml dumped out consistent with real status of the device.
The idea is right, we should adjust the capabilities depending on the current driver the device is bound to, however see my comments below. The patch doesn't compile and fails syntax-check, before sending patches, always run make check syntax-check. [...]
+static int +nodeDeviceSysfsGetPCIMdevTypesCaps(const char *sysfsPath, + virNodeDevCapPCIDevPtr pci_dev) +{ + size_t i; + int ret = -1; + struct udev *udev = NULL; + struct udev_device *device = NULL; + + /* this could be a refresh, so clear out the old data */ + for (i = 0; i < pci_dev->nmdev_types; i++) + VIR_FREE(pci_dev->mdev_types[i]); + VIR_FREE(pci_dev->mdev_types);
I assume this is a copy paste from the other XGetYCaps methods, because this leaks memory - virNodeDevCapMdevTypeFree should be used here instead.
+ pci_dev->nmdev_types = 0; + + udev = udev_new();
Although it doesn't make any difference for libvirt, creating new context just because you need an object of a specific type is not the correct approach, we already have an existing context in the driver object. Having said that, we don't make use of the udev's @userdata, therefore you don't even need to ref it when you extract it from the nodedev driver object.
+ if (!udev) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to create udev context")); + goto cleanup; + }
^This hunk will become unnecessary. [...]
/* check whether the device is mediated devices framework capable, if so, * process it + * + * UDEV doesn't report attributes under subdirectories by default but is + * able to query them if the path to the attribute is relative to the + * device's base path, e.g. /sys/devices/../0000:00:01.0/ is the device's + * base path as udev reports it, but we're interested in attributes under + * /sys/devices/../0000:00:01.0/mdev_supported_types/<type>/. So, we need to + * scan the subdirectories ourselves.
The same commentary is already part of udevPCIGetMdevTypesCap, why do we need it ^here as well?
*/ - if (udevPCIGetMdevTypesCap(device, pci_dev) < 0) + if (udevPCIGetMdevTypesCap(udev_device_get_syspath(device), pci_dev) < 0) goto cleanup;
^this bit causes the patch to fail compilation.
ret = 0; diff --git a/src/node_device/node_device_udev.h b/src/node_device/node_device_udev.h index f15e520..9f78289 100644 --- a/src/node_device/node_device_udev.h +++ b/src/node_device/node_device_udev.h @@ -19,6 +19,8 @@ * * Author: Dave Allan <dallan@redhat.com> */ +#ifndef __VIR_NODE_DEVICE_UDEV_H__ +#define __VIR_NODE_DEVICE_UDEV_H__
I guess this might be considered a change for a separate patch, however the change is tiny and everyone understands what the preprocessor macro means, so no need to split it really. What needs to be adjusted though is the indentation because the whole hunk fails syntax-check.
#include <libudev.h> #include <stdint.h> @@ -26,3 +28,9 @@ #define SYSFS_DATA_SIZE 4096 #define DMI_DEVPATH "/sys/devices/virtual/dmi/id" #define DMI_DEVPATH_FALLBACK "/sys/class/dmi/id" + +int +udevPCIGetMdevTypesCap(struct udev_device *device, virNodeDevCapPCIDevPtr pcidata); + + +#endif /* __VIR_NODE_DEVICE_UDEV_H__ */
Erik
participants (2)
-
Erik Skultety -
Wu Zongyong