Re: [libvirt] [PATCH 3/5] domaincaps: Report graphics spiceGL

On 05/10/2016 11:50 AM, Daniel P. Berrange wrote: > On Tue, May 10, 2016 at 11:26:29AM -0400, Cole Robinson wrote: >> On 05/10/2016 05:10 AM, Daniel P. Berrange wrote: >>> On Mon, May 09, 2016 at 06:53:01PM -0400, Cole Robinson wrote: >>>> On 05/09/2016 09:48 AM, Daniel P. Berrange wrote: >>> >>> IMHO it is a total failure if we require the application to extend its >>> parser every time we add a new enum to the domain capabilities. We have >>> the ability to design something that is data driven - we should not build >>> something it is forced to be code driven with code changes for every >>> libvirt addition. >> >> This is ignoring the point I made previously that the schema is not already >> fully introspectable. Most of the current enums cannot be programmatically >> consumed anyways, because there's no way to map an enum name to its domain XML >> property. So if that's our goal to be data driven, we should address that >> issue first. > > Knowing the mapping of the capabilities enums to the domain schema is > a pre-requisite for consuming the capabilities data, no matter which > approach discussed in this thread is chosen. Assuming the app knows > that mapping, then the enum conditionals can be programmatically > handled with the approach I describe. > > Providing a way for the app to automatically determine the mapping > from capabilities enums to the domain schema would be a nice > addition, but it isn't a pre-requisite blocker for what's discussed > here. It is something that can be deal with in parallel. > >> I can't really think of a good way to represent that without nesting >> deeply or using specially formatted strings. Do you have a suggestion >> for that? > > Probably the best thing is to use a very simplified xpath style notation. > If we add a 'mapping' attribute to the <enum> that expresses the attribute > or element it is associated with, relative to the parent container element. > > eg, consider the tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml > data file with the mapping info: > > <domainCapabilities> > <path>/usr/bin/qemu-system-x86_64</path> > <domain>kvm</domain> > <machine>pc-1.2</machine> > <arch>x86_64</arch> > <os supported='yes'> > <loader supported='yes'> > <value>/usr/share/AAVMF/AAVMF_CODE.fd</value> > <value>/usr/share/OVMF/OVMF_CODE.fd</value> > <enum name='type' mapping="@type"> > <value>rom</value> > <value>pflash</value> > </enum> > <enum name='readonly' mapping="@readonly"> > <value>yes</value> > <value>no</value> > </enum> > </loader> > </os> > <devices> > <disk supported='yes'> > <enum name='diskDevice' mapping="@device"> > <value>disk</value> > <value>cdrom</value> > <value>floppy</value> > <value>lun</value> > </enum> > <enum name='bus' mapping="target/@bus"> > <value>ide</value> > <value>fdc</value> > <value>scsi</value> > <value>virtio</value> > <value>usb</value> > </enum> > </disk> > <hostdev supported='yes'> > <enum name='mode' mapping="@mode"> > <value>subsystem</value> > </enum> > <enum name='startupPolicy' mapping="source/@startupPolicy"> > <value>default</value> > <value>mandatory</value> > <value>requisite</value> > <value>optional</value> > </enum> > <enum name='subsysType' mapping="@type"> > <value>usb</value> > <value>pci</value> > <value>scsi</value> > </enum> > <enum name='capsType' mapping="@type"/> > <enum name='pciBackend' mapping="driver/@name"> > <value>default</value> > <value>kvm</value> > <value>vfio</value> > </enum> > </hostdev> > </devices> > <features> > <gic supported='no'/> > </features> > </domainCapabilities> > > > NB, with my proposed conditionals, the hostdev enums above would actually > want to be different. eg it would want to look like this: > > <enum name='mode' mapping="@mode"> > <value>subsystem</value> > </enum> > <enum name='startupPolicy' mapping="source/@startupPolicy"> > <value>default</value> > <value>mandatory</value> > <value>requisite</value> > <value>optional</value> > </enum> > <enum name='type' mapping="@type"> > <condition> > <enum name='mode' value='subsystem'/> > </condition> > <value>usb</value> > <value>pci</value> > <value>scsi</value> > </enum> > <enum name='driver' mapping="driver/@name"> > <condition> > <enum name='mode' value='subsystem'/> > <enum name='type' value='pci'/> > </condition> > <value>default</value> > <value>kvm</value> > <value>vfio</value> > </enum> > Yeah that mapping= bit makes sense to me. I don't have time to see pick this up now though, so I've stuffed it in a bug: