10:40 a.m.
On 07/27/2011 01:47 AM, Laine Stump wrote:
> I'm not sure this is right. I think we want to require
either
> type=address (which implies address=nnn must also be present, and
> network=nnn must not be present) or type=network (which implies that
> network=nnn must be present, and address=nnn might show up in live XML
> dumpxml output to show what was resolved, but is ignored in input
> parsing. Also, autoport=yes and port=nnn should be a forbidden
> combination.
Actually it's not forbidden, that's really the only practical use I can
see for the autoport attribute - in live XML "autoport='yes'
port='nnn'"
is used to indicate that the port was auto-generated and this time 'nnn'
was used. (and not that it's any indication of what is right or wrong,
but the current RNG file allows it).
...
Well, existing code doesn't do that. "Do no harm".
Certainly in live XML this is acceptable, and for inactive XML there may
be cases where someone grabs live XML, modifies it, and sends the
results back to the parse as config (aka inactive).
Yeah, on thinking more, it would be nice if the rng file accepted all
live xml (it doesn't, because of other aspects like <alias> and
<actual>, but in general we tend to ignore rather than reject stuff we
don't recognize).
So maybe your rng is okay as is - it accepts more than what is strictly
necessary, but as long as it doesn't reject anything that the code
accepts, we're okay. The question comes down to whether it is easy to
make the rng reject stuff that the code rejects, or whether to keep the
rng simple and over-permissive.
I think if we made autoport a single setting global to all instances of
listen, and tried to prevent simultaneous (global) autoport and (local
to one listen) port, the RNG would be quite unwieldy. (you would have to
have a listenElementsPort and a listenElementsAutoport, and the graphics
RNG would put the two of them in a <choice> depending on the setting of
autoport)
Interesting point. Overall, I think it boils down to whether we ever
envision having more than one <listen>, where one specified a port and
the other let the port be chosen automatically. Conversely, if we start
strict, we can always relax later (especially since it won't be till
later that we support multiple <listen>), whereas if we start relaxed,
it's harder to tighten things up. But I'm starting to swing towards
having autoport remain with <listen>.
> Shouldn't this fail if type is not found? That is, I think
type is a
> mandatory attribute (must be address or network).
No. It's also possible that no address/network information is given (in
which case type would be "default", which ends up as just no type
showing up at all). In that case, the qemu driver has default listen
addresses for VNC and for Spice. So it's completely valid to give a
port, but no address.
Makes sense.
So I think this can be expressed in rng as:
<optional>
<choice>
<group>
<attribute>...type=address
<attribute>...address=iporname
</group>
<group>
<attribute>...type=network
<attribute>...network=name
<optional>
<attribute>...address=ip
</optional>
</group>
</choice>
</optional>
That is, type is optional, but if present, then it determines whether
address or network must also be present. Meanwhile, address can appear
with either type, but if address appears, then type must appear.
>
>> +
>> + if (address&& address[0]) {
>> + def->address = address;
>> + address = NULL;
>
> Should we reject address if not VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS?
No. Although I haven't found the right place to put it in, it's possible
that the listen type may be network, and the resolved address will be
filled in so that it can show up as status in the live XML. (I actually
originally had exactly that check put in, but removed it for this reason).
Yep, I agree that we want to be able to parse existing live output, even
if we ignore that parse on INACTIVE.
>
> I learned with my virDomainSaveImageDefineXML patch that parsing must
> be symmetric to output format. Since your output function produces
> address for network mode on live xml but skips it for inactive, you
> _must_ make the parser leave address as NULL on parsing if (flags &
> VIR_DOMAIN_XML_INACTIVE) and network mode is detected.
I hadn't thought of that. But should we reject it in that case, or
should we *ignore* it? (Will it ever be the case that someone will feed
live XML back to the parser as config? And should we complain in that
case, or silently ignore the extra, just as we would if it was something
completely unknown to us?) (I usually tend to follow the IETF mantra of
being lenient in what you accept, and strict in what you give out)
In general, we ignore (not reject) live-only information when doing an
INACTIVE parse. See for example how <alias> is treated - the format
routines only output <alias> on live images, and when parsing INACTIVE,
any existing <alias> is silently ignored.
Possibly, I can modify the formatter to (1) never output port=-1 in
listen, and 2) only output autoport in listen if we're doing live XML.
Then on input, if <graphics has port=-1 that ends up being changed to
port=0 + autoport=yes anyway, so the check to verify they match would
still work.
Seems like a good plan of attack.
Two points here:
1) I did say in earlier conversations that the attributes in <graphics>
should match the first <listen> that has type='address'. I didn't have
the time to implement it that way, though. Fortunately we currently have
no driver that understands more than a single <listen> anyway, so this
can be glossed over for the moment (as long as it's not forgotten).
If you don't address it now, be sure to leave some comments to that effect.
And if I haven't made it clear before, I'd like to see something like
this patch make it into 0.9.4 - we've missed the rc1 freeze, but this
only changes xml rather than adding new API, and it can be argued that
this patch is necessary to round out a new feature being advertised for
0.9.4 (that is, the feature of being able to isolate host-dependent
network information out of domain xml is buggy without this patch).
2) Your example brings up an interesting question: the idea of allowing
both a <listen> as well as the legacy attributes in <graphics> is really
intended to ease compatibility with old applications that only
understand the old attributes, and the duplication should *really* only
be done by our formatter, not by anyone creating new XML. Our formatter
would never copy port='5900' into <graphics> without also putting in
listen='127.0.0.1' (since that's in the <listen> too. But the parser
only checks that individual attributes match when specified in both
places; it doesn't do a "if *any* attribute is specified in both places,
they must *all* be specified in both places" check. Do we need to do
that, or is it overkill? (this is a place where I can get on the "it's
overkill" boat :-)
I think you've convinced me - pairwise comparison is simple enough to
get this patch finished, and while entire set comparison would be
stricter, it's probably overkill to worry about it now.
> See the earlier comments - if you honor VIR_DOMAIN_XML_INACTIVE
here
> to control what gets output, you must also honor it on parsing to
> control what gets skipped.
Skipped, or forbidden? I've seen you suggest both in different (but
similar) contexts.
You've convinced me - skip (not reject) anything that is valid as live
xml but irrelevant as inactive xml (that is, autoport=yes + port=nnn on
an INACTIVE parse takes only autoport=yes, and ignores port). We can
reserve rejections for something that will 1) never be output as live
xml, and 2) would create an actual conflict (like trying to specify tls
port and vnc).
We may disagree about whether certain "extra stuff" should be ignored or
rejected, though. I'll go through it again in the morning and see what
falls out.
I definitely think I found a few things, but my original review mail is
probably larger than the actual amount of changes you need to make in a v4.
Thanks for the review!
One idea for splitting this into easier reviews (don't know how hard it
would be though): in patch 1, implement the helper functions to get and
set values, and convert all callers to use the helper functions, but
where the helper functions are one-liner wrappers around direct access.
In patch 2, change domain_conf to support <listen> attributes, and
update the helper functions, as well as update the test cases to react
to the new formatted xml.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org