[libvirt] [PATCH v2] virsh: Rewrite cmdDomDisplay
Peter Krempa
pkrempa at redhat.com
Wed Nov 28 13:38:34 UTC 2012
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
More information about the libvir-list
mailing list