Re: [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