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

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

 



On 05/09/2016 09:48 AM, Daniel P. Berrange wrote:
> On Mon, May 09, 2016 at 09:30:30AM -0400, Cole Robinson wrote:
>> On 05/09/2016 07:30 AM, Pavel Hrdina wrote:
>>> On Sun, May 08, 2016 at 01:49:06PM -0400, Cole Robinson wrote:
>>>> Reports a tristate enum value for acceptable graphics type=spice
>>>> <gl enable=XXX/>.
>>>>
>>>> Wire it up for qemu too. 'no' is always a valid value, so we
>>>> unconditionally report it.
>>>> ---
>>>
>>> [...]
>>>
>>>> diff --git a/tests/domaincapsschemadata/domaincaps-full.xml b/tests/domaincapsschemadata/domaincaps-full.xml
>>>> index 2f529ff..4eb5637 100644
>>>> --- a/tests/domaincapsschemadata/domaincaps-full.xml
>>>> +++ b/tests/domaincapsschemadata/domaincaps-full.xml
>>>> @@ -47,6 +47,11 @@
>>>>          <value>desktop</value>
>>>>          <value>spice</value>
>>>>        </enum>
>>>> +      <enum name='spiceGL'>
>>>> +        <value>default</value>
>>>> +        <value>yes</value>
>>>> +        <value>no</value>
>>>> +      </enum>
>>>
>>> Ewww, this doesn't look good and I agree with Michal that what if we add
>>> support for another graphics device and what if one graphics device will have
>>> more than one capability?
>>>
>>> I would suggest something like this:
>>>
>>>     <graphics supported='yes'>
>>>       <enum name='type'>
>>>         <value>spice</value>
>>>         <value>vnc</value>
>>>         <value>sdl</value>
>>>       </enum>
>>>       <type name='spice'>
>>>         <gl supported='yes'/>
>>>       </type>
>>>     </graphics>
>>>
>>> or even do something like this:
>>>
>>>     <graphics supported='yes'>
>>>       <type name='spice'>
>>>         <gl supported='yes'/>
>>>       </type>
>>>       <type name='vnc'/>
>>>       <type name='sdl'/>
>>>     </graphics>
>>>
>>
>> Really wish someone would have chimed in with this to my (unresponded) RFC
>> email about these things...
>>
>> http://www.redhat.com/archives/libvir-list/2016-April/msg01839.html
>>
>> That pattern is fine in this case, but what about the case of ports= element
>> that is only supported with a controller type=usb model=nec-xhci ? Do we have
>> new XML like:
>>
>> <controller supported='yes'>
>>   <type name='usb'>
>>     <model name='nec-xhci'/>
>>       <param name='ports' supported='yes'/>
>>     </model>
>>   </type>
>> </controller>
>>
>> It seems like these types of relationships could grow deeply nested, so I
>> opted for following the limited existing convention of adding unique enum
>> names to represent the relationship.
> 
> Yeah, I find flat modelling quite desirable, because the relationships
> between attributes will certainly grow quite complicate, and they do
> not neccessarily form a nice simple tree relationship.
> 
> ie, whether  foo=bar is permitted may depend on whether wizz=eek *AND*
> when lala=ooh, and you can't have the enum for 'foo' nested underneath
> the enum for 'wizz' & 'lala' at the same time.  Also with nesting, if
> you want the same values reported for multiple options, we end up
> repeating the same information multiple times which is not good.
> 
> At the same time the flat modelling with "spiceGL" also doesn't scale
> as you have to invent new names for each one, and again it doesn't
> work if the 'gl' enum depended on type="spice" *and* something else
> at the same time.
> 
> So I think we need a more flexible way to express dependancies here.
> 
> We could let the <enum> be repeated multiple times and express conditional
> rules against each instance of the enum
> 
> So for example
> 
>     <graphics supported='yes'>
>       <enum name='type'>
>         <value>spice</value>
>         <value>vnc</value>
>         <value>sdl</value>
>       </enum>
>       <enum name='gl'>
>         <condition>
> 	  <enum name="type" value="spice">
> 	</condition>
>         <value>default</value>
>         <value>yes</value>
>         <value>no</value>
>       </enum>
>       <enum name='gl'>
>         <value>default</value>
>         <value>no</value>
>       </enum>
>     </graphics>
> 
> 
> This would express that the first <enum type="gl"> entry only applies
> when the @type == spice. If that doesn't match them fallback to the
> second <enum type="gl">.  The nice thing about this is that we can
> make the conditions arbitrarly complex
> 
> For example:
> 
>     <graphics supported='yes'>
>       <enum name='type'>
>         <value>spice</value>
>         <value>vnc</value>
>         <value>sdl</value>
>       </enum>
>       <enum name='gl'>
>         <condition op="and">
> 	  <enum name="type" value="spice">
> 	  <condition op="or">
>             <enum name="type" value="vnc">
> 	    <enum name="wibble" value="eek">
> 	  </condition>
> 	</condition>
>         <value>default</value>
>         <value>yes</value>
>         <value>no</value>
>       </enum>
>       <enum name='gl'>
>         <value>default</value>
>         <value>no</value>
>       </enum>
>     </graphics>
> 
> 
> This shows how the "gl" option can be used with VNC, but only if you
> also have the @wibble attribute set to "eek".
> 
> Of course complex conditions like this become quite tedious & verbose
> so obviously we should strive to keep them as simple as possible and
> not use nested conditions unless absolutely needed.
> 

