On 7/7/21 11:29 PM, Jonathon Jongsma wrote: > Inactive mdevs were simply formatting their parent name as the value > received from mdevctl rather than looking up the libvirt nodedev name of > the parent device. This resulted in a parent value of e.g. > '0000:5b:00.0' instead of 'pci_0000_5b_00_0'. This prevented defining a > new mdev device from the output of nodedev-dumpxml. > > Unfortunately, it's not simple to fix this comprehensively due to the > fact that mdevctl supports defining (inactive) mdevs for parent devices > that do not actually exist on the host (yet). So for those persistent > mdev definitions that do not have a valid parent in the device list, the > parent device will be set to the root "computer" device. > > Unfortunately, because the value of the 'parent' field now depends on > the configuration of the host, the mdevctl parsing test will output > 'computer' for all test devices. Fixing this would require a more > extensive mock test environment. > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1979761 > > Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx> > > fixup > --- > src/node_device/node_device_driver.c | 20 ++++++++++++++++++- > .../mdevctl-list-multiple.out.xml | 8 ++++---- > 2 files changed, 23 insertions(+), 5 deletions(-) > > diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c > index b4dd57e5f4..26b0c2f032 100644 > --- a/src/node_device/node_device_driver.c > +++ b/src/node_device/node_device_driver.c > @@ -1051,6 +1051,7 @@ nodeDeviceParseMdevctlChildDevice(const char *parent, > virJSONValue *props; > virJSONValue *attrs; > g_autoptr(virNodeDeviceDef) child = g_new0(virNodeDeviceDef, 1); > + g_autofree char *parent_sysfs_path = NULL; > > /* the child object should have a single key equal to its uuid. > * The value is an object describing the properties of the mdev */ > @@ -1060,7 +1061,24 @@ nodeDeviceParseMdevctlChildDevice(const char *parent, > uuid = virJSONValueObjectGetKey(json, 0); > props = virJSONValueObjectGetValue(json, 0); > > - child->parent = g_strdup(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 = realpath(parent_sysfs_path, NULL); I wonder why syntax-check did not catch this. We prefer virFileCanonicalizePath(). > + virNodeDeviceObj *parentobj = NULL; > + if ((parentobj = virNodeDeviceObjListFindBySysfsPath(driver->devs, And we also like an empty line between these two ^^ to break the block into two. > + canon_syspath))) { > + virNodeDeviceDef *parentdef = virNodeDeviceObjGetDef(parentobj); > + child->parent = g_strdup(parentdef->name); > + virNodeDeviceObjEndAPI(&parentobj); > + > + child->parent_sysfs_path = g_steal_pointer(&canon_syspath); > + } > + } > + if (!child->parent) > + child->parent = g_strdup("computer"); > child->caps = g_new0(virNodeDevCapsDef, 1); > child->caps->data.type = VIR_NODE_DEV_CAP_MDEV; > Reviewed-by: Michal Privoznik <mprivozn@xxxxxxxxxx> Michal
