[PATCH v3 0/5] test: fix nodedev mdev XML regression
The virt-manager test suite is busted with libvirt 10.1.0+ after this commit: commit e67bca23e4fe38a3491749f724b9edf743d0e916 Author: Boris Fiuczynski <fiuczy@linux.ibm.com> Date: Thu Feb 22 14:02:01 2024 +0100 nodedev: add an active config to mdev See patch #5 for the full explanation. First 4 patches are nodedev test driver improvements I hit when debugging this v3: really truly send to the correct list v2: Send to the correct mailing list Fix version strings in test driver table Cole Robinson (5): test: Fix `virsh nodedev-list` test: Implement virNodeDeviceIsActive test: Implement virNodeDeviceIsPersistent test: make nodedevs active by default conf: nodedev: Fill active_config at XML parse time src/conf/node_device_conf.c | 5 ++++- src/test/test_driver.c | 35 ++++++++++++++++++++++++++++++++++- tests/nodedevxml2xmltest.c | 15 --------------- 3 files changed, 38 insertions(+), 17 deletions(-) -- 2.44.0
$ virsh --connect test:///default nodedev-list error: Failed to list node devices error: unsupported flags (0x80000000) in function testConnectListAllNodeDevices The test driver handles the nodedev state flags, we just need to allow them Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/test/test_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ed0cdc0dab..852a084bce 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -7481,7 +7481,7 @@ testConnectListAllNodeDevices(virConnectPtr conn, { testDriver *driver = conn->privateData; - virCheckFlags(VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP, -1); + virCheckFlags(VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_ALL, -1); return virNodeDeviceObjListExport(conn, driver->devs, devices, NULL, flags); -- 2.44.0
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/9/24 16:56, Cole Robinson wrote:
$ virsh --connect test:///default nodedev-list error: Failed to list node devices error: unsupported flags (0x80000000) in function testConnectListAllNodeDevices
The test driver handles the nodedev state flags, we just need to allow them
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/test/test_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ed0cdc0dab..852a084bce 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -7481,7 +7481,7 @@ testConnectListAllNodeDevices(virConnectPtr conn, { testDriver *driver = conn->privateData;
- virCheckFlags(VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP, -1); + virCheckFlags(VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_ALL, -1);
return virNodeDeviceObjListExport(conn, driver->devs, devices, NULL, flags);
-- 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
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/test/test_driver.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 852a084bce..f9bd6f4e67 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -7779,6 +7779,21 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) return ret; } +static int +testNodeDeviceIsActive(virNodeDevicePtr dev) +{ + testDriver *privconn = dev->conn->privateData; + virNodeDeviceObj *obj = NULL; + int ret = -1; + + if (!(obj = testNodeDeviceObjFindByName(privconn, dev->name))) + return -1; + + ret = virNodeDeviceObjIsActive(obj); + virNodeDeviceObjEndAPI(&obj); + return ret; +} + /* Domain event implementations */ static int @@ -10657,6 +10672,7 @@ static virNodeDeviceDriver testNodeDeviceDriver = { .nodeDeviceListCaps = testNodeDeviceListCaps, /* 0.7.2 */ .nodeDeviceCreateXML = testNodeDeviceCreateXML, /* 0.7.3 */ .nodeDeviceDestroy = testNodeDeviceDestroy, /* 0.7.3 */ + .nodeDeviceIsActive = testNodeDeviceIsActive, /* 10.3.0 */ }; static virConnectDriver testConnectDriver = { -- 2.44.0
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/9/24 16:56, Cole Robinson wrote:
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/test/test_driver.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 852a084bce..f9bd6f4e67 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -7779,6 +7779,21 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) return ret; }
+static int +testNodeDeviceIsActive(virNodeDevicePtr dev) +{ + testDriver *privconn = dev->conn->privateData; + virNodeDeviceObj *obj = NULL; + int ret = -1; + + if (!(obj = testNodeDeviceObjFindByName(privconn, dev->name))) + return -1; + + ret = virNodeDeviceObjIsActive(obj); + virNodeDeviceObjEndAPI(&obj); + return ret; +} +
/* Domain event implementations */ static int @@ -10657,6 +10672,7 @@ static virNodeDeviceDriver testNodeDeviceDriver = { .nodeDeviceListCaps = testNodeDeviceListCaps, /* 0.7.2 */ .nodeDeviceCreateXML = testNodeDeviceCreateXML, /* 0.7.3 */ .nodeDeviceDestroy = testNodeDeviceDestroy, /* 0.7.3 */ + .nodeDeviceIsActive = testNodeDeviceIsActive, /* 10.3.0 */ };
static virConnectDriver testConnectDriver = {
-- 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
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/test/test_driver.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index f9bd6f4e67..41828f86b6 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -7794,6 +7794,21 @@ testNodeDeviceIsActive(virNodeDevicePtr dev) return ret; } +static int +testNodeDeviceIsPersistent(virNodeDevicePtr dev) +{ + testDriver *privconn = dev->conn->privateData; + virNodeDeviceObj *obj = NULL; + int ret = -1; + + if (!(obj = testNodeDeviceObjFindByName(privconn, dev->name))) + return -1; + + ret = virNodeDeviceObjIsPersistent(obj); + virNodeDeviceObjEndAPI(&obj); + return ret; +} + /* Domain event implementations */ static int @@ -10673,6 +10688,7 @@ static virNodeDeviceDriver testNodeDeviceDriver = { .nodeDeviceCreateXML = testNodeDeviceCreateXML, /* 0.7.3 */ .nodeDeviceDestroy = testNodeDeviceDestroy, /* 0.7.3 */ .nodeDeviceIsActive = testNodeDeviceIsActive, /* 10.3.0 */ + .nodeDeviceIsPersistent = testNodeDeviceIsPersistent, /* 10.3.0 */ }; static virConnectDriver testConnectDriver = { -- 2.44.0
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/9/24 16:56, Cole Robinson wrote:
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/test/test_driver.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index f9bd6f4e67..41828f86b6 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -7794,6 +7794,21 @@ testNodeDeviceIsActive(virNodeDevicePtr dev) return ret; }
+static int +testNodeDeviceIsPersistent(virNodeDevicePtr dev) +{ + testDriver *privconn = dev->conn->privateData; + virNodeDeviceObj *obj = NULL; + int ret = -1; + + if (!(obj = testNodeDeviceObjFindByName(privconn, dev->name))) + return -1; + + ret = virNodeDeviceObjIsPersistent(obj); + virNodeDeviceObjEndAPI(&obj); + return ret; +} +
/* Domain event implementations */ static int @@ -10673,6 +10688,7 @@ static virNodeDeviceDriver testNodeDeviceDriver = { .nodeDeviceCreateXML = testNodeDeviceCreateXML, /* 0.7.3 */ .nodeDeviceDestroy = testNodeDeviceDestroy, /* 0.7.3 */ .nodeDeviceIsActive = testNodeDeviceIsActive, /* 10.3.0 */ + .nodeDeviceIsPersistent = testNodeDeviceIsPersistent, /* 10.3.0 */ };
static virConnectDriver testConnectDriver = {
-- 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
This was the implied default before nodedevs gained a notion of being inactive, and matches how we handle parsing other objects Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/test/test_driver.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 41828f86b6..9db7a44035 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1269,6 +1269,7 @@ testParseNodedevs(testDriver *privconn, return -1; } + virNodeDeviceObjSetActive(obj, true); virNodeDeviceObjSetSkipUpdateCaps(obj, true); virNodeDeviceObjEndAPI(&obj); } -- 2.44.0
On 4/9/24 16:56, Cole Robinson wrote:
This was the implied default before nodedevs gained a notion of being inactive, and matches how we handle parsing other objects
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/test/test_driver.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 41828f86b6..9db7a44035 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1269,6 +1269,7 @@ testParseNodedevs(testDriver *privconn, return -1; }
+ virNodeDeviceObjSetActive(obj, true);
This will actually render the mdev object to be transient which is an active mdev not having a persistent definition. The data using virNodeDeviceDefParseXML is stored in the mdevs defined_config only therefore the data is showing up when you use "virsh nodedev-dumpxml" as it defaults to the "current state" of the nodedev. For data consistency of the node you should instead do virNodeDeviceObjSetPersistent(obj, true); Now the mdev is inactive and persistent and the virsh command should be correct. I guess this would also resolve the requirement for the next patch unless you have a requirement to mock dumping transient mdevs.
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
On 4/9/24 11:19 AM, Boris Fiuczynski wrote:
On 4/9/24 16:56, Cole Robinson wrote:
This was the implied default before nodedevs gained a notion of being inactive, and matches how we handle parsing other objects
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/test/test_driver.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 41828f86b6..9db7a44035 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1269,6 +1269,7 @@ testParseNodedevs(testDriver *privconn, return -1; } + virNodeDeviceObjSetActive(obj, true);
This will actually render the mdev object to be transient which is an active mdev not having a persistent definition. The data using virNodeDeviceDefParseXML is stored in the mdevs defined_config only therefore the data is showing up when you use "virsh nodedev-dumpxml" as it defaults to the "current state" of the nodedev.
For data consistency of the node you should instead do virNodeDeviceObjSetPersistent(obj, true);
Now the mdev is inactive and persistent and the virsh command should be correct.
I guess this would also resolve the requirement for the next patch unless you have a requirement to mock dumping transient mdevs.
Oops I replied to patch 5 before seeing this one. Let me explore this a bit. Thanks, Cole
On 4/17/24 10:04 AM, Cole Robinson wrote:
On 4/9/24 11:19 AM, Boris Fiuczynski wrote:
On 4/9/24 16:56, Cole Robinson wrote:
This was the implied default before nodedevs gained a notion of being inactive, and matches how we handle parsing other objects
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/test/test_driver.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 41828f86b6..9db7a44035 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1269,6 +1269,7 @@ testParseNodedevs(testDriver *privconn, return -1; } + virNodeDeviceObjSetActive(obj, true);
This will actually render the mdev object to be transient which is an active mdev not having a persistent definition. The data using virNodeDeviceDefParseXML is stored in the mdevs defined_config only therefore the data is showing up when you use "virsh nodedev-dumpxml" as it defaults to the "current state" of the nodedev.
For data consistency of the node you should instead do virNodeDeviceObjSetPersistent(obj, true);
Good catch that this does not make the devices persistent, I missed that detail. Ok so there's a bit more context needed here. For test driver XML parsing, all other objects are considered persistent by default. Transient has to be opted in via testdriver specific XML namespace. So yeah makes sense for consistency to make object persistent as well. But same logic applies for making the object 'active' by default. test driver assumes any read in domain/interface/network/storage object is default active and persistent. Makes sense to do the same for nodedev IMO. Notable we need this so `virsh nodedev-list` works in the way it historically did, before nodedev devices could be inactive.
Now the mdev is inactive and persistent and the virsh command should be correct.
virt-manager/virt-install isn't using virsh, we are using direct API via python bindings. We are calling virNodeDeviceGetXMLDesc with flags=0, the only historically supported value. But even still, this doesn't fix `virsh nodedev-dumpxml $mdev` for test driver, because GetXMLDesc test driver implementation doesn't force INACTIVE XML flag when the object is inactive, like the real node_device_driver.c does. So flags=0 tries to dump 'active' XML of an inactive object. So that's another bit to fix. But still IMO that is not sufficient, since nodedevs parsed via test driver input XML should be considered active by default. I will push patch 1-3 which you and michal reviewed. I will send another version of this series with the 'persistent' bit fixed, and an additional patch to fix nodedev GetXML semantics to match node_device driver. But that still leaves something like patch 5 as a requirement. Thanks, Cole
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. Example before: ``` $ 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> ``` Example after: ``` $ 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> ``` Simplest solution is to fill in `active_config` at XML define time as well. The real node_device driver already takes care to free any `active_config` when it live updates this info, so we are safe there. This also lets us drop the test suite logic to duplicate this data. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/node_device_conf.c | 5 ++++- tests/nodedevxml2xmltest.c | 15 --------------- 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 5cfbd6a7eb..f381ea128c 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -2222,6 +2222,7 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt, _("missing type id attribute for '%1$s'"), def->name); return -1; } + mdev->active_config.type = g_strdup(mdev->defined_config.type); if ((uuidstr = virXPathString("string(./uuid[1])", ctxt))) { unsigned char uuidbuf[VIR_UUID_BUFLEN]; @@ -2248,8 +2249,10 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt, if ((nattrs = virXPathNodeSet("./attr", ctxt, &attrs)) < 0) return -1; - for (i = 0; i < nattrs; i++) + for (i = 0; i < nattrs; i++) { virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i], &mdev->defined_config); + virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i], &mdev->active_config); + } return 0; } diff --git a/tests/nodedevxml2xmltest.c b/tests/nodedevxml2xmltest.c index e918922672..d2663a8d68 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,20 +51,6 @@ 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 (!(actual = virNodeDeviceDefFormat(dev, flags))) -- 2.44.0
On 4/9/24 16:56, 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.
Example before:
``` $ 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> ```
Example after:
``` $ 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> ```
Simplest solution is to fill in `active_config` at XML define time as well. The real node_device driver already takes care to free any `active_config` when it live updates this info, so we are safe there.
I do not think that it is a good idea to hack the general code which creates in the real environments fake data. If your tests require active mdevs than the mocking code should set these active and also do the config copy.
This also lets us drop the test suite logic to duplicate this data.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/node_device_conf.c | 5 ++++- tests/nodedevxml2xmltest.c | 15 --------------- 2 files changed, 4 insertions(+), 16 deletions(-)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 5cfbd6a7eb..f381ea128c 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -2222,6 +2222,7 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt, _("missing type id attribute for '%1$s'"), def->name); return -1; } + mdev->active_config.type = g_strdup(mdev->defined_config.type);
if ((uuidstr = virXPathString("string(./uuid[1])", ctxt))) { unsigned char uuidbuf[VIR_UUID_BUFLEN]; @@ -2248,8 +2249,10 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt, if ((nattrs = virXPathNodeSet("./attr", ctxt, &attrs)) < 0) return -1;
- for (i = 0; i < nattrs; i++) + for (i = 0; i < nattrs; i++) { virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i], &mdev->defined_config); + virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i], &mdev->active_config); + }
return 0; } diff --git a/tests/nodedevxml2xmltest.c b/tests/nodedevxml2xmltest.c index e918922672..d2663a8d68 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,20 +51,6 @@ 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 (!(actual = virNodeDeviceDefFormat(dev, flags)))
-- 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
On 4/9/24 11:20 AM, Boris Fiuczynski wrote:
On 4/9/24 16:56, 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.
Example before:
``` $ 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> ```
Example after:
``` $ 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> ```
Simplest solution is to fill in `active_config` at XML define time as well. The real node_device driver already takes care to free any `active_config` when it live updates this info, so we are safe there.
I do not think that it is a good idea to hack the general code which creates in the real environments fake data.
I can't tell how this affects real environments. Is there a place in the udev driver where XML is parsed, and then active_config isn't explicitly updated? If not, do you object to me pushing this with Michal's ACK? This is causing some pain with virt-manager dev workflow
If your tests require active mdevs than the mocking code should set these active and also do the config copy.
Obviously this can work but IMO it sets a bad precedent. If this active/inactive_config structure is extended in the future, the test driver syncing needs to be extended to match, or we need more plumbing to share more of this Personally I would have rather seen this implemented using ->newDef pattern used by domain, network, storage. That's the closest thing to an internal standard for handling differences between inactive config and runtime config. ->def is copied to ->newDef at object startup time, and the drivers change newDef as needed to reflect runtime state of the object. Thanks, Cole
On Wed, Apr 17, 2024 at 09:58:10AM -0400, Cole Robinson wrote:
On 4/9/24 11:20 AM, Boris Fiuczynski wrote:
On 4/9/24 16:56, 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.
Example before:
``` $ 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> ```
Example after:
``` $ 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> ```
Simplest solution is to fill in `active_config` at XML define time as well. The real node_device driver already takes care to free any `active_config` when it live updates this info, so we are safe there.
I do not think that it is a good idea to hack the general code which creates in the real environments fake data.
I can't tell how this affects real environments. Is there a place in the udev driver where XML is parsed, and then active_config isn't explicitly updated?
If not, do you object to me pushing this with Michal's ACK? This is causing some pain with virt-manager dev workflow
If your tests require active mdevs than the mocking code should set these active and also do the config copy.
Obviously this can work but IMO it sets a bad precedent. If this active/inactive_config structure is extended in the future, the test driver syncing needs to be extended to match, or we need more plumbing to share more of this
Personally I would have rather seen this implemented using ->newDef pattern used by domain, network, storage. That's the closest thing to an internal standard for handling differences between inactive config and runtime config. ->def is copied to ->newDef at object startup time, and the drivers change newDef as needed to reflect runtime state of the object.
Yes, if we need to store both active and inactive config information then I would strongly prefer to have the normal def+newDef pattern, so we can expose this difference in the APIs with an "INACTIVE" flag and --inactive virsh opt. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 4/17/24 10:12 AM, Daniel P. Berrangé wrote:
On Wed, Apr 17, 2024 at 09:58:10AM -0400, Cole Robinson wrote:
On 4/9/24 11:20 AM, Boris Fiuczynski wrote:
On 4/9/24 16:56, 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.
Example before:
``` $ 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> ```
Example after:
``` $ 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> ```
Simplest solution is to fill in `active_config` at XML define time as well. The real node_device driver already takes care to free any `active_config` when it live updates this info, so we are safe there.
I do not think that it is a good idea to hack the general code which creates in the real environments fake data.
I can't tell how this affects real environments. Is there a place in the udev driver where XML is parsed, and then active_config isn't explicitly updated?
If not, do you object to me pushing this with Michal's ACK? This is causing some pain with virt-manager dev workflow
If your tests require active mdevs than the mocking code should set these active and also do the config copy.
Obviously this can work but IMO it sets a bad precedent. If this active/inactive_config structure is extended in the future, the test driver syncing needs to be extended to match, or we need more plumbing to share more of this
Personally I would have rather seen this implemented using ->newDef pattern used by domain, network, storage. That's the closest thing to an internal standard for handling differences between inactive config and runtime config. ->def is copied to ->newDef at object startup time, and the drivers change newDef as needed to reflect runtime state of the object.
Yes, if we need to store both active and inactive config information then I would strongly prefer to have the normal def+newDef pattern, so we can expose this difference in the APIs with an "INACTIVE" flag and --inactive virsh opt.
That is already implemented in git, by Boris. But it's implemented without the newDef pattern - Cole
On 4/18/24 8:52 AM, Cole Robinson wrote:
On 4/17/24 10:12 AM, Daniel P. Berrangé wrote:
On Wed, Apr 17, 2024 at 09:58:10AM -0400, Cole Robinson wrote:
On 4/9/24 11:20 AM, Boris Fiuczynski wrote:
On 4/9/24 16:56, 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.
Example before:
``` $ 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> ```
Example after:
``` $ 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> ```
Simplest solution is to fill in `active_config` at XML define time as well. The real node_device driver already takes care to free any `active_config` when it live updates this info, so we are safe there.
I do not think that it is a good idea to hack the general code which creates in the real environments fake data.
I can't tell how this affects real environments. Is there a place in the udev driver where XML is parsed, and then active_config isn't explicitly updated?
If not, do you object to me pushing this with Michal's ACK? This is causing some pain with virt-manager dev workflow
If your tests require active mdevs than the mocking code should set these active and also do the config copy.
Obviously this can work but IMO it sets a bad precedent. If this active/inactive_config structure is extended in the future, the test driver syncing needs to be extended to match, or we need more plumbing to share more of this
Personally I would have rather seen this implemented using ->newDef pattern used by domain, network, storage. That's the closest thing to an internal standard for handling differences between inactive config and runtime config. ->def is copied to ->newDef at object startup time, and the drivers change newDef as needed to reflect runtime state of the object.
Yes, if we need to store both active and inactive config information then I would strongly prefer to have the normal def+newDef pattern, so we can expose this difference in the APIs with an "INACTIVE" flag and --inactive virsh opt.
That is already implemented in git, by Boris. But it's implemented without the newDef pattern
- Cole
I can try to adapt the current implementation to use the def/newDef pattern. I didn't think to suggest this when the patch was originally submitted. Jonathon
On Thu, Apr 18, 2024 at 09:03:45AM -0500, Jonathon Jongsma wrote:
On 4/18/24 8:52 AM, Cole Robinson wrote:
On 4/17/24 10:12 AM, Daniel P. Berrangé wrote:
On Wed, Apr 17, 2024 at 09:58:10AM -0400, Cole Robinson wrote:
On 4/9/24 11:20 AM, Boris Fiuczynski wrote:
On 4/9/24 16:56, 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.
Example before:
``` $ 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> ```
Example after:
``` $ 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> ```
Simplest solution is to fill in `active_config` at XML define time as well. The real node_device driver already takes care to free any `active_config` when it live updates this info, so we are safe there.
I do not think that it is a good idea to hack the general code which creates in the real environments fake data.
I can't tell how this affects real environments. Is there a place in the udev driver where XML is parsed, and then active_config isn't explicitly updated?
If not, do you object to me pushing this with Michal's ACK? This is causing some pain with virt-manager dev workflow
If your tests require active mdevs than the mocking code should set these active and also do the config copy.
Obviously this can work but IMO it sets a bad precedent. If this active/inactive_config structure is extended in the future, the test driver syncing needs to be extended to match, or we need more plumbing to share more of this
Personally I would have rather seen this implemented using ->newDef pattern used by domain, network, storage. That's the closest thing to an internal standard for handling differences between inactive config and runtime config. ->def is copied to ->newDef at object startup time, and the drivers change newDef as needed to reflect runtime state of the object.
Yes, if we need to store both active and inactive config information then I would strongly prefer to have the normal def+newDef pattern, so we can expose this difference in the APIs with an "INACTIVE" flag and --inactive virsh opt.
That is already implemented in git, by Boris. But it's implemented without the newDef pattern
- Cole
I can try to adapt the current implementation to use the def/newDef pattern. I didn't think to suggest this when the patch was originally submitted.
I think that'd be a nice cleanup if you have time to work on it. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 4/17/24 15:58, Cole Robinson wrote:
Simplest solution is to fill in `active_config` at XML define time as well. The real node_device driver already takes care to free any `active_config` when it live updates this info, so we are safe there. I do not think that it is a good idea to hack the general code which creates in the real environments fake data.
I can't tell how this affects real environments. Is there a place in the udev driver where XML is parsed, and then active_config isn't explicitly updated?
If not, do you object to me pushing this with Michal's ACK? This is causing some pain with virt-manager dev workflow
virNodeDevCapMdevParseXML is used when defining a new mdev and starting a transient mdev but of course not in the udev driver. There updates start mdevctl and parse the returned json to populate the data into the mdev. -- 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
On 4/18/24 5:11 AM, Boris Fiuczynski wrote:
On 4/17/24 15:58, Cole Robinson wrote:
Simplest solution is to fill in `active_config` at XML define time as well. The real node_device driver already takes care to free any `active_config` when it live updates this info, so we are safe there. I do not think that it is a good idea to hack the general code which creates in the real environments fake data.
I can't tell how this affects real environments. Is there a place in the udev driver where XML is parsed, and then active_config isn't explicitly updated?
If not, do you object to me pushing this with Michal's ACK? This is causing some pain with virt-manager dev workflow
virNodeDevCapMdevParseXML is used when defining a new mdev and starting a transient mdev but of course not in the udev driver. There updates start mdevctl and parse the returned json to populate the data into the mdev.
I see, but I still don't see how this change causes problems with real environments (the udev driver) - Cole
On 4/9/24 16:56, Cole Robinson wrote:
The virt-manager test suite is busted with libvirt 10.1.0+ after this commit:
commit e67bca23e4fe38a3491749f724b9edf743d0e916 Author: Boris Fiuczynski <fiuczy@linux.ibm.com> Date: Thu Feb 22 14:02:01 2024 +0100
nodedev: add an active config to mdev
See patch #5 for the full explanation. First 4 patches are nodedev test driver improvements I hit when debugging this
v3: really truly send to the correct list v2: Send to the correct mailing list Fix version strings in test driver table
Cole Robinson (5): test: Fix `virsh nodedev-list` test: Implement virNodeDeviceIsActive test: Implement virNodeDeviceIsPersistent test: make nodedevs active by default conf: nodedev: Fill active_config at XML parse time
src/conf/node_device_conf.c | 5 ++++- src/test/test_driver.c | 35 ++++++++++++++++++++++++++++++++++- tests/nodedevxml2xmltest.c | 15 --------------- 3 files changed, 38 insertions(+), 17 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> But before pushing, also please add Boris' R-B as he also reviewed these patches. Michal
On 4/9/24 17:09, Michal Prívozník wrote:
On 4/9/24 16:56, Cole Robinson wrote:
The virt-manager test suite is busted with libvirt 10.1.0+ after this commit:
commit e67bca23e4fe38a3491749f724b9edf743d0e916 Author: Boris Fiuczynski <fiuczy@linux.ibm.com> Date: Thu Feb 22 14:02:01 2024 +0100
nodedev: add an active config to mdev
See patch #5 for the full explanation. First 4 patches are nodedev test driver improvements I hit when debugging this
v3: really truly send to the correct list v2: Send to the correct mailing list Fix version strings in test driver table
Cole Robinson (5): test: Fix `virsh nodedev-list` test: Implement virNodeDeviceIsActive test: Implement virNodeDeviceIsPersistent test: make nodedevs active by default conf: nodedev: Fill active_config at XML parse time
src/conf/node_device_conf.c | 5 ++++- src/test/test_driver.c | 35 ++++++++++++++++++++++++++++++++++- tests/nodedevxml2xmltest.c | 15 --------------- 3 files changed, 38 insertions(+), 17 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
But before pushing, also please add Boris' R-B as he also reviewed these patches.
Michal
I agree and also gave my R-B to patches 1 to 3 with the same remarks as Michael on patch 2 and 3. I disagree with patches 4 and 5! -- 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 (5)
-
Boris Fiuczynski -
Cole Robinson -
Daniel P. Berrangé -
Jonathon Jongsma -
Michal Prívozník