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

[Laine Stump]

On 07/26/2011 08:09 PM, Eric Blake wrote:
> On 07/25/2011 03:00 AM, Laine Stump wrote:
>> Once it's plugged in, the<listen element will be an optional
>> replacement for the "listen", "port", "tlsPort", and "autoport"
>> attributes that graphics elements already have. If the<listen>
>> type='address', it will have an attribute called 'address' which will
>> contain an IP address or dns name that the guest's display server
>> should listen on. If, however, type='network', the<listen>  element
>> should have an attribute called 'network' that will be set to the name
>> of a network configuration to get the IP address from.
>>
>>   91 files changed, 1274 insertions(+), 343 deletions(-)
>
> Big diff, but like you said mostly mechanical.
>
>> @@ -2110,6 +2116,70 @@ qemu-kvm -net nic,model=? /dev/null
>> </dd>
>> </dl>
>>
>> +<p>
>> +      Rather than putting the information used to setup the listening
>
> setup is a noun, but here you want a verb form:
>
> s/setup/set up/
>
>> +<dt><code>autoport</code></dt>
>> +<dd>If set to 'yes', a listen port will be determined
>> +        automatically at runtime, and reflected in the domain's live
>> +        XML.
>> +</dd>
>
> May need tweaks depending on the decision on where autoport should live.
>
>> @@ -1500,6 +1503,49 @@
>> </choice>
>> </element>
>> </define>
>> +
>> +<define name="listenElements">
>> +<zeroOrMore>
>> +<element name="listen">
>> +<optional>
>> +<attribute name="type">
>> +<choice>
>> +<value>address</value>
>> +<value>network</value>
>
> /me curses thunderbird for squashing whitespace in my reply
>
> 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).

(Personally, I would prefer if the currently in-use port was stored in a 
separate attribute, and "autoport" simply didn't exist. But


>   That is, I think this should be as follows (assuming we keep 
> autoport in <listen>, further changes if autoport is global only):


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)


>
>
> <define name="listenElements">
> <zeroOrMore>
> <element name="listen">
> <choice>
> <group>
> <attribute name="type">
> <value>address</value>
> </attribute>
> <attribute name="address">
> <ref name="addrIPorName"/>
> </attribute>
> </group>
> <group>
> <attribute name="type">
> <value>network</value>
> </attribute>
> <attribute name="network">
> <text/>
> </attribute>
> <optional>
> <attribute name="address">
> <ref name="addrIPorName"/>
> </attribute>
> </optional>
> </group>
> </choice>
> <choice>
> <attribute name="autoport">
> <value>yes</value>
> </attribute>
> <group>
> <optional>
> <attribute name="autoport">
> <value>no</value>
> </attribute>
> </optional>
> <optional>
> <attribute name="port">
> <ref name="PortNumber"/>
> </attribute>
> </optional>
> <optional>
> <attribute name="tlsPort">
> <ref name="PortNumber"/>
> </attribute>
> </optional>
> </group>
> </choice>
> </element>
>
>> +</zeroOrMore>
>> +</define>
>
> Two spaces too much indentation on </define> (not like it shows up any 
> better in my nitpick, though).
>
>> @@ -4010,19 +4029,118 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr 
>> node,
>>       return 0;
>>   }
>>
>> +static int
>> +virDomainGraphicsListenParseXML(virDomainGraphicsListenDefPtr def,
>> +                                xmlNodePtr node,
>> +                                enum virDomainGraphicsType 
>> graphicsType,
>> +                                unsigned int flags)
>> +{
>> +    int ret = -1;
>> +    char *type     = virXMLPropString(node, "type");
>> +    char *address  = virXMLPropString(node, "address");
>> +    char *network  = virXMLPropString(node, "network");
>> +    char *port     = virXMLPropString(node, "port");
>> +    char *tlsPort  = virXMLPropString(node, "tlsPort");
>> +    char *autoport = virXMLPropString(node, "autoport");
>> +
>> +    if (type&&  (def->type = 
>> virDomainGraphicsListenTypeFromString(type))<  0) {
>> +        virDomainReportError(VIR_ERR_XML_ERROR,
>> +                             _("unknown graphics listen type '%s'"), 
>> type);
>> +        goto error;
>> +    }
>
> 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.


>
>> +
>> +    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).


