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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list