[PATCH v5 0/3] test: fix nodedev mdev XML regression
See last patch for explanation. First two patches are related bugfixes/improvements v5: - Changed impl to match Boris' suggestion Cole Robinson (3): test: make parsed nodedevs active and persistent test: Sync GetXML INACTIVE behavior with live driver test: nodedev: fill active_config at driver startup time src/conf/node_device_conf.c | 24 ++++++++++++++++++++++++ src/conf/node_device_conf.h | 3 +++ src/libvirt_private.syms | 1 + src/test/test_driver.c | 20 +++++++++++++++++++- tests/nodedevxml2xmltest.c | 18 +++--------------- 5 files changed, 50 insertions(+), 16 deletions(-) -- 2.44.0
This was the implied default before nodedevs gained a notion of being inactive and transient. It also matches the implied default when parsing other object types Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/test/test_driver.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 41828f86b6..153ab7cdc2 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1269,6 +1269,8 @@ testParseNodedevs(testDriver *privconn, return -1; } + virNodeDeviceObjSetPersistent(obj, true); + virNodeDeviceObjSetActive(obj, true); virNodeDeviceObjSetSkipUpdateCaps(obj, true); virNodeDeviceObjEndAPI(&obj); } -- 2.44.0
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/23/24 15:44, Cole Robinson wrote:
This was the implied default before nodedevs gained a notion of being inactive and transient. It also matches the implied default when parsing other object types
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/test/test_driver.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 41828f86b6..153ab7cdc2 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1269,6 +1269,8 @@ testParseNodedevs(testDriver *privconn, return -1; }
+ virNodeDeviceObjSetPersistent(obj, true); + virNodeDeviceObjSetActive(obj, true); virNodeDeviceObjSetSkipUpdateCaps(obj, true); virNodeDeviceObjEndAPI(&obj); }
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
- Error if INACTIVE requested for transient object - Force dumping INACTIVE XML when object is inactive Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/test/test_driver.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 153ab7cdc2..e7d2b6c866 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -7514,15 +7514,30 @@ testNodeDeviceGetXMLDesc(virNodeDevicePtr dev, { testDriver *driver = dev->conn->privateData; virNodeDeviceObj *obj; + virNodeDeviceDef *def; char *ret = NULL; virCheckFlags(VIR_NODE_DEVICE_XML_INACTIVE, NULL); if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) return NULL; + def = virNodeDeviceObjGetDef(obj); - ret = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(obj), flags); + if (flags & VIR_NODE_DEVICE_XML_INACTIVE) { + if (!virNodeDeviceObjIsPersistent(obj)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("node device '%1$s' is not persistent"), + def->name); + goto cleanup; + } + } else { + if (!virNodeDeviceObjIsActive(obj)) + flags |= VIR_NODE_DEVICE_XML_INACTIVE; + } + ret = virNodeDeviceDefFormat(def, flags); + + cleanup: virNodeDeviceObjEndAPI(&obj); return ret; } -- 2.44.0
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/23/24 15:44, Cole Robinson wrote:
- Error if INACTIVE requested for transient object - Force dumping INACTIVE XML when object is inactive
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/test/test_driver.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 153ab7cdc2..e7d2b6c866 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -7514,15 +7514,30 @@ testNodeDeviceGetXMLDesc(virNodeDevicePtr dev, { testDriver *driver = dev->conn->privateData; virNodeDeviceObj *obj; + virNodeDeviceDef *def; char *ret = NULL;
virCheckFlags(VIR_NODE_DEVICE_XML_INACTIVE, NULL);
if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) return NULL; + def = virNodeDeviceObjGetDef(obj);
- ret = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(obj), flags); + if (flags & VIR_NODE_DEVICE_XML_INACTIVE) { + if (!virNodeDeviceObjIsPersistent(obj)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("node device '%1$s' is not persistent"), + def->name); + goto cleanup; + } + } else { + if (!virNodeDeviceObjIsActive(obj)) + flags |= VIR_NODE_DEVICE_XML_INACTIVE; + }
+ ret = virNodeDeviceDefFormat(def, flags); + + cleanup: virNodeDeviceObjEndAPI(&obj); return ret; }
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Commit v10.0.0-265-ge67bca23e4 added a `active_config` and `defined_config` to nodedev mdev internal XML handling. `defined_config` can be filled at XML parse time, but `active_config` must be filled in by nodedev driver. This wasn't implemented for the test driver however, which caused virt-manager test suite regressions. Working example: ``` $ virsh --connect test:///home/crobinso/src/virt-manager/tests/data/testdriver/testdriver.xml nodedev-dumpxml mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110 <device> <name>mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110</name> <path>/sys/devices/css0/0.0.0023/8e37ee90-2b51-45e3-9b25-bf8283c03110</path> <parent>css_0_0_0023</parent> <capability type='mdev'> <type id='vfio_ccw-io'/> <iommuGroup number='0'/> </capability> </device> ``` Broken example: ``` $ virsh --connect test:///home/crobinso/src/virt-manager/tests/data/testdriver/testdriver.xml nodedev-dumpxml mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110 <device> <name>mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110</name> <path>/sys/devices/css0/0.0.0023/8e37ee90-2b51-45e3-9b25-bf8283c03110</path> <parent>css_0_0_0023</parent> <capability type='mdev'> <iommuGroup number='0'/> </capability> </device> ``` There's already code that does what we want in the test suite. Move it to a shared function, and call it in test driver when creating a nodedev from driver startup XML. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/node_device_conf.c | 24 ++++++++++++++++++++++++ src/conf/node_device_conf.h | 3 +++ src/libvirt_private.syms | 1 + src/test/test_driver.c | 1 + tests/nodedevxml2xmltest.c | 18 +++--------------- 5 files changed, 32 insertions(+), 15 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 5cfbd6a7eb..fe6d9a36b2 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -2804,6 +2804,30 @@ virNodeDeviceCapsListExport(virNodeDeviceDef *def, return ncaps; } +void +virNodeDeviceSyncMdevActiveConfig(virNodeDeviceDef *def) +{ + size_t i; + virNodeDevCapsDef *caps; + + for (caps = def->caps; caps; caps = caps->next) { + virNodeDevCapData *data = &caps->data; + + if (caps->data.type != VIR_NODE_DEV_CAP_MDEV) + continue; + + data->mdev.active_config.type = g_strdup(data->mdev.defined_config.type); + for (i = 0; i < data->mdev.defined_config.nattributes; i++) { + g_autoptr(virMediatedDeviceAttr) attr = g_new0(virMediatedDeviceAttr, 1); + + attr->name = g_strdup(data->mdev.defined_config.attributes[i]->name); + attr->value = g_strdup(data->mdev.defined_config.attributes[i]->value); + VIR_APPEND_ELEMENT(data->mdev.active_config.attributes, + data->mdev.active_config.nattributes, + attr); + } + } +} #ifdef __linux__ diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index f0a5333881..4b82636af7 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -470,3 +470,6 @@ virNodeDeviceUpdateCaps(virNodeDeviceDef *def); int virNodeDeviceCapsListExport(virNodeDeviceDef *def, virNodeDevCapType **list); + +void +virNodeDeviceSyncMdevActiveConfig(virNodeDeviceDef *def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 839fe4f545..3186dd6d23 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -888,6 +888,7 @@ virNodeDeviceGetPCIDynamicCaps; virNodeDeviceGetSCSIHostCaps; virNodeDeviceGetSCSITargetCaps; virNodeDeviceGetWWNs; +virNodeDeviceSyncMdevActiveConfig; virNodeDeviceUpdateCaps; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index e7d2b6c866..d2d1bc43e3 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1272,6 +1272,7 @@ testParseNodedevs(testDriver *privconn, virNodeDeviceObjSetPersistent(obj, true); virNodeDeviceObjSetActive(obj, true); virNodeDeviceObjSetSkipUpdateCaps(obj, true); + virNodeDeviceSyncMdevActiveConfig(def); virNodeDeviceObjEndAPI(&obj); } diff --git a/tests/nodedevxml2xmltest.c b/tests/nodedevxml2xmltest.c index e918922672..814a817725 100644 --- a/tests/nodedevxml2xmltest.c +++ b/tests/nodedevxml2xmltest.c @@ -24,7 +24,6 @@ testCompareXMLToXMLFiles(const char *xml, const char *outfile, unsigned int flag int ret = -1; virNodeDeviceDef *dev = NULL; virNodeDevCapsDef *caps; - size_t i; if (virTestLoadFile(xml, &xmlData) < 0) goto fail; @@ -52,22 +51,11 @@ testCompareXMLToXMLFiles(const char *xml, const char *outfile, unsigned int flag data->storage.logical_block_size; } } - - if (caps->data.type == VIR_NODE_DEV_CAP_MDEV && - !(flags & VIR_NODE_DEVICE_XML_INACTIVE)) { - data->mdev.active_config.type = g_strdup(data->mdev.defined_config.type); - for (i = 0; i < data->mdev.defined_config.nattributes; i++) { - g_autoptr(virMediatedDeviceAttr) attr = g_new0(virMediatedDeviceAttr, 1); - - attr->name = g_strdup(data->mdev.defined_config.attributes[i]->name); - attr->value = g_strdup(data->mdev.defined_config.attributes[i]->value); - VIR_APPEND_ELEMENT(data->mdev.active_config.attributes, - data->mdev.active_config.nattributes, - attr); - } - } } + if (!(flags & VIR_NODE_DEVICE_XML_INACTIVE)) + virNodeDeviceSyncMdevActiveConfig(dev); + if (!(actual = virNodeDeviceDefFormat(dev, flags))) goto fail; -- 2.44.0
Cole, thanks for fixing the problem in test_driver caused by my changes. Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/23/24 15:44, Cole Robinson wrote:
Commit v10.0.0-265-ge67bca23e4 added a `active_config` and `defined_config` to nodedev mdev internal XML handling. `defined_config` can be filled at XML parse time, but `active_config` must be filled in by nodedev driver. This wasn't implemented for the test driver however, which caused virt-manager test suite regressions.
Working example:
``` $ virsh --connect test:///home/crobinso/src/virt-manager/tests/data/testdriver/testdriver.xml nodedev-dumpxml mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110 <device> <name>mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110</name> <path>/sys/devices/css0/0.0.0023/8e37ee90-2b51-45e3-9b25-bf8283c03110</path> <parent>css_0_0_0023</parent> <capability type='mdev'> <type id='vfio_ccw-io'/> <iommuGroup number='0'/> </capability> </device> ```
Broken example:
``` $ virsh --connect test:///home/crobinso/src/virt-manager/tests/data/testdriver/testdriver.xml nodedev-dumpxml mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110 <device> <name>mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110</name> <path>/sys/devices/css0/0.0.0023/8e37ee90-2b51-45e3-9b25-bf8283c03110</path> <parent>css_0_0_0023</parent> <capability type='mdev'> <iommuGroup number='0'/> </capability> </device> ```
There's already code that does what we want in the test suite. Move it to a shared function, and call it in test driver when creating a nodedev from driver startup XML.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/node_device_conf.c | 24 ++++++++++++++++++++++++ src/conf/node_device_conf.h | 3 +++ src/libvirt_private.syms | 1 + src/test/test_driver.c | 1 + tests/nodedevxml2xmltest.c | 18 +++--------------- 5 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 5cfbd6a7eb..fe6d9a36b2 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -2804,6 +2804,30 @@ virNodeDeviceCapsListExport(virNodeDeviceDef *def, return ncaps; }
+void +virNodeDeviceSyncMdevActiveConfig(virNodeDeviceDef *def) +{ + size_t i; + virNodeDevCapsDef *caps; + + for (caps = def->caps; caps; caps = caps->next) { + virNodeDevCapData *data = &caps->data; + + if (caps->data.type != VIR_NODE_DEV_CAP_MDEV) + continue; + + data->mdev.active_config.type = g_strdup(data->mdev.defined_config.type); + for (i = 0; i < data->mdev.defined_config.nattributes; i++) { + g_autoptr(virMediatedDeviceAttr) attr = g_new0(virMediatedDeviceAttr, 1); + + attr->name = g_strdup(data->mdev.defined_config.attributes[i]->name); + attr->value = g_strdup(data->mdev.defined_config.attributes[i]->value); + VIR_APPEND_ELEMENT(data->mdev.active_config.attributes, + data->mdev.active_config.nattributes, + attr); + } + } +}
#ifdef __linux__
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index f0a5333881..4b82636af7 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -470,3 +470,6 @@ virNodeDeviceUpdateCaps(virNodeDeviceDef *def); int virNodeDeviceCapsListExport(virNodeDeviceDef *def, virNodeDevCapType **list); + +void +virNodeDeviceSyncMdevActiveConfig(virNodeDeviceDef *def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 839fe4f545..3186dd6d23 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -888,6 +888,7 @@ virNodeDeviceGetPCIDynamicCaps; virNodeDeviceGetSCSIHostCaps; virNodeDeviceGetSCSITargetCaps; virNodeDeviceGetWWNs; +virNodeDeviceSyncMdevActiveConfig; virNodeDeviceUpdateCaps;
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index e7d2b6c866..d2d1bc43e3 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1272,6 +1272,7 @@ testParseNodedevs(testDriver *privconn, virNodeDeviceObjSetPersistent(obj, true); virNodeDeviceObjSetActive(obj, true); virNodeDeviceObjSetSkipUpdateCaps(obj, true); + virNodeDeviceSyncMdevActiveConfig(def); virNodeDeviceObjEndAPI(&obj); }
diff --git a/tests/nodedevxml2xmltest.c b/tests/nodedevxml2xmltest.c index e918922672..814a817725 100644 --- a/tests/nodedevxml2xmltest.c +++ b/tests/nodedevxml2xmltest.c @@ -24,7 +24,6 @@ testCompareXMLToXMLFiles(const char *xml, const char *outfile, unsigned int flag int ret = -1; virNodeDeviceDef *dev = NULL; virNodeDevCapsDef *caps; - size_t i;
if (virTestLoadFile(xml, &xmlData) < 0) goto fail; @@ -52,22 +51,11 @@ testCompareXMLToXMLFiles(const char *xml, const char *outfile, unsigned int flag data->storage.logical_block_size; } } - - if (caps->data.type == VIR_NODE_DEV_CAP_MDEV && - !(flags & VIR_NODE_DEVICE_XML_INACTIVE)) { - data->mdev.active_config.type = g_strdup(data->mdev.defined_config.type); - for (i = 0; i < data->mdev.defined_config.nattributes; i++) { - g_autoptr(virMediatedDeviceAttr) attr = g_new0(virMediatedDeviceAttr, 1); - - attr->name = g_strdup(data->mdev.defined_config.attributes[i]->name); - attr->value = g_strdup(data->mdev.defined_config.attributes[i]->value); - VIR_APPEND_ELEMENT(data->mdev.active_config.attributes, - data->mdev.active_config.nattributes, - attr); - } - } }
+ if (!(flags & VIR_NODE_DEVICE_XML_INACTIVE)) + virNodeDeviceSyncMdevActiveConfig(dev); + if (!(actual = virNodeDeviceDefFormat(dev, flags))) goto fail;
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
participants (2)
-
Boris Fiuczynski -
Cole Robinson