This is a generic function that you can provide your own predicate function to search for a particular device. It will be used in an upcoming commit. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/conf/virnodedeviceobj.c | 53 +++++++++++++++++++++++++++++++------ src/conf/virnodedeviceobj.h | 11 +++++--- src/libvirt_private.syms | 1 + 3 files changed, 54 insertions(+), 11 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index b213592b56..6e7b354f96 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -1026,19 +1026,19 @@ virNodeDeviceObjSetPersistent(virNodeDeviceObj *obj, } -struct virNodeDeviceObjListRemoveHelperData +typedef struct { - virNodeDeviceObjListRemoveIterator callback; + virNodeDeviceObjListPredicate predicate; void *opaque; -}; +} PredicateHelperData; static int virNodeDeviceObjListRemoveHelper(void *key G_GNUC_UNUSED, void *value, void *opaque) { - struct virNodeDeviceObjListRemoveHelperData *data = opaque; + PredicateHelperData *data = opaque; - return data->callback(value, data->opaque); + return data->predicate(value, data->opaque); } @@ -1054,11 +1054,11 @@ static int virNodeDeviceObjListRemoveHelper(void *key G_GNUC_UNUSED, */ void virNodeDeviceObjListForEachRemove(virNodeDeviceObjList *devs, - virNodeDeviceObjListRemoveIterator callback, + virNodeDeviceObjListPredicate callback, void *opaque) { - struct virNodeDeviceObjListRemoveHelperData data = { - .callback = callback, + PredicateHelperData data = { + .predicate = callback, .opaque = opaque }; @@ -1068,3 +1068,40 @@ virNodeDeviceObjListForEachRemove(virNodeDeviceObjList *devs, &data); virObjectRWUnlock(devs); } + + +static int virNodeDeviceObjListFindHelper(const void *payload, + const char *name G_GNUC_UNUSED, + const void *opaque) +{ + PredicateHelperData *data = (PredicateHelperData *) opaque; + virNodeDeviceObj *obj = (virNodeDeviceObj *) payload; + + return data->predicate(obj, data->opaque); +} + + +/** + * virNodeDeviceObjListFind + * @devs: Pointer to object list + * @predicate: function to test the device for a certain property + * @opaque: Opaque data to use as argument to helper + * + * For each object in @devs, call the @predicate helper using @opaque as + * an argument until it returns TRUE. The list may not be modified while + * iterating. + */ +virNodeDeviceObj * +virNodeDeviceObjListFind(virNodeDeviceObjList *devs, + virNodeDeviceObjListPredicate predicate, + void *opaque) +{ + PredicateHelperData data = { + .predicate = predicate, + .opaque = opaque + }; + + return virNodeDeviceObjListSearch(devs, + virNodeDeviceObjListFindHelper, + &data); +} diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 7353e4619b..0cb78748a4 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -136,9 +136,14 @@ void virNodeDeviceObjSetPersistent(virNodeDeviceObj *obj, bool persistent); -typedef bool (*virNodeDeviceObjListRemoveIterator)(virNodeDeviceObj *obj, - const void *opaque); +typedef bool (*virNodeDeviceObjListPredicate)(virNodeDeviceObj *obj, + const void *opaque); void virNodeDeviceObjListForEachRemove(virNodeDeviceObjList *devs, - virNodeDeviceObjListRemoveIterator callback, + virNodeDeviceObjListPredicate callback, void *opaque); + +virNodeDeviceObj * +virNodeDeviceObjListFind(virNodeDeviceObjList *devs, + virNodeDeviceObjListPredicate callback, + void *opaque); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4bc2974e7f..9d16d7c6b3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1287,6 +1287,7 @@ virNodeDeviceObjIsActive; virNodeDeviceObjIsPersistent; virNodeDeviceObjListAssignDef; virNodeDeviceObjListExport; +virNodeDeviceObjListFind; virNodeDeviceObjListFindByName; virNodeDeviceObjListFindBySysfsPath; virNodeDeviceObjListFindMediatedDeviceByUUID; -- 2.31.1

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(a)redhat.com&gt; --- 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> <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"); + 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; -- 2.31.1

mdevctl can report multiple defined devices with the same UUID but different parents, including parents that don't actually exist on the host machine. Libvirt sets the parent to the 'computer' device for all of the mdevs that have nonexistent parents. Because of this, it's possible that there are multiple devices with the same UUID and the same 'computer' device as their parent, so the combination of uuid and parent 'nodedev name is not guaranteed to be a unique name. We need to ensure that each nodedev has a unique name. If we can't use the UUID as a unique nodedev name, and we can't use the combination of UUID and nodedev parent name, we need to find another solution. By caching and using the parent name reported by mdevctl in combination with the UUID, we can achieve a unique name. mdevctl guarantees that its uuid/parent combination is unique. This value will be used to set the mdev nodedev name in a following commit. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/conf/node_device_conf.h | 1 + src/node_device/node_device_driver.c | 1 + src/node_device/node_device_udev.c | 16 ++++++++++++++++ 3 files changed, 18 insertions(+) diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index a60562e4fe..556878b9a8 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -153,6 +153,7 @@ struct _virNodeDevCapMdev { char *uuid; virMediatedDeviceAttr **attributes; size_t nattributes; + char *parent_addr; }; typedef struct _virNodeDevCapPCIDev virNodeDevCapPCIDev; diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index a3a261f08b..ad2ca2a614 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1088,6 +1088,7 @@ nodeDeviceParseMdevctlChildDevice(const char *parent, mdev = &child->caps->data.mdev; mdev->uuid = g_strdup(uuid); + mdev->parent_addr = g_strdup(parent); mdev->type = g_strdup(virJSONValueObjectGetString(props, "mdev_type")); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index f99578414d..e7db74b325 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1024,6 +1024,7 @@ udevProcessMediatedDevice(struct udev_device *dev, char *linkpath = NULL; char *canonicalpath = NULL; virNodeDevCapMdev *data = &def->caps->data.mdev; + struct udev_device *parent_device = NULL; /* Because of a kernel uevent race, we might get the 'add' event prior to * the sysfs tree being ready, so any attempt to access any sysfs attribute @@ -1051,6 +1052,21 @@ udevProcessMediatedDevice(struct udev_device *dev, if ((iommugrp = virMediatedDeviceGetIOMMUGroupNum(data->uuid)) < 0) goto cleanup; + /* lookup the address of parent device */ + parent_device = udev_device_get_parent(dev); + if (parent_device) { + const char *parent_sysfs_path = udev_device_get_syspath(parent_device); + if (parent_sysfs_path) + data->parent_addr = g_path_get_basename(parent_sysfs_path); + } + + if (!data->parent_addr) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get parent of '%s'"), + udev_device_get_syspath(dev)); + return -1; + } + udevGenerateDeviceName(dev, def, NULL); data->iommuGroupNumber = iommugrp; -- 2.31.1

At the moment, this is only for mediated devices. When a new mediated device is created or defined, the xml is expected specify the nodedev name of an existing device as its parent. We were not previously validating this and were simply accepting any string here. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/conf/node_device_conf.c | 30 +++++++++--- src/conf/node_device_conf.h | 20 +++++++- src/conf/virnodedeviceobj.h | 1 + src/hypervisor/domain_driver.c | 7 +-- src/node_device/node_device_driver.c | 68 +++++++++++++++++++++++++++- src/node_device/node_device_driver.h | 3 ++ src/node_device/node_device_udev.c | 2 + src/test/test_driver.c | 6 ++- tests/nodedevmdevctltest.c | 7 ++- tests/nodedevxml2xmltest.c | 3 +- 10 files changed, 129 insertions(+), 18 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 332b12f997..cd1c07092d 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -2174,10 +2174,12 @@ static virNodeDeviceDef * virNodeDeviceDefParse(const char *str, const char *filename, int create, - const char *virt_type) + const char *virt_type, + virNodeDeviceDefParserCallbacks *parserCallbacks, + void *opaque) { xmlDocPtr xml; - virNodeDeviceDef *def = NULL; + g_autoptr(virNodeDeviceDef) def = NULL; if ((xml = virXMLParse(filename, str, _("(node_device_definition)")))) { def = virNodeDeviceDefParseNode(xml, xmlDocGetRootElement(xml), @@ -2185,25 +2187,39 @@ virNodeDeviceDefParse(const char *str, xmlFreeDoc(xml); } - return def; + if (parserCallbacks) { + int ret = 0; + /* validate definition */ + if (parserCallbacks->validate) { + ret = parserCallbacks->validate(def, opaque); + if (ret < 0) + return NULL; + } + } + + return g_steal_pointer(&def); } virNodeDeviceDef * virNodeDeviceDefParseString(const char *str, int create, - const char *virt_type) + const char *virt_type, + virNodeDeviceDefParserCallbacks *parserCallbacks, + void *opaque) { - return virNodeDeviceDefParse(str, NULL, create, virt_type); + return virNodeDeviceDefParse(str, NULL, create, virt_type, parserCallbacks, opaque); } virNodeDeviceDef * virNodeDeviceDefParseFile(const char *filename, int create, - const char *virt_type) + const char *virt_type, + virNodeDeviceDefParserCallbacks *parserCallbacks, + void *opaque) { - return virNodeDeviceDefParse(NULL, filename, create, virt_type); + return virNodeDeviceDefParse(NULL, filename, create, virt_type, parserCallbacks, opaque); } diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index 556878b9a8..786de85060 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -349,15 +349,31 @@ struct _virNodeDeviceDef { char * virNodeDeviceDefFormat(const virNodeDeviceDef *def); + +typedef int (*virNodeDeviceDefPostParseCallback)(virNodeDeviceDef *dev, + void *opaque); + +typedef int (*virNodeDeviceDefValidateCallback)(virNodeDeviceDef *dev, + void *opaque); + +typedef struct _virNodeDeviceDefParserCallbacks { + virNodeDeviceDefPostParseCallback postParse; + virNodeDeviceDefValidateCallback validate; +} virNodeDeviceDefParserCallbacks; + virNodeDeviceDef * virNodeDeviceDefParseString(const char *str, int create, - const char *virt_type); + const char *virt_type, + virNodeDeviceDefParserCallbacks *callbacks, + void *opaque); virNodeDeviceDef * virNodeDeviceDefParseFile(const char *filename, int create, - const char *virt_type); + const char *virt_type, + virNodeDeviceDefParserCallbacks *callbacks, + void *opaque); virNodeDeviceDef * virNodeDeviceDefParseNode(xmlDocPtr xml, diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 0cb78748a4..1fdd4f65da 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -47,6 +47,7 @@ struct _virNodeDeviceDriverState { /* Immutable pointer, self-locking APIs */ virObjectEventState *nodeDeviceEventState; + virNodeDeviceDefParserCallbacks parserCallbacks; }; void diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index 29e11c0447..2969d55173 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -395,7 +395,8 @@ virDomainDriverNodeDeviceReset(virNodeDevicePtr dev, if (!xml) return -1; - def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL); + def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL, + NULL, NULL); if (!def) return -1; @@ -440,7 +441,7 @@ virDomainDriverNodeDeviceReAttach(virNodeDevicePtr dev, if (!xml) return -1; - def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL); + def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL, NULL, NULL); if (!def) return -1; @@ -488,7 +489,7 @@ virDomainDriverNodeDeviceDetachFlags(virNodeDevicePtr dev, if (!xml) return -1; - def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL); + def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL, NULL, NULL); if (!def) return -1; diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index ad2ca2a614..8f39d79c68 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -896,7 +896,8 @@ nodeDeviceCreateXML(virConnectPtr conn, virt_type = virConnectGetType(conn); - if (!(def = virNodeDeviceDefParseString(xmlDesc, CREATE_DEVICE, virt_type))) + if (!(def = virNodeDeviceDefParseString(xmlDesc, CREATE_DEVICE, virt_type, + &driver->parserCallbacks, NULL))) return NULL; if (virNodeDeviceCreateXMLEnsureACL(conn, def) < 0) @@ -1376,7 +1377,8 @@ nodeDeviceDefineXML(virConnect *conn, virt_type = virConnectGetType(conn); - if (!(def = virNodeDeviceDefParseString(xmlDesc, CREATE_DEVICE, virt_type))) + if (!(def = virNodeDeviceDefParseString(xmlDesc, CREATE_DEVICE, virt_type, + &driver->parserCallbacks, NULL))) return NULL; if (virNodeDeviceDefineXMLEnsureACL(conn, def) < 0) @@ -1761,3 +1763,65 @@ nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst, return ret; } + + +/* validate that parent exists */ +static int nodeDeviceDefValidateMdev(virNodeDeviceDef *def, + G_GNUC_UNUSED virNodeDevCapMdev *mdev, + G_GNUC_UNUSED void *opaque) +{ + virNodeDeviceObj *obj = NULL; + if (!def->parent) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("missing parent device")); + return -1; + } + obj = virNodeDeviceObjListFindByName(driver->devs, def->parent); + if (!obj) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("invalid parent device '%s'"), + def->parent); + return -1; + } + + virNodeDeviceObjEndAPI(&obj); + return 0; +} + +int nodeDeviceDefValidate(virNodeDeviceDef *def, + G_GNUC_UNUSED void *opaque) +{ + virNodeDevCapsDef *caps = NULL; + for (caps = def->caps; caps != NULL; caps = caps->next) { + switch (caps->data.type) { + case VIR_NODE_DEV_CAP_MDEV: + if (nodeDeviceDefValidateMdev(def, &caps->data.mdev, opaque) < 0) + return -1; + break; + + case VIR_NODE_DEV_CAP_SYSTEM: + case VIR_NODE_DEV_CAP_PCI_DEV: + case VIR_NODE_DEV_CAP_USB_DEV: + case VIR_NODE_DEV_CAP_USB_INTERFACE: + case VIR_NODE_DEV_CAP_NET: + case VIR_NODE_DEV_CAP_SCSI_HOST: + case VIR_NODE_DEV_CAP_SCSI_TARGET: + case VIR_NODE_DEV_CAP_SCSI: + case VIR_NODE_DEV_CAP_STORAGE: + case VIR_NODE_DEV_CAP_FC_HOST: + case VIR_NODE_DEV_CAP_VPORTS: + case VIR_NODE_DEV_CAP_SCSI_GENERIC: + case VIR_NODE_DEV_CAP_DRM: + case VIR_NODE_DEV_CAP_MDEV_TYPES: + case VIR_NODE_DEV_CAP_CCW_DEV: + case VIR_NODE_DEV_CAP_CSS_DEV: + case VIR_NODE_DEV_CAP_VDPA: + case VIR_NODE_DEV_CAP_AP_CARD: + case VIR_NODE_DEV_CAP_AP_QUEUE: + case VIR_NODE_DEV_CAP_AP_MATRIX: + case VIR_NODE_DEV_CAP_LAST: + break; + } + } + return 0; +} diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index edd763f0e4..d9b9b7a961 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -171,3 +171,6 @@ bool nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst, int nodeDeviceCreate(virNodeDevice *dev, unsigned int flags); + +int nodeDeviceDefValidate(virNodeDeviceDef *def, + void *opaque); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index e7db74b325..ccf94d8e7d 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2243,6 +2243,8 @@ nodeStateInitialize(bool privileged, driver->privateData = priv; driver->nodeDeviceEventState = virObjectEventStateNew(); + driver->parserCallbacks.validate = nodeDeviceDefValidate; + if (udevPCITranslateInit(privileged) < 0) goto unlock; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 892dc978f2..c2d642ff6b 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -7481,7 +7481,8 @@ testNodeDeviceMockCreateVport(testDriver *driver, if (!xml) goto cleanup; - if (!(def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL))) + if (!(def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL, NULL, + NULL))) goto cleanup; VIR_FREE(def->name); @@ -7543,7 +7544,8 @@ testNodeDeviceCreateXML(virConnectPtr conn, virCheckFlags(0, NULL); - if (!(def = virNodeDeviceDefParseString(xmlDesc, CREATE_DEVICE, NULL))) + if (!(def = virNodeDeviceDefParseString(xmlDesc, CREATE_DEVICE, NULL, + NULL, NULL))) goto cleanup; /* We run this simply for validation - it essentially validates that diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index 7af89ab5f1..bf1d7b4984 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -12,6 +12,10 @@ #define VIRT_TYPE "QEMU" +static virNodeDeviceDefParserCallbacks parser_callbacks = { + .validate = nodeDeviceDefValidate +}; + struct TestInfo { const char *filename; virMdevctlCommand command; @@ -66,7 +70,8 @@ testMdevctlCmd(virMdevctlCommand cmd_type, return -1; } - if (!(def = virNodeDeviceDefParseFile(mdevxml, create, VIRT_TYPE))) + if (!(def = virNodeDeviceDefParseFile(mdevxml, create, VIRT_TYPE, + &parser_callbacks, NULL))) return -1; /* this function will set a stdin buffer containing the json configuration diff --git a/tests/nodedevxml2xmltest.c b/tests/nodedevxml2xmltest.c index e9716726d7..d8f81456ee 100644 --- a/tests/nodedevxml2xmltest.c +++ b/tests/nodedevxml2xmltest.c @@ -25,7 +25,8 @@ testCompareXMLToXMLFiles(const char *xml, const char *outfile) if (virTestLoadFile(xml, &xmlData) < 0) goto fail; - if (!(dev = virNodeDeviceDefParseString(xmlData, EXISTING_DEVICE, NULL))) + if (!(dev = virNodeDeviceDefParseString(xmlData, EXISTING_DEVICE, NULL, + NULL, NULL))) goto fail; /* Calculate some things that are not read in */ -- 2.31.1

This can be used similarly to other postparse callbacks in libvirt -- filling in additional information that can be determined by using the information provided in the XML. In this case, we determine the address of the parent device and cache it in the mdev caps so that we can use it for generating a unique name and interacting with mdevctl. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/conf/node_device_conf.c | 7 +++++ src/node_device/node_device_driver.c | 45 +++++++++++++++++++++------- src/node_device/node_device_driver.h | 3 ++ src/node_device/node_device_udev.c | 1 + tests/nodedevmdevctltest.c | 1 + 5 files changed, 46 insertions(+), 11 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index cd1c07092d..a7cd90b9e6 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -2189,6 +2189,13 @@ virNodeDeviceDefParse(const char *str, if (parserCallbacks) { int ret = 0; + /* fill in backend-specific aspects */ + if (parserCallbacks->postParse) { + ret = parserCallbacks->postParse(def, opaque); + if (ret < 0) + return NULL; + } + /* validate definition */ if (parserCallbacks->validate) { ret = parserCallbacks->validate(def, opaque); diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 8f39d79c68..d76268285f 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -718,11 +718,9 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def, char **outbuf, char **errbuf) { - g_autofree char *parent_addr = NULL; 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: @@ -747,12 +745,7 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def, switch (cmd_type) { case MDEVCTL_CMD_CREATE: case MDEVCTL_CMD_DEFINE: - if ((parent_obj = nodeDeviceObjFindByName(def->parent))) { - parent_addr = nodeDeviceObjFormatAddress(parent_obj); - virNodeDeviceObjEndAPI(&parent_obj); - } - - if (!parent_addr) { + if (!def->caps->data.mdev.parent_addr) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to find parent device '%s'"), def->parent); return NULL; @@ -764,7 +757,7 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def, return NULL; } - virCommandAddArgPair(cmd, "--parent", parent_addr); + virCommandAddArgPair(cmd, "--parent", def->caps->data.mdev.parent_addr); virCommandAddArgPair(cmd, "--jsonfile", "/dev/stdin"); virCommandSetInputBuffer(cmd, inbuf); @@ -1765,9 +1758,30 @@ nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst, } +int nodeDeviceDefPostParse(virNodeDeviceDef *def, + G_GNUC_UNUSED void *opaque) +{ + virNodeDevCapsDef *caps = NULL; + for (caps = def->caps; caps != NULL; caps = caps->next) { + if (caps->data.type == VIR_NODE_DEV_CAP_MDEV) { + virNodeDeviceObj *obj = NULL; + + if (def->parent) + obj = virNodeDeviceObjListFindByName(driver->devs, def->parent); + + if (obj) { + caps->data.mdev.parent_addr = nodeDeviceObjFormatAddress(obj); + virNodeDeviceObjEndAPI(&obj); + } + } + } + return 0; +} + + /* validate that parent exists */ static int nodeDeviceDefValidateMdev(virNodeDeviceDef *def, - G_GNUC_UNUSED virNodeDevCapMdev *mdev, + virNodeDevCapMdev *mdev, G_GNUC_UNUSED void *opaque) { virNodeDeviceObj *obj = NULL; @@ -1783,8 +1797,17 @@ static int nodeDeviceDefValidateMdev(virNodeDeviceDef *def, def->parent); return -1; } - virNodeDeviceObjEndAPI(&obj); + + /* the post-parse callback should have found the address of the parent + * device and stored it in the mdev caps */ + if (!mdev->parent_addr) { + virReportError(VIR_ERR_PARSE_FAILED, + _("Unable to find address for parent device '%s'"), + def->parent); + return -1; + } + return 0; } diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index d9b9b7a961..fdc92b8aef 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -172,5 +172,8 @@ int nodeDeviceCreate(virNodeDevice *dev, unsigned int flags); +int nodeDeviceDefPostParse(virNodeDeviceDef *def, + void *opaque); + int nodeDeviceDefValidate(virNodeDeviceDef *def, void *opaque); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index ccf94d8e7d..81037d8139 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2243,6 +2243,7 @@ nodeStateInitialize(bool privileged, driver->privateData = priv; driver->nodeDeviceEventState = virObjectEventStateNew(); + driver->parserCallbacks.postParse = nodeDeviceDefPostParse; driver->parserCallbacks.validate = nodeDeviceDefValidate; if (udevPCITranslateInit(privileged) < 0) diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index bf1d7b4984..d88e08f485 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -13,6 +13,7 @@ #define VIRT_TYPE "QEMU" static virNodeDeviceDefParserCallbacks parser_callbacks = { + .postParse = nodeDeviceDefPostParse, .validate = nodeDeviceDefValidate }; -- 2.31.1

On 7/23/21 6:40 PM, Jonathon Jongsma wrote: Again, typedef on one line, struct on the other. Michal