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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On 4/17/24 17:17, 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.
I still think that the mocking of state changes should be handled inside
of the test driver itself of the virNodeDeviceDriver in the
implementation the interfaces:
nodeDeviceCreateXML => creates a transient mdev from the XML (no
persistent config)
nodeDeviceDestroy => removes the active mdev (a transient mdev is
completely removed)
nodeDeviceDefineXML => creates a persistent mdev config from the XML
nodeDeviceUndefine => removes the persistent mdev config (if mdev is
active the active config remains)
nodeDeviceCreate => creates the active config from the persistent
config
Therefore for mocking
* copy defined_config to active_config
* reset defined_config
* reset active_config
should be sufficient.
Since there are only nodeDeviceCreateXML and nodeDeviceDestroy
implemented in the test driver the first two should do the trick.
>
> 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)))
--
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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On 4/19/24 8:38 AM, Boris Fiuczynski wrote: > On 4/17/24 17:17, 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. > > I still think that the mocking of state changes should be handled inside > of the test driver itself of the virNodeDeviceDriver in the > implementation the interfaces: > nodeDeviceCreateXML => creates a transient mdev from the XML (no > persistent config) > nodeDeviceDestroy => removes the active mdev (a transient mdev is > completely removed) > nodeDeviceDefineXML => creates a persistent mdev config from the XML > nodeDeviceUndefine => removes the persistent mdev config (if mdev is > active the active config remains) > nodeDeviceCreate => creates the active config from the persistent > config > > Therefore for mocking > * copy defined_config to active_config > * reset defined_config > * reset active_config > should be sufficient. > > Since there are only nodeDeviceCreateXML and nodeDeviceDestroy > implemented in the test driver the first two should do the trick. > OK, patches incoming which take this change out of the common parser. I did not fix the test driver API impls because they are unrelated to my goal of fixing the virt-manager test suite Thanks, Cole _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
© 2016 - 2026 Red Hat, Inc.