[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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!



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]