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

Re: [libvirt] [PATCH v2] virsh: Rewrite cmdDomDisplay



On 11/22/12 16:24, Martin Kletzander wrote:
Just a little rewrite of the cmdDomDisplay function to make it
consistent and hopefully more readable.  This also fixes a problem
with password not being displayed for vnc even with the
"--include-password" option.
---
  tools/virsh-domain.c | 132 +++++++++++++++++++++++++--------------------------
  1 file changed, 64 insertions(+), 68 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index cc47383..1e8ccc9 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -7003,9 +7003,9 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
      virDomainPtr dom;
      virBuffer buf = VIR_BUFFER_INITIALIZER;
      bool ret = false;
-    char *doc;
-    char *xpath;
-    char *listen_addr;
+    char *doc = NULL;
+    char *xpath = NULL;
+    char *listen_addr = NULL;
      int port, tls_port = 0;
      char *passwd = NULL;
      char *output = NULL;
@@ -7013,6 +7013,8 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
      int iter = 0;
      int tmp;
      int flags = 0;
+    bool params = false;
+    const char *xpath_fmt = "string(/domain/devices/graphics[ type='%s']/@%s)";

      if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
          return false;
@@ -7025,109 +7027,95 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
      if (vshCommandOptBool(cmd, "include-password"))
          flags |= VIR_DOMAIN_XML_SECURE;

-    doc = virDomainGetXMLDesc(dom, flags);
-
-    if (!doc)
+    if (!(doc = virDomainGetXMLDesc(dom, flags)))
          goto cleanup;

