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