[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/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. That is, I think this should be as follows (assuming we keep autoport in <listen>, further changes if autoport is global only):

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

+
+    if (address&&  address[0]) {
+        def->address = address;
+        address = NULL;

Should we reject address if not VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS?

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.

+    }
+
+    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?

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

+        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;
+    }
+
+    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?

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

+            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>?


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

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

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


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.

--
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


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