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

[Peter Krempa]

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