>
> 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)

>
>> +    }
>> +
>> +    if (network&&  network[0]) {
>> +        if (def->type != VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK) {
>> +            virDomainReportError(VIR_ERR_XML_ERROR,
>> +                                 _("network attribute not allowed 
>> when listen type is not network"));
>> +            goto error;
>> +        }
>> +        def->network = network;
>> +        network = NULL;
>> +    }
>
> Are we also missing checks that if type is address, then address was 
> specified; if type is network, then network was specified?


True. I missed that. Definitely that can't be ignored.


>
>> +
>> +    if (port) {
>> +        if (virStrToLong_i(port, NULL, 10,&def->port)<  0) {
>> +            virDomainReportError(VIR_ERR_XML_ERROR,
>> +                                 _("cannot parse listen port %s"), 
>> port);
>> +            goto error;
>> +        }
>> +        /* legacy syntax for graphics type=vnc used -1 for
>> +         * autoport. We need to maintain it here because the legacy
>> +         * attributes in<graphics>  must match those in
>> +         *<listen>. */
>
> I'm not sure I buy this.  We need to output the legacy attributes in 
> <graphics>, but <listen> is not constrained by legacy, so we can 
> require that if you use <listen> you only use the newer syntax.

(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.

>
>> +        if ((graphicsType == VIR_DOMAIN_GRAPHICS_TYPE_VNC)&&
>> +            (def->port == -1)) {
>> +            if (flags&  VIR_DOMAIN_XML_INACTIVE)
>> +                def->port = 0;
>> +            def->autoport = true;
>> +        }
>> +    } else {
>> +        def->autoport = true;
>> +    }

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...


>> +
>> +    if (tlsPort&&
>> +        (graphicsType == VIR_DOMAIN_GRAPHICS_TYPE_SPICE)) {
>> +        if (virStrToLong_i(tlsPort, NULL, 10,&def->tlsPort)<  0) {
>> +            virDomainReportError(VIR_ERR_XML_ERROR,
>> +                                 _("cannot parse spice tlsPort %s"), 
>> tlsPort);
>> +            goto error;
>> +        }
>> +    }
>
> Should this error out if tlsPort but not GRAPHICS_TYPE_SPICE?

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).

>
>
>> +
>> +    if (autoport&&  STREQ(autoport, "yes")) {
>> +        if (flags&  VIR_DOMAIN_XML_INACTIVE) {
>> +            def->port = 0;
>> +            def->tlsPort = 0;
>> +        }
>> +        def->autoport = true;
>
> We should check that autoport=yes and port are not simultaneously 
> 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).

>
>> +            if (listenAddr&&  STRNEQ_NULLABLE(listenAddr,
>> +                                              
>> virDomainGraphicsListenGetAddress(def, 0))) {
>> +                virDomainReportError(VIR_ERR_XML_ERROR,
>> +                                     _("graphics listen attribute %s 
>> must match address "
>> +                                       "attribute of first listen 
>> element (found %s)"),
>> +                                     listenAddr, 
>> virDomainGraphicsListenGetAddress(def, 0));
>> +                goto error;
>> +            }
>
> So what if I do:
>
> <graphics port="5900">
> <listen type="network" network="name" port="5901"/>
> <listen type="address" address="127.0.0.1" port="5900"/>
> </graphics>
>
> Is that a valid setup, even though the legacy port doesn't match the 
> first <listen> port?  That is, are we validating that the legacy must 
> match the first <listen>, or only the first <listen type=address>?

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 :-)


>
>>
>> +
>> +static void
>> +virDomainGraphicsListenDefFormat(virBufferPtr buf,
>> +                                 virDomainGraphicsListenDefPtr def,
>> +                                 enum virDomainGraphicsType 
>> graphicsType,
>> +                                 unsigned int flags)
>> +{
>> +    virBufferAddLit(buf, "<listen");
>> +
>> +    if (def->type) {
>> +        virBufferAsprintf(buf, " type='%s'",
>> +                          
>> virDomainGraphicsListenTypeToString(def->type));
>> +    }
>> +
>> +    if (def->address&&
>> +        ((def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS)
>> +         || !(flags&  VIR_DOMAIN_XML_INACTIVE))) {
>> +        /* address may also be set to show current status when 
>> type='network',
>> +         * but we don't want to print that if INACTIVE data is 
>> requested. */
>> +        virBufferAsprintf(buf, " address='%s'", def->address);
>> +    }
>
> 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.


>
>> +
>> +    if (def->network&&
>> +        (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK)) {
>> +        virBufferAsprintf(buf, " network='%s'", def->network);
>> +    }
>> +
>> +    /* For VNC definitions, if the INACTIVE XML was requested and
>> +     * autoport is on, we don't print output the port. (Not sure why
>> +     * the same thing isn't done for RDP and SPICE, but the existing
>> +     * code that this is mimicking doesn't do it.) */
>
> Is that something we should do right, now that we're using new xml?  I 
> think it boils down to:
>
> In inactive xml, we want to know if the user specified a port 
> (autoport=no and port=nnn) or is okay with a default port 
> (autoport=yes and port can be omitted) - here, the autoport attribute 
> seems redundant.


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.)


>
> In active xml, we want to know if the user specified a port 
> (autoport=no and port=nnn), or was okay with a default port 
> (autoport=yes) and which port they got (port=nnn, or omitted if they 
> have not got one yet).
>
>> +static virDomainGraphicsListenDefPtr
>> +virDomainGraphicsGetListen(virDomainGraphicsDefPtr def, size_t ii, 
>> bool force0)
>> +{
>> +    if ((def->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) ||
>> +        (def->type == VIR_DOMAIN_GRAPHICS_TYPE_RDP) ||
>> +        (def->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE)) {
>> +
>> +        if (!def->listens&&  (ii == 0)&&  force0) {
>> +            if (VIR_ALLOC(def->listens)<  0)
>> +                virReportOOMError();
>
> This fails and leaves a message,
>
>> +            else
>> +                def->nListens = 1;
>> +        }
>> +
>> +        if (!def->listens || (def->nListens<= ii))
>> +            return NULL;
>
> but this fails without leaving a message.  That can confuse callers 
> (if they got NULL, do they need to report a message, or has one 
> already been reported)?  It might be better if you refactor things to 
> always report an error on failure here, except that...
>
>> +
>> +bool
>> +virDomainGraphicsListenGetAutoport(virDomainGraphicsDefPtr def, 
>> size_t ii)
>> +{
>> +    virDomainGraphicsListenDefPtr listenInfo
>> +        = virDomainGraphicsGetListen(def, ii, false);
>> +
>> +    if (!listenInfo)
>> +        return false;
>> +    return listenInfo->autoport;
>
> this is an example where you don't want an error message (returning 
> false is the right thing to do for out-of-bounds array)
>
>> +}
>> +
>> +
>> +int
>> +virDomainGraphicsListenSetAutoport(virDomainGraphicsDefPtr def, 
>> size_t ii, bool val)
>> +{
>> +    virDomainGraphicsListenDefPtr listenInfo
>> +        = virDomainGraphicsGetListen(def, ii, true);
>> +
>> +    if (!listenInfo)
>> +        return -1;
>
> whereas here, you want to know whether the -1 is because of an 
> allocation failure (message was reported, in the current code) or 
> because of a mismatch (<graphics> was not spice or vnc, so it has no 
> listen elements, no error message in the current code).


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.


>
> The rest of the patch looks fairly reasonable, so I think any 
> remaining effort is on ensuring a sane domain_conf.c parse and format 
> of the new attribute, how it interacts with autoport, and how the .rng 
> file reflects whatever restrictions are in the code.

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!