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@xxxxxxxxxx>
> */
> +#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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
