On Mon, May 09, 2016 at 06:53:01PM -0400, Cole Robinson wrote: > On 05/09/2016 09:48 AM, Daniel P. Berrange wrote: > > 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. This is all simply a matter of documentation - if we're to allow multiple <enum> elements, then clearly we need to document the rules that a parser should follow so that its behaviour is consistent. IOW, we would have to document whether the default (no conditional) enum always comes first or last, or whether the application should explicitly look for an enum without any <condition> rules. Any one of those is a viable option - we just need to document which. Or if we're really needing to avoid causing trouble for pre-existing parsers, we give the non-default enums which permit use of <condition> a different element name, eg <enumVariant>. > 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. Again you're just describing a case where we need to document the intended semantics of the information. If apps using the data ignore the documented semantics that's their problem. We also need to be extra careful about how we extend the information in the future such that we don't add extra elements can could confuse existing parsers. This is a little more strict that we've been with XML extension in general, but totally managable. > 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. We would document that if an application doesn't understand the particular condition, then it should ignore that whole enum block. It would thus use the next/previous best-matching enum block it found. > 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. Again apps should explicitly ignore enums with comparison operators that they don't know about, and fallback to the next best matching enum they can find. > 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. 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. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list