Re: [libvirt] [PATCHv3 1/2] conf: add <listen> subelement to domain <graphics> element

On 07/26/2011 08:09 PM, Eric Blake wrote: 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). (Personally, I would prefer if the currently in-use port was stored in a separate attribute, and "autoport" simply didn't exist. But 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) 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. 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). 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) True. I missed that. Definitely that can't be ignored. (Keeping in mind that 3 days ago I'd never even thought about any of this, and while writing the code one of my top priorities was that I didn't break/modify any existing behavior of existing attributes, good or bad...) The data in the <listen> and the data in <graphics> are stored in the same place, and are required to match. The data that you're parsing in this listen may have been generated from the following sequence: <graphics type='vnc' listen='1.2.3.4' port='-1'/> parse, format <graphics type='vnc' listen='1.2.3.4' port='-1' autoport='yes'> <listen type='address', address='1.2.3.4' port='-1' autoport='yes'/> </graphics> (now we parse again) What we *don't* want to happen is for valid XML on the first input to turn into invalid XML after a parse/format roundtrip. In the existing code, autoport is always included in the output, and port is always set to -1 if autoport is yes and we're outputting inactive XML. 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. Okay, yes, I think I was being too pedantic there. Something I missed here when I combined the multiple places of parsing port (separate for vnc, rdp, and spice) into one - in the case of spice, if no port is specified, it hard codes a default of 5900, rather than setting autoport. Yuck. I guess I have to replicate that, but it certainly doesn't mean I have to like it... I think this falls in the category of something outside the bounds of what we recognize, so should be ignored, just as it would be ignored if graphics type was VNC and someone put in the attribute "frozzlePort='1234'". These kind of things are a gray area for me though - sometimes it's difficult to decide whether to ignore (it's extra stuff we don't regognize) or give an error (it's attempting to specify something that directly contradicts something else that has also been specified). 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). 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). 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 :-) Skipped, or forbidden? I've seen you suggest both in different (but similar) contexts. Yes, this is an extension of the earlier discussion. I only started thinking about this last Friday afternoon, and at first was nervous about having everything match up exactly all the time. I think I understand the interactions better now, and we can make the <listen> grammar more sane. For starters, I think autoport should never be output in config/inactive XML, and the ugly "port='-1'" should also never show up. In live XML, we should only output autoport if it is "yes" (we already do this). For parsing, I think we should silently accept autoport in live XML (and also in inactive XML as long as the provided value doesn't conflict with the setting of port), but port='-1' should have no special meaning (that will work okay, since the existing code never actually stores a port of -1 as such - it converts it to autoport=yes on input, and back to port=-1 on output.) Academically interesting, but currently not significant, since 1) the function is only ever called by code that has already qualified that the type is VNC, SPICE, or RDP (so the "wrong type" error will never happen), 2) the *Set*() functions are always called with ii=0 and force0=true, so for the Sets there will never be a case where we return NULL because the requested listen doesn't exist; it will only return NULL if we ran out of memory, and 3) for the *Get*() functions, again they're only called when type has already been qualified, so the only possible reason for returning NULL would be if the desired listen wasn't there at all, which isn't an error. I see this function as a convenience for a few functions in the same file of the current code, not as an API that has any type of contract that needs to be followed/maintained for future potential callers. I think I was too paranoid in my initial coding about maintaining exact 1:1 correspondence between the new <listen> parse/output and the old <graphics> parse/output. 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. Thanks for the review!