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
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