Writing a parser for this type of XML, that maintains behavior in a future
proof way, is going to be a lot of work. Hypothetical example:

  <video supported='yes'>
    <enum name='modelType'/>
      <value>cirrus</value>
      <value>qxl</value>
    </enum>
    <enum name='accel2d'/>
      <value>no</value>
    </enum>
  </video>

6 months later, qemu gets some accel2d support for model=qxl

  <video supported='yes'>
    <enum name='modelType'/>
      <value>cirrus</value>
      <value>qxl</value>
    </enum>
    <enum name='accel2d'/>
      <value>no</value>
    </enum>
    <enum name='accel2d'/>
      <condition>
	<enum name="modelType" value="qxl">
      </condition>
      <value>yes</value>
      <value>no</value>
    </enum>
  </video>

Another 6 months later, qemu gets accel2d support for cirrus only, but it
requires <address type='pci'/> and doesn't work for <address type='isa'/>

  <video supported='yes'>
    <enum name='modelType'/>
      <value>cirrus</value>
      <value>qxl</value>
    </enum>
    <enum name='addressType'/>
      <value>pci</value>
      <value>isa</value>
    </enum>
    <enum name='accel2d'/>
      <value>no</value>
    </enum>
    <enum name='accel2d'/>
      <condition>
	<enum name="modelType" value="qxl">
      </condition>
      <value>yes</value>
      <value>no</value>
    </enum>
    <enum name='accel2d'/>
      <condition>
	<enum name="modelType" value="cirrus">
        <enum name="addressType" value="pci">
      </condition>
      <value>yes</value>
      <value>no</value>
    </enum>
  </video>


The app wants to answer the question 'can I specify accel2d by default for
model=cirrus?' (note, cirrus, and not qxl)

For the first example, the code parses the accel2d enum, doesn't see 'yes', so
it doesn't enable accel2d. The code will need to contain some knowledge that
enum name='accel2d' == ./video/acceleration/@accel2d but that's presently
unavoidable.

For the second example, code will need to be added to parse the basic
condition, see that modelType=cirrus (./video/model/@type) doesn't match the
condition, and so again accel2d=no

For the third example, code hits the modelType=cirrus condition, but then also
needs to check that addressType (./video/address/@type) == pci, and if so,
sets accel2d=yes

So, that's all achievable. But what kind of parser does the app write at each
step that may fall over with the next XML iteration?

What if the parser in step 1 doesn't expect multiple <enum name='accel2d'/> ?
If it naively parses only the first instance, then nothing will regress in
step #2 or step #3. step #3's conclusion will be wrong, but that's okay. Note
this depends on libvirt being smart about appending conditional enums, and not
putting them first.

