9:56 a.m.
The virt-manager test suite is busted with libvirt 10.1.0+ after
this commit:
commit e67bca23e4fe38a3491749f724b9edf743d0e916
Author: Boris Fiuczynski <fiuczy(a)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
9:56 a.m.
New subject: [PATCH v3 1/5] test: Fix `virsh nodedev-list`
$ 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(a)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
10:15 a.m.
New subject: [PATCH v3 1/5] test: Fix `virsh nodedev-list`
Reviewed-by: Boris Fiuczynski <fiuczy(a)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(a)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
9:56 a.m.
New subject: [PATCH v3 2/5] test: Implement virNodeDeviceIsActive
Signed-off-by: Cole Robinson <crobinso(a)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
10:16 a.m.
New subject: [PATCH v3 2/5] test: Implement virNodeDeviceIsActive
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
On 4/9/24 16:56, Cole Robinson wrote:
Signed-off-by: Cole Robinson <crobinso(a)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
9:56 a.m.
New subject: [PATCH v3 3/5] test: Implement virNodeDeviceIsPersistent
Signed-off-by: Cole Robinson <crobinso(a)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
10:17 a.m.
New subject: [PATCH v3 3/5] test: Implement virNodeDeviceIsPersistent
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
On 4/9/24 16:56, Cole Robinson wrote:
Signed-off-by: Cole Robinson <crobinso(a)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
9:56 a.m.
New subject: [PATCH v3 4/5] test: make nodedevs active by default
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(a)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
10:19 a.m.
New subject: [PATCH v3 4/5] test: make nodedevs active by default
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(a)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
9:04 a.m.
New subject: [PATCH v3 4/5] test: make nodedevs active by default
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(a)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
9:46 a.m.
New subject: [PATCH v3 4/5] test: make nodedevs active by default
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(a)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
9:56 a.m.
New subject: [PATCH v3 5/5] 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
10:20 a.m.
New subject: [PATCH v3 5/5] conf: nodedev: Fill active_config at XML parse
time
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(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:58 a.m.
New subject: [PATCH v3 5/5] conf: nodedev: Fill active_config at XML parse
time
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
9:12 a.m.
New subject: [PATCH v3 5/5] conf: nodedev: Fill active_config at XML parse
time
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 :|
8:52 a.m.
New subject: [PATCH v3 5/5] conf: nodedev: Fill active_config at XML parse
time
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
9:03 a.m.
New subject: [PATCH v3 5/5] conf: nodedev: Fill active_config at XML parse
time
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
9:21 a.m.
New subject: [PATCH v3 5/5] conf: nodedev: Fill active_config at XML parse
time
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 :|
4:11 a.m.
New subject: [PATCH v3 5/5] conf: nodedev: Fill active_config at XML parse
time
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
8:59 a.m.
New subject: [PATCH v3 5/5] conf: nodedev: Fill active_config at XML parse
time
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
10:09 a.m.
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(a)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(a)redhat.com>
But before pushing, also please add Boris' R-B as he also reviewed these
patches.
Michal
10:12 a.m.
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(a)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(a)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
218
days inactive
227
days old
21 comments
5 participants
participants (5)
-
Boris Fiuczynski -
Cole Robinson -
Daniel P. Berrangé -
Jonathon Jongsma -
Michal Prívozník