10:17 a.m.
See last patch for explanation. First two patches are related
bugfixes/improvements
v4 changes:
- 3 patches pushed
- mark parsed devices as persistent too
- add GetXML fix patch
Cole Robinson (3):
test: make parsed nodedevs active and persistent
test: Sync GetXML INACTIVE behavior with live driver
conf: nodedev: Fill active_config at XML parse time
src/conf/node_device_conf.c | 5 ++++-
src/test/test_driver.c | 19 ++++++++++++++++++-
tests/nodedevxml2xmltest.c | 15 ---------------
3 files changed, 22 insertions(+), 17 deletions(-)
--
2.44.0
10:17 a.m.
New subject: [PATCH v4 1/3] test: make parsed nodedevs active and persistent
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(a)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
10:17 a.m.
New subject: [PATCH v4 2/3] test: Sync GetXML INACTIVE behavior with live driver
- Error if INACTIVE requested for transient object
- Force dumping INACTIVE XML when object is inactive
Signed-off-by: Cole Robinson <crobinso(a)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
10:17 a.m.
New subject: [PATCH v4 3/3] conf: nodedev: Fill active_config at XML parse time
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(a)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
7:38 a.m.
New subject: [PATCH v4 3/3] conf: nodedev: Fill active_config at XML parse
time
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(a)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
8:44 a.m.
New subject: [PATCH v4 3/3] conf: nodedev: Fill active_config at XML parse
time
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
214
days inactive
220
days old
5 comments
2 participants
participants (2)
-
Boris Fiuczynski -
Cole Robinson