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

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

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.

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
RNG schema validation is the only sane way to catch this


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'/>

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

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)

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