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

On Tue, May 10, 2016 at 07:15:01PM -0400, Cole Robinson wrote: > 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: Thinking about this some more last night, I realize that once we have the 'mapping' attribute, then the choice of values in the 'name' attribute becomes totally irrelevant. IOW, we could use your suggestion for giving each enum a unique name, eg 'spiceGL', 'vncGL', etc, etc. So apps which want to consume this data have a choice between just matching on explicit enum names and ignoring the <condition> rules, or they can match on the 'mapping' attribute and use the <condition> rules.. So we get the best of both ideas.