Re: [PATCH v3 5/5] conf: nodedev: Fill active_config at XML parse time

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux