[libvirt] [PATCH] virsh: Add domdisplay command for VNC and SPICE

[Michal Privoznik]

On 22.06.2012 07:30, Doug Goldstein wrote:
> Add a new 'domdisplay' command that provides a URI for both VNC and
> SPICE connections. Presently the 'vncdisplay' command provides you with
> the port info that QEMU is listening on but there is no counterpart for
> SPICE. Additionally this provides you with the bind address as specified
> in the XML, which the existing 'vncdisplay' lacks.
> 
> Signed-off-by: Doug Goldstein <cardoe at cardoe.com>
> ---
>  tools/virsh.c |  128 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 128 insertions(+), 0 deletions(-)
> 

Overall, this patch needed a bit tweaking just to be able to apply it.
If you can make your e-mail client to not wrap long line it would be
perfect. Or just use git send-email.

> diff --git a/tools/virsh.c b/tools/virsh.c
> index 4d34d49..88d4681 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -13624,6 +13624,133 @@ cmdSysinfo (vshControl *ctl, const vshCmd
> *cmd ATTRIBUTE_UNUSED)
>  }
> 
>  /*
> + * "display" command
> + */
> +static const vshCmdInfo info_domdisplay[] = {
> +    {"help", N_("domain display connection URI")},
> +    {"desc", N_("Output the IP address and port number for the
> graphical display.")},
> +    {NULL, NULL}
> +};
> +
> +static const vshCmdOptDef opts_domdisplay[] = {
> +    {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")},
> +    {NULL, 0, 0, NULL}
> +};
> +
> +static bool
> +cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
> +{
> +    xmlDocPtr xml = NULL;
> +    xmlXPathObjectPtr obj = NULL;
> +    xmlXPathContextPtr ctxt = NULL;
> +    virDomainPtr dom;
> +    bool ret = false;
> +    int port = 0;
> +    char *doc;
> +    char *xpath;
> +    const char *scheme[] = { "vnc", "spice", NULL };
> +    int iter = 0t
> +
> +    if (!vshConnectionUsability(ctl, ctl->conn))
> +        return false;
> +
> +    if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
> +        return false;
> +

if (!virDomainIsActive(dom)) {
    vshError(ctl, _("Domain is not running"));
    goto cleanup;
}

rather than [1]
> +    doc = virDomainGetXMLDesc(dom, 0);
> +    if (!doc)
> +        goto cleanup;
> +
> +    xml = virXMLParseStringCtxt(doc, _("(domain_definition)"), &ctxt);
> +    VIR_FREE(doc);
> +    if (!xml)
> +        goto cleanup;
> +
> +    /* Attempt to grab our display info */
> +    for (iter = 0; scheme[iter] != NULL; iter++) {
> +        /* Create our XPATH lookup for the current display */
> +        virAsprintf(&xpath, "string(/domain/devices/graphics[@type='%s']"
> +                "/@port)", scheme[iter]);
> +        if (!xpath) {
> +            virReportOOMError();
> +            goto cleanup;
> +        }
> +
> +        obj = xmlXPathEval(BAD_CAST xpath, ctxt);

we prefer virXPathLong which does all the checks and returns just long.

> +        VIR_FREE(xpath);
> +        if (obj == NULL || obj->type != XPATH_STRING ||
> +                obj->stringval == NULL) {
> +            if (obj) {
> +                /* Clean up memory */
> +                xmlXPathFreeObject(obj);
> +                obj = NULL;
> +            }
> +            continue;
> +        }
> +
> +        /* If there was a port number, then the guest uses the
> current scheme */
> +        if (obj->stringval[0] != 0) {
> +
> +            /* Make sure this is a valid port number */
> +            if (virStrToLong_i((const char *)obj->stringval, NULL,
> 10, &port)) {
> +                vshError(ctl, _("Unable to interpret '%s' as a port number."),
> +                        obj->stringval);
> +                goto cleanup;
> +            }
> +
> +            if (port < 0) 

...[1] this:

> +                vshError(ctl, _("Invalid port '%d', guest likely not
> started."),
> +                        port);
> +                goto cleanup;
> +            }
> +
> +            /* Clean up memory */
> +            xmlXPathFreeObject(obj);
> +            obj = NULL;
> +
> +            virAsprintf(&xpath, "string(/domain/devices/graphics[@type='%s']"
> +                    "/@listen)", scheme[iter]);
> +            if (!xpath) {
> +                virReportOOMError();
> +                goto cleanup;
> +            }
> +
> +            obj = xmlXPathEval(BAD_CAST xpath, ctxt);
> +            VIR_FREE(xpath);
> +
> +            /* VNC protocol handlers take their port number as X - 5900 */
> +            if (scheme[iter] == "vnc")

if (STREQ(scheme[iter], "vnc"))

> +                port -= 5900;
> +
> +            if (obj == NULL || obj->type != XPATH_STRING ||
> +                obj->stringval == NULL || obj->stringval[0] == 0 ||
> +                STREQ((const char *)obj->stringval, "0.0.0.0")) {
> +                vshPrint(ctl, "%s://localhost:%d\n", scheme[iter], port);
> +            } else {
> +                vshPrint(ctl, "%s://%s:%d\n", scheme[iter],
> +                        (const char  *)obj->stringval, port);
> +            }
> +
> +            /* Clean up memory */
> +            xmlXPathFreeObject(obj);
> +            obj = NULL;
> +
> +            /* We got what we came for so return successfully */
> +            ret = true;
> +            break;
> +        }
> +
> +    }
> +
> +cleanup:
> +    xmlXPathFreeObject(obj);
> +    xmlXPathFreeContext(ctxt);
> +    xmlFreeDoc(xml);
> +    virDomainFree(dom);
> +    return ret;
> +}
> +
> +/*
>   * "vncdisplay" command
>   */
>  static const vshCmdInfo info_vncdisplay[] = {
> @@ -17702,6 +17829,7 @@ static const vshCmdDef domManagementCmds[] = {
>      {"detach-disk", cmdDetachDisk, opts_detach_disk, info_detach_disk, 0},
>      {"detach-interface", cmdDetachInterface, opts_detach_interface,
>       info_detach_interface, 0},
> +    {"domdisplay", cmdDomDisplay, opts_domdisplay, info_domdisplay, 0},
>      {"domid", cmdDomid, opts_domid, info_domid, 0},
>      {"domif-setlink", cmdDomIfSetLink, opts_domif_setlink,
> info_domif_setlink, 0},
>      {"domiftune", cmdDomIftune, opts_domiftune, info_domiftune, 0},
> 

And you need to update tools/virsh.pod as well.
I think using libvirt wrappers for XML/XPath will make this patch much
smaller. I like the idea though.

Michal