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

Re: [libvirt] [PATCH v2] Correct include-password option and rewrite domdisplay



On 11/22/12 11:34, Martin Kletzander wrote:
The 'virsh domdisplay' command is able to display the password
configured for spice, but it was missing for vnc type graphics.
Also, there were some inconsistencies that are cleaned now.
---
  tools/virsh-domain.c | 74 +++++++++++++++++++++++++++++-----------------------
  1 file changed, 42 insertions(+), 32 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index cc47383..cc5c830 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c


@@ -7069,14 +7074,36 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
          listen_addr = virXPathString(xpath, ctxt);
          VIR_FREE(xpath);

+        /* We can query this info for all the graphics types since we'll
+         * get nothing for the unsupported ones (just rdp for now) */
+        if (vshCommandOptBool(cmd, "include-password")) {
+            /* Create our XPATH lookup for the password */
+            virAsprintf(&xpath,
+                        "string(/domain/devices/graphics"
+                        "[ type='%s']/@passwd)",
+                        scheme[iter]);

The usual way is to check the return value of virAsprintf here instead of checking the allocated memory.

+
+            if (!xpath) {
+                virReportOOMError();
+                goto cleanup;

Hm, a no_memory label would decrease the line count somewhat, but that's not necessary ...

+            }
+
+            /* Attempt to get the password */
+            passwd = 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 */
+            /* 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]);
+            virAsprintf(&xpath,
+                        "string(/domain/devices/graphics[ type='%s']"
+                        "/@tlsPort)",
+                        scheme[iter]);

Same as the first comment (although it was pre-existing).

              if (!xpath) {
                  virReportOOMError();
                  goto cleanup;
@@ -7087,25 +7114,17 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
              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;
-                }
-
-                /* Attempt to get the SPICE password */
-                passwd = virXPathString(xpath, ctxt);
-                VIR_FREE(xpath);
-            }
          }

          /* 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 (passwd) {
+            virBufferAsprintf(&buf, ":%s@", passwd);

Doesn't this break something that was working previously? I'm not using this but the original way was to append "?password=adsgf".

+            VIR_FREE(passwd);

Move this free to the cleanup section.

+        }
+
          /* Then host name or IP */
          if (!listen_addr || STREQ((const char *)listen_addr, "0.0.0.0"))
              virBufferAddLit(&buf, "localhost");
@@ -7115,20 +7134,11 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
          VIR_FREE(listen_addr);

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

          /* TLS Port */
          if (tls_port)
-            virBufferAsprintf(&buf, "&tls-port=%d", tls_port);
-
-        /* Password */
-        if (passwd) {
-            virBufferAsprintf(&buf, "&password=%s", passwd);
-            VIR_FREE(passwd);
-        }
+            virBufferAsprintf(&buf, "?tls-port=%d", tls_port);

          /* Ensure we can print our URI */
          if (virBufferError(&buf)) {


I'm not sure about the change of the password parameter. Could you back that up somehow?

Peter


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