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)
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list