[libvirt] [RFC PATCH 07/11] nodedev: Fill in the mdev info for the parent PCI device
Pavel Hrdina
phrdina at redhat.com
Tue Apr 4 14:49:22 UTC 2017
On Wed, Mar 29, 2017 at 02:51:17PM +0200, Erik Skultety wrote:
> The parent device needs to report the generic stuff about the supported
> mediated devices types, like device API, available instances, and
> description which might hold some useful data about supported
> resolutions for the type given, framebuffer size, etc. Unfortunately,
> these are not standardized yet to be considered for separate elements.
>
> Signed-off-by: Erik Skultety <eskultet at redhat.com>
> ---
> src/conf/node_device_conf.h | 2 +
> src/node_device/node_device_udev.c | 115 +++++++++++++++++++++++++++++++++++++
> 2 files changed, 117 insertions(+)
>
> diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
> index b7db35dd50..d521dd9a4c 100644
> --- a/src/conf/node_device_conf.h
> +++ b/src/conf/node_device_conf.h
> @@ -166,6 +166,8 @@ struct _virNodeDevCapPCIDev {
> int numa_node;
> virPCIEDeviceInfoPtr pci_express;
> int hdrType; /* enum virPCIHeaderType or -1 */
> + virNodeDevCapMdevPtr *mdevs;
> + size_t nmdevs;
> };
>
> typedef struct _virNodeDevCapUSBDev virNodeDevCapUSBDev;
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index dbbd8e5d06..be031fa5a8 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -324,6 +324,115 @@ udevTranslatePCIIds(unsigned int vendor,
> return 0;
> }
>
> +#define MDEV_GET_SYSFS_ATTR(attr, cb, ...) \
> + do { \
> + if (virAsprintf(&attrpath, "%s/%s", relpath, #attr) < 0) \
> + goto cleanup; \
> + \
> + if (cb(device, attrpath, __VA_ARGS__) < 0) \
> + goto cleanup; \
You need to free *attrpath*, otherwise every next call of this macro will
store new string into that variable and the previous one is leaked.
> + } while (0) \
I would move the macro into the function in order to make it clear that
the macro supposed to be used only inside that function because it uses
variables available only inside the function and jumps to *cleanup* label.
Usually you define this type of macro, use it and undefine it.
> +
> +
> +static int
> +udevGetMdevCaps(struct udev_device *device,
> + const char *sysfspath,
> + virNodeDevCapMdevPtr mdev)
> +{
> + int ret = -1;
> + char *attrpath = NULL; /* relative path to the actual sysfs attribute */
> + const char *devpath = NULL; /* base sysfs path as reported by udev */
> + const char *relpath = NULL; /* diff between @sysfspath and @devpath */
> + char *tmp = NULL;
> +
> +
> + /* UDEV doesn't report attributes under subdirectories but can query them
> + * if specified as relative paths 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, let's
> + * strip the common part of the path and let udev chew the relative bit.
> + */
> + devpath = udev_device_get_syspath(device);
> + relpath = sysfspath + strlen(devpath);
> +
> + /* When calling from the mdev child device, @sysfspath is a symbolic link
> + * to the actual mdev type (rather than a physical path), so we need to
> + * resolve it in order to get the type's name.
> + */
> + if (virFileResolveLink(sysfspath, &tmp) < 0)
> + goto cleanup;
> +
> + if (VIR_STRDUP(mdev->type, last_component(tmp)) < 0)
> + goto cleanup;
> +
> + MDEV_GET_SYSFS_ATTR(name, udevGetStringSysfsAttr,
> + &mdev->name);
> + MDEV_GET_SYSFS_ATTR(description, udevGetStringSysfsAttr,
> + &mdev->description);
> + MDEV_GET_SYSFS_ATTR(device_api, udevGetStringSysfsAttr,
> + &mdev->device_api);
> + MDEV_GET_SYSFS_ATTR(available_instances, udevGetUintSysfsAttr,
> + &mdev->available_instances, 10);
> +
> + ret = 0;
> + cleanup:
> + VIR_FREE(attrpath);
> + VIR_FREE(tmp);
> + return ret;
> +}
> +#undef MDEV_GET_SYSFS_ATTR
> +
> +
> +static int
> +udevPCIGetMdevCaps(struct udev_device *device,
> + virNodeDevCapPCIDevPtr pcidata)
> +{
> + int ret = -1;
> + DIR *dir = NULL;
> + struct dirent *entry;
> + char *path = NULL;
> + char *tmppath = NULL;
> + virNodeDevCapMdevPtr mdev = NULL;
> +
> + if (virAsprintf(&path, "%s/mdev_supported_types",
> + udev_device_get_syspath(device)) < 0)
> + return -1;
> +
> + if ((ret = virDirOpenIfExists(&dir, path)) <= 0)
> + goto cleanup;
This is usually a source of a lot of bugs. I would rather have another
variable called *rc*:
if ((rc = virDirOpenIfExists(&dir, path)) < 0)
goto cleanup;
if (rc == 0) {
ret = 0;
goto cleanup;
}
The reason for doing this is to make it clear what's happening, ...
> +
> + if (VIR_ALLOC(pcidata->mdevs) < 0)
> + goto cleanup;
This should be freed in virNodeDevCapsDefFree including all the mdev caps
inside that array if there are any.
> + /* since udev doesn't provide means to list other than top-level
> + * attributes, we need to scan the subdirectories ourselves
> + */
> + while ((ret = virDirRead(dir, &entry, path)) > 0) {
> + if (VIR_ALLOC(mdev) < 0)
> + goto cleanup;
because here for example if the allocation fails the *ret* contains *1*
and you jump directly to *cleanup* and that would result into returning
*1* instead of *-1*.
So there you need to do:
while ((rc = virDirRead(dir, &entry, path)) > 0) {
...
}
if (rc == 0)
ret = 0;
> + if (virAsprintf(&tmppath, "%s/%s", path, entry->d_name) < 0)
> + goto cleanup;
> +
> + if (udevGetMdevCaps(device, tmppath, mdev) < 0)
> + goto cleanup;
> +
> + if (VIR_APPEND_ELEMENT(pcidata->mdevs, pcidata->nmdevs, mdev) < 0)
> + goto cleanup;
> +
> + VIR_FREE(tmppath);
> + }
> +
> + ret = 0;
> + cleanup:
> + VIR_DIR_CLOSE(dir);
> + VIR_FREE(path);
> + VIR_FREE(tmppath);
> + virNodeDevCapMdevFree(mdev);
> + return ret;
> +}
> +
>
> static int
> udevProcessPCI(struct udev_device *device,
> @@ -412,6 +521,12 @@ udevProcessPCI(struct udev_device *device,
> }
> }
>
> + /* check whether the device is mediated devices framework capable, if so,
> + * process it
> + */
> + if (udevPCIGetMdevCaps(device, pci_dev) < 0)
> + goto cleanup;
> +
> ret = 0;
>
> cleanup:
> --
> 2.12.2
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170404/62eb21e4/attachment-0001.sig>
More information about the libvir-list
mailing list