[libvirt PATCH 2/7] nodedev: fix xml output for mdev parents in test suite
Shalini Chellathurai Saroja
shalini at linux.ibm.com
Mon Aug 2 10:58:55 UTC 2021
On 7/23/21 6:40 PM, Jonathon Jongsma wrote:
> Commit 51fbbfdce8 attempted to get the proper nodedev name for the
> parent of an defined mdev by traversing the filesystem and looking for a
> device that had the appropriate sysfs path. This works, but it would be
> cleaner to to avoid mucking around in the filesystem and instead just
> just examine the list of devices we have in memory.
>
> We already had a function nodeDeviceFindAddressByName() which constructs
> an address for parent device in a format that can be used with mdevctl.
> So if we refactor this function into a a function that simply formats an
> address for an arbitrary virNodeDeviceObj*, then we can use this
> function as a predicate for our new virNodeDeviceObjListFind() function
> from the previous commit. This will search our list of devices for one
> whose address matches the address we get from mdevctl.
>
> One nice benefit of this approach is that our test cases will now
> display xml output with the proper parent name for mdevs (assuming that
> we've added the appropriate mock parent devices to the test driver).
> Previously they just displayed 'computer' for the parent because the
> alternative would have required specially constructing a mock filesystem
> environment with a sysfs that mapped to the appropriate parent.
>
> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> ---
> src/node_device/node_device_driver.c | 55 ++++++++++---------
> .../mdevctl-list-multiple.out.xml | 8 +--
> tests/nodedevmdevctltest.c | 28 +++++++++-
> 3 files changed, 59 insertions(+), 32 deletions(-)
>
> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> index 6f8968f1f0..a3a261f08b 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -648,17 +648,11 @@ nodeDeviceDefToMdevctlConfig(virNodeDeviceDef *def, char **buf)
>
>
> static char *
> -nodeDeviceFindAddressByName(const char *name)
> +nodeDeviceObjFormatAddress(virNodeDeviceObj *obj)
> {
> - virNodeDeviceDef *def = NULL;
> virNodeDevCapsDef *caps = NULL;
> char *addr = NULL;
> - virNodeDeviceObj *dev = virNodeDeviceObjListFindByName(driver->devs, name);
> -
> - if (!dev)
> - return NULL;
> -
> - def = virNodeDeviceObjGetDef(dev);
> + virNodeDeviceDef *def = virNodeDeviceObjGetDef(obj);
> for (caps = def->caps; caps != NULL; caps = caps->next) {
> switch (caps->data.type) {
> case VIR_NODE_DEV_CAP_PCI_DEV: {
> @@ -714,8 +708,6 @@ nodeDeviceFindAddressByName(const char *name)
> break;
> }
>
> - virNodeDeviceObjEndAPI(&dev);
> -
> return addr;
> }
>
> @@ -730,6 +722,7 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def,
> g_autoptr(virCommand) cmd = NULL;
> const char *subcommand = virMdevctlCommandTypeToString(cmd_type);
> g_autofree char *inbuf = NULL;
> + virNodeDeviceObj *parent_obj = NULL;
>
> switch (cmd_type) {
> case MDEVCTL_CMD_CREATE:
> @@ -754,7 +747,10 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def,
> switch (cmd_type) {
> case MDEVCTL_CMD_CREATE:
> case MDEVCTL_CMD_DEFINE:
> - parent_addr = nodeDeviceFindAddressByName(def->parent);
> + if ((parent_obj = nodeDeviceObjFindByName(def->parent))) {
> + parent_addr = nodeDeviceObjFormatAddress(parent_obj);
> + virNodeDeviceObjEndAPI(&parent_obj);
> + }
>
> if (!parent_addr) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -1042,6 +1038,21 @@ static void mdevGenerateDeviceName(virNodeDeviceDef *dev)
> }
>
>
> +static bool
> +matchDeviceAddress(virNodeDeviceObj *obj,
> + const void *opaque)
> +{
> + g_autofree char *addr = NULL;
> + bool want = false;
> +
> + virObjectLock(obj);
> + addr = nodeDeviceObjFormatAddress(obj);
> + want = STREQ_NULLABLE(addr, opaque);
> + virObjectUnlock(obj);
> + return want;
> +}
> +
> +
> static virNodeDeviceDef*
> nodeDeviceParseMdevctlChildDevice(const char *parent,
> virJSONValue *json)
> @@ -1051,7 +1062,7 @@ nodeDeviceParseMdevctlChildDevice(const char *parent,
> virJSONValue *props;
> virJSONValue *attrs;
> g_autoptr(virNodeDeviceDef) child = g_new0(virNodeDeviceDef, 1);
> - g_autofree char *parent_sysfs_path = NULL;
> + virNodeDeviceObj *parent_obj;
>
> /* the child object should have a single key equal to its uuid.
> * The value is an object describing the properties of the mdev */
> @@ -1064,20 +1075,12 @@ nodeDeviceParseMdevctlChildDevice(const char *parent,
> /* Look up id of parent device. mdevctl supports defining mdevs for parent
> * devices that are not present on the system (to support starting mdevs on
> * hotplug, etc) so the parent may not actually exist. */
> - parent_sysfs_path = g_strdup_printf("/sys/class/mdev_bus/%s", parent);
> - if (virFileExists(parent_sysfs_path)) {
> - g_autofree char *canon_syspath = virFileCanonicalizePath(parent_sysfs_path);
> - virNodeDeviceObj *parentobj = NULL;
> -
> - if ((parentobj = virNodeDeviceObjListFindBySysfsPath(driver->devs,
> - canon_syspath))) {
> - virNodeDeviceDef *parentdef = virNodeDeviceObjGetDef(parentobj);
> - child->parent = g_strdup(parentdef->name);
> - virNodeDeviceObjEndAPI(&parentobj);
> -
> - child->parent_sysfs_path = g_steal_pointer(&canon_syspath);
> - }
> - }
> + if ((parent_obj = virNodeDeviceObjListFind(driver->devs, matchDeviceAddress,
> + (void *)parent))) {
> + virNodeDeviceDef *parentdef = virNodeDeviceObjGetDef(parent_obj);
> + child->parent = g_strdup(parentdef->name);
> + virNodeDeviceObjEndAPI(&parent_obj);
> + };
> if (!child->parent)
> child->parent = g_strdup("computer");
> child->caps = g_new0(virNodeDevCapsDef, 1);
> diff --git a/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml b/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml
> index eead6f2a40..c23a3130c1 100644
> --- a/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml
> +++ b/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml
> @@ -1,6 +1,6 @@
> <device>
> <name>mdev_200f228a_c80a_4d50_bfb7_f5a0e4e34045</name>
> - <parent>computer</parent>
> + <parent>pci_0000_00_02_0</parent>
> <capability type='mdev'>
> <type id='i915-GVTg_V5_4'/>
> <uuid>200f228a-c80a-4d50-bfb7-f5a0e4e34045</uuid>
> @@ -9,7 +9,7 @@
> </device>
> <device>
> <name>mdev_de807ffc_1923_4d5f_b6c9_b20ecebc6d4b</name>
> - <parent>computer</parent>
> + <parent>pci_0000_00_02_0</parent>
> <capability type='mdev'>
> <type id='i915-GVTg_V5_4'/>
> <uuid>de807ffc-1923-4d5f-b6c9-b20ecebc6d4b</uuid>
> @@ -18,7 +18,7 @@
> </device>
> <device>
> <name>mdev_435722ea_5f43_468a_874f_da34f1217f13</name>
> - <parent>computer</parent>
> + <parent>pci_0000_00_02_0</parent>
> <capability type='mdev'>
> <type id='i915-GVTg_V5_8'/>
> <uuid>435722ea-5f43-468a-874f-da34f1217f13</uuid>
> @@ -28,7 +28,7 @@
> </device>
> <device>
> <name>mdev_783e6dbb_ea0e_411f_94e2_717eaad438bf</name>
> - <parent>computer</parent>
> + <parent>matrix_matrix</parent>
Hello Jonathon,
The nodedev object name of a matrix device is ap_matrix, so please
change matrix_matrix to ap_matrix.
> <capability type='mdev'>
> <type id='vfio_ap-passthrough'/>
> <uuid>783e6dbb-ea0e-411f-94e2-717eaad438bf</uuid>
> diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c
> index e246de4d87..7af89ab5f1 100644
> --- a/tests/nodedevmdevctltest.c
> +++ b/tests/nodedevmdevctltest.c
> @@ -231,7 +231,7 @@ fakeRootDevice(void)
> * parent of the mdev, and it needs a PCI address
> */
> static virNodeDeviceDef *
> -fakeParentDevice(void)
> +fakePCIDevice(void)
> {
> virNodeDeviceDef *def = NULL;
> virNodeDevCapPCIDev *pci_dev;
> @@ -252,6 +252,29 @@ fakeParentDevice(void)
> return def;
> }
>
> +
> +/* Add a fake matrix device that can be used as a parent device for mediated
> + * devices. For our purposes, it only needs to have a name that matches the
> + * parent of the mdev, and it needs the proper name
> + */
> +static virNodeDeviceDef *
> +fakeMatrixDevice(void)
> +{
> + virNodeDeviceDef *def = NULL;
> + virNodeDevCapAPMatrix *cap;
> +
> + def = g_new0(virNodeDeviceDef, 1);
> + def->caps = g_new0(virNodeDevCapsDef, 1);
> +
> + def->name = g_strdup("matrix_matrix");
Please change matrix_matrix to ap_matrix in here as well, thank you.
> + def->parent = g_strdup("computer");
> +
> + def->caps->data.type = VIR_NODE_DEV_CAP_AP_MATRIX;
> + cap = &def->caps->data.ap_matrix;
> + cap->addr = g_strdup("matrix");
> +
> + return def;
> +}
> static int
> addDevice(virNodeDeviceDef *def)
> {
> @@ -274,7 +297,8 @@ static int
> nodedevTestDriverAddTestDevices(void)
> {
> if (addDevice(fakeRootDevice()) < 0 ||
> - addDevice(fakeParentDevice()) < 0)
> + addDevice(fakePCIDevice()) < 0 ||
> + addDevice(fakeMatrixDevice()) < 0)
> return -1;
>
> return 0;
--
Kind regards
Shalini Chellathurai Saroja
Linux on Z and Virtualization Development
Vorsitzende des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
More information about the libvir-list
mailing list