However if it parses the last instance, such as by iterating over every XML
node and taking the last one that matches name='accel2d' (virtinst used to
have a pattern like this when parsing capabilities XML), on step #2 the code
will think the <enum> with the 'qxl' condition is the only instance. The
parser doesn't know about <condition> yet, will assume accel2d is supported
for everyone, and set accel2d=on for cirrus which will probably fail.

Okay, starting from a working step #2. The parser knows about multiple enums,
and basic <condition> statements. Upgrade to step #3. If the parser was dumb,
maybe it didn't know about multiple <condition> enums, which may cause an
incorrect match, setting accel2d=yes, and the config fails.

Assume the parser is smart enough to know about multiple <condition> enums. It
sees addressType, but it doesn't know what XML value it maps to. Does the app
just ignore it and hope the config works, possibly generating an error? Does
the app completely abandon the check even though it may work correctly? The
latter is certainly the safest, but coding errors/naive impls could make
mistakes or possible even throw other errors.

Now consider that pattern expanding out with conditional or/and/not operators,
maybe even numerical comparisons like <, >, etc. The only way for the parser
from step #1 to not make a fatal error in step #3 is to implement <condition>
parsing from the start and code very defensively. That's a lot of upfront
overhead. And I think it will be tempting for apps to try and get by _without_
implementing the whole comparison engine and potentially set themselves up for
future pain.

Also having a very interconnected schema like this means that if we ever need
to extend the XML format we need to think _very_ hard about how parsers are
likely handing the existing XML, and how our additions might screw things up,
because we are requiring users to perform some kind of comparison on the
result. This is much different than extending the domain XML which is largely
just a config file. <capabilities> XML is the closet analog but even that
requires very little processing, the only tricky bit is handling that <domain
type='kvm'> has its own <machine> list

And even after all that, the XML is not going to be completely introspectable
unless we find a way to describe addressType=./video/address/@type in the XML.
So any general parser is still going to need to be extended regularly to
handle new enum names. So we end up with XML that
appears-introspectable-but-not-quite


So let's redo the example with the unique string names and flat namespace:

Step #1 is identical
Step #2 addition is

    <enum name='qxlAccel2D'/>
      <value>yes</value>
      <value>no</value>
    </enum>

Step #3 addition is

    <enum name='cirrusAccel2D'/>
      <value>yes</value>
      <value>no</value>
    </enum>

And for step #3 we document that it's only valid for address type=pci, If
address type=isa grows support later, we add a new top level string if we
think it's worth it. The type=pci vs type=isa is a misleading example here, if
it was some more obscure option that cirrus accel2d depended on, maybe we
would want to put that in the string name.

The misinterpretation problems for the app are non-existent. Yes it will need
to be extended at step #3 to support cirrus accel2d=yes but that's completely
fine IMO.

If the next day, xen supports domcaps and wants to expose cirrusAccel2D but
with sufficiently different semantics, then we would have to add a new string.
Yes that sucks, but it's the safest way by far for apps.

Benefits:
- The app XML parser is much simpler
- The app XML parser is much quicker to bring up. Important because I bet most
apps will only want to check a small set of support checks
- I suspect extending the libvirt code will be much simpler
- We can always come up with a way to add a comparison engine later if this
approach gets too out of hand

Downsides:
- Much less generalizable (but as I said above I don't think this there _is_ a
generalizable domcaps XML schema, nor should it be the goal)
- Apps/readers need to look to the docs to understand the enum semantics
- The app's parser requires more maintenance in relation to how many features
it wants to parse

I think the guide here should largely be exposing the types of things that we
already track with qemu capabilities, and at that granularity. Give that
information to the user (which they presently can't find out reliably on their
own), and leave the rest of the bits like interdependence of XML options up to
them to figure out. So, 'qemu supports spiceGL', and not 'a working spiceGL
config is spiceGL with listen=none or listen=unix and video virtio with accel3D'

All that said, I'm probably missing some examples where this flat modeling
especially sucks too :) So please throw them my way

Thanks,
Cole

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[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]