-    xml = virXMLParseStringCtxt(doc, _("(domain_definition)"), &ctxt);
-    VIR_FREE(doc);
-    if (!xml)
+    if (!(xml = virXMLParseStringCtxt(doc, _("(domain_definition)"), &ctxt)))
          goto cleanup;

      /* Attempt to grab our display info */
      for (iter = 0; scheme[iter] != NULL; iter++) {
          /* Create our XPATH lookup for the current display's port */
-        virAsprintf(&xpath, "string(/domain/devices/graphics[ type='%s']"
-                "/@port)", scheme[iter]);
-        if (!xpath) {
-            virReportOOMError();
-            goto cleanup;
-        }
+        if (virAsprintf(&xpath, xpath_fmt, scheme[iter], "port") < 0)
+            goto no_memory;

          /* Attempt to get the port number for the current graphics scheme */
          tmp = virXPathInt(xpath, ctxt, &port);
          VIR_FREE(xpath);

          /* If there is no port number for this type, then jump to the next
-         * scheme
-         */
+         * scheme */
          if (tmp)
              continue;

          /* Create our XPATH lookup for the current display's address */
-        virAsprintf(&xpath, "string(/domain/devices/graphics[ type='%s']"
-                "/@listen)", scheme[iter]);
-        if (!xpath) {
-            virReportOOMError();
-            goto cleanup;
-        }
+        if (virAsprintf(&xpath, xpath_fmt, scheme[iter], "listen") < 0)
+            goto no_memory;

          /* Attempt to get the listening addr if set for the current
-         * graphics scheme
-         */
+         * graphics scheme */
          listen_addr = virXPathString(xpath, ctxt);
          VIR_FREE(xpath);

-        /* Per scheme data mangling */
-        if (STREQ(scheme[iter], "vnc")) {
-            /* VNC protocol handlers take their port number as 'port' - 5900 */
+        /* We can query this info for all the graphics types since we'll
+         * get nothing for the unsupported ones (just rdp for now).
+         * Also the parameter '--include-password' was already taken
+         * care of when getting the XML */
+
+        /* Create our XPATH lookup for the password */
+        if (virAsprintf(&xpath, xpath_fmt, scheme[iter], "passwd") < 0)
+            goto no_memory;
+
+        /* Attempt to get the password */
+        passwd = virXPathString(xpath, ctxt);

You forgot to VIR_FREE(xpath) here leaking one of the query strings.

+
+        if (STREQ(scheme[iter], "vnc"))
+            /* VNC protocol handlers take their port number as
+             * 'port' - 5900 */
              port -= 5900;
-        } else if (STREQ(scheme[iter], "spice")) {
-            /* Create our XPATH lookup for the SPICE TLS Port */
-            virAsprintf(&xpath, "string(/domain/devices/graphics[ type='%s']"
-                    "/@tlsPort)", scheme[iter]);
-            if (!xpath) {
-                virReportOOMError();
-                goto cleanup;
-            }

-            /* Attempt to get the TLS port number for SPICE */
-            tmp = virXPathInt(xpath, ctxt, &tls_port);
-            VIR_FREE(xpath);
-            if (tmp)
-                tls_port = 0;
-
-            if (vshCommandOptBool(cmd, "include-password")) {
-                /* Create our XPATH lookup for the SPICE password */
-                virAsprintf(&xpath, "string(/domain/devices/graphics"
-                        "[ type='%s']/@passwd)", scheme[iter]);
-                if (!xpath) {
-                    virReportOOMError();
-                    goto cleanup;
-                }
+        /* Create our XPATH lookup for TLS Port (automatically skipped
+         * for unsupported schemes */
+        if (virAsprintf(&xpath, xpath_fmt, scheme[iter], "tlsPort") < 0)
+            goto no_memory;

-                /* Attempt to get the SPICE password */
-                passwd = virXPathString(xpath, ctxt);
-                VIR_FREE(xpath);
-            }
-        }
+        /* Attempt to get the TLS port number */
+        tmp = virXPathInt(xpath, ctxt, &tls_port);
+        VIR_FREE(xpath);
+        if (tmp)
+            tls_port = 0;

          /* Build up the full URI, starting with the scheme */
          virBufferAsprintf(&buf, "%s://", scheme[iter]);

+        /* There is no user, so just append password if there's any */
+        if (STREQ(scheme[iter], "vnc") && passwd)
+            virBufferAsprintf(&buf, ":%s@", passwd);
+
          /* Then host name or IP */
          if (!listen_addr || STREQ((const char *)listen_addr, "0.0.0.0"))
              virBufferAddLit(&buf, "localhost");

Pre_existing: Sigh, this still doesn't work on remote connections :(

          else
              virBufferAsprintf(&buf, "%s", listen_addr);

-        VIR_FREE(listen_addr);
-
          /* Add the port */
-        if (STREQ(scheme[iter], "spice"))
-            virBufferAsprintf(&buf, "?port=%d", port);
-        else
-            virBufferAsprintf(&buf, ":%d", port);

This changes the semantics sligthly but I don't care that much if you are convinced this doesn't break anything. You honor the original place where port should be specified in an URI

+        virBufferAsprintf(&buf, ":%d", port);

          /* TLS Port */
-        if (tls_port)
-            virBufferAsprintf(&buf, "&tls-port=%d", tls_port);
+        if (tls_port) {
+            virBufferAsprintf(&buf,
+                              "%stls-port=%d",
+                              params ? "&" : "?",
+                              tls_port);
+            params = true;
+        }

-        /* Password */
-        if (passwd) {
-            virBufferAsprintf(&buf, "&password=%s", passwd);
-            VIR_FREE(passwd);
+        if (STREQ(scheme[iter], "spice") && passwd) {
+            virBufferAsprintf(&buf,
+                              "%spassword=%s",
+                              params ? "&" : "?",
+                              passwd);
+            params = true;
          }

          /* Ensure we can print our URI */
@@ -7139,7 +7127,6 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
          /* Print out our full URI */
          output = virBufferContentAndReset(&buf);
          vshPrint(ctl, "%s", output);
-        VIR_FREE(output);

          /* We got what we came for so return successfully */
          ret = true;
@@ -7147,10 +7134,19 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
      }

  cleanup:
+    VIR_FREE(doc);
+    VIR_FREE(xpath);
+    VIR_FREE(passwd);
+    VIR_FREE(listen_addr);
+    VIR_FREE(output);
      xmlXPathFreeContext(ctxt);
      xmlFreeDoc(xml);
      virDomainFree(dom);
      return ret;
+
+no_memory:
+    virReportOOMError();
+    goto cleanup;
  }

  /*


Nice cleanup. ACK with the memleak fixed.

Peter


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