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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org