[v1] nodedev: update mdev_types caps before dumpxml

[libvirt] [PATCH] nodedev: update mdev_types caps before dumpxml

Posted by Wu Zongyong 6 years, 3 months ago

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

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

Re: [libvirt] [PATCH] nodedev: update mdev_types caps before dumpxml

Posted by Erik Skultety 6 years, 3 months ago

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

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