[libvirt] ignore vs. error when inappropriate or unrecognized attributes/elements are present in XML

[Laine Stump]

On 08/16/2011 04:58 PM, Daniel P. Berrange wrote:
> On Tue, Aug 16, 2011 at 03:29:04PM -0400, Laine Stump wrote:
>> On 08/16/2011 11:47 AM, Daniel P. Berrange wrote:
>>> On Tue, Aug 16, 2011 at 04:44:42AM -0400, Laine Stump wrote:
>>>> This is related to: https://bugzilla.redhat.com/show_bug.cgi?id=638633#c14
>>>>
>>>> I had started to reply to it in the comments of the bug, but my
>>>> reply became too long, and expanded into an issue wider than that
>>>> single bug, so I figured I'd better discuss it here instead.
>>> [snip]
>>>
>>>> Actually, I can see now there are several different classes of this
>>>> problem. Here are the first few that come to mind:
>>>>
>>>> 1) an attribute/element is completely unknown/unexpected in all
>>>> cases (e.g. "frozzle='fib'" anywhere, or more insidious, something
>>>> that *looks* correct, but isn't, e.g. "<script
>>>> name='/path/to/script'/>"*)
>>> RNG schema validation is the only sane way to catch this
>>
>> Agreed.
>>
>>
>>>> 2) an attribute/element is useful/expected only when some other
>>>> attribute is set to a particular value (usually one called "type",
>>>> but could be something else), for example keymap='blah' is only
>>>> expected in a<graphics>   element when type='spice' or type='vnc'.
>>> We should always catch these when parsing, since this is done
>>> via our enumeration helpers.
>>
>> "Enumeration helpers"? My brain isn't making the connection to what
>> you're talking about... :-)

Okay, I think I finally see what happened here - I believe my example 
misled you into thinking the problem I was talking about was having the 
value "blah" for the keymap attribute rather than some recognized value 
(otherwise why would you talk about enumeration helpers?). (of course 
for keymap, your suggestion isn't valid either, since it's stored as a 
char*, not as an enum value, so maybe I'm still misunderstanding you).

The problem I was *really* trying to point out is that of a keymap 
attribute being given *at all* when type is something other than "spice" 
or "vnc". The problem is that keymap is stored in a union, and the parts 
of the union that are active when type != "(spice|vnc)" don't have any 
keymap members. So even if keymap has something that would have been 
valid when type="(spice|vnc)" (but not otherwise), it isn't flagged in 
the parser (because the parser ignores keymap when it's not appropriate, 
rather than logging an error) and the driver *can't* do anything about 
it (because it has no way of knowing that a keymap was given - the 
parser can't put keymap into the config object because there's no place 
to put it).


Specifically for the bug report I'm looking at, this can't be flagged as 
an error:

<interface type='network'>
<script path='/path/to/script'/>
</interface>

because the script path is in a union, and only active if 
type="(bridge|ethernet)".

>>>> 3) an attribute/element is useful/expected only for certain
>>>> combinations of the value of some other attribute and which driver
>>>> is using the element, e.g. the subject of this bug - script='blah'
>>>> is only expected when type='bridge' and it's used by the Xen driver,
>>>> or type='ethernet' and it's used by the qemu driver.
>>> IMHO this is just another case of 1) really.
>> I guess as long as the hypervisor being used is part of the XML, and
>> the RNG is detailed enough to note that, eg, when the hypervisor is
>> xen and the interface type is bridge, a script element is okay, or
>> when the hypervisor is qemu and the interface type is ethernet, a
>> script is okay, but not in other cases. Wouldn't an RNG file that
>> adequately described that be fairly gigantic and filled with
>> redundancies, though?
> Forgot to mention that the RNG schemas shouldn't contain hypervisor
> specific conditionals. For cases where the XML is valid, but the
> config is not supported by the driver in question, should report a
> VIR_ERR_CONFIG_UNSUPPORTED error

Ah, so it *isn't* really just another case of (1) :-) (since your answer 
to (1) was to catch it in the RNG, and now you're saying the opposite)

I'll point out that a "hypervisor specific conditional" is just a larger 
scale example of the same phenomenon as a "interface-type specific 
conditional". What is it that makes hypervisor a special case? Isn't 
this exactly the way we should also handle, e.g. having keymap specified 
when graphic type="sdp"? (i.e., the hypervisor *does* support keymap, 
but graphics is a type that doesn't support it).


To try and get at a solution - I think one of the main problems here is 
the use of unions in the config objects - they're encoding 
driver-specific stuff into the config object, but the parser (and 
therefore the config object) should be driver-agnostic. Having those 
unions makes it impossible for the driver to report 
VIR_ERR_CONFIG_UNSUPPORTED in many cases, because the parser is unable 
to tell the driver that such an attempt was made. The unions look all 
pretty and nicely organized, and make it easy to tell which bits of 
config are used in which cases by just looking in the .h file, but I 
think in a majority of cases they are inappropriate and should be 
eliminated.

In particular, to solve 
https://bugzilla.redhat.com/show_bug.cgi?id=638633 according to your 
recommendation, I think what I need to do is:

1) Remove the script attribute from the bridge & ethernet unions in 
virDomainNetDef and place it in the common part of the struct.

2) Change the parser to always populate script, no matter what is the 
type of the virDomainNetDef.

3) In the driver code that uses the virDomainNetDef, check for script in 
*all* cases (not just the cases where the driver *wants* to see a 
script), and log VIR_ERR_CONFIG_UNSUPPORTED when it is specified at an 
inappropriate time.

Does this sound correct?

Or do you think that the only appropriate solution to that bug is to add 
the "validate" flag you mentioned earlier, and tell people to add that 
flag if they want an error like this caught? (I would argue that it 
should be up to the driver, not the parser, to decide whether or not a 
script is appropriate for any particular type of network interface)