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

Re: [libvirt] [PATCH v2] virsh: Add domdisplay cmd for VNC, SPICE and RDP



On Wed, Jun 27, 2012 at 03:21:48PM +0200, Michal Privoznik wrote:
> On 24.06.2012 23:14, Doug Goldstein wrote:
> > v2:
> > - Refactored to use virBuffer
> > - Refactored to use virXPath wrappers
> > - Added support for tls-port and password for SPICE
> > - Added optional flag to disable SPICE password to the URI
> > - Added support for RDP
> > - Fixed code reviews
> > 
> > Add a new 'domdisplay' command that provides a URI for VNC, SPICE and
> > RDP connections. Presently the 'vncdisplay' command provides you with
> > the port info that QEMU is listening on but there is no counterpart for
> > SPICE and RDP. Additionally this provides you with the bind address as
> > specified in the XML, which the existing 'vncdisplay' lacks. For SPICE
> > connections it supports secure and unsecure channels and optionally
> > providing the password for the SPICE channel.
> > 
> > Signed-off-by: Doug Goldstein <cardoe cardoe com>
> > ---
> >  tools/virsh.c   |  173 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  tools/virsh.pod |    6 ++
> >  2 files changed, 179 insertions(+), 0 deletions(-)
> > 
> > diff --git a/tools/virsh.c b/tools/virsh.c
> > index 0354822..da80477 100644
> > --- a/tools/virsh.c
> > +++ b/tools/virsh.c
> > @@ -13827,6 +13827,178 @@ cmdSysinfo (vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
> >  }
> >  
> >  /*
> > + * "display" command
> > + */
> 
> In fact it's domdisplay 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")},
> > +    {"include-password", VSH_OT_BOOL, VSH_OFLAG_NONE, N_("includes the password into the connection URI if available")},
> > +    {NULL, 0, 0, NULL}
> > +};
> > +
> > +static bool
> > +cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
> > +{
> > +    xmlDocPtr xml = NULL;
> > +    xmlXPathContextPtr ctxt = NULL;
> > +    virDomainPtr dom;
> > +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> > +    bool ret = false;
> > +    char *doc;
> > +    char *xpath;
> > +    char *listen_addr;
> > +    int port, tls_port = 0;
> > +    char *passwd = NULL;
> > +    const char *scheme[] = { "vnc", "spice", "rdp", NULL };
> > +    int iter = 0;
> > +    int tmp;
> > +
> > +    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;
> > +    }
> > +
> > +    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's port */
> > +        virAsprintf(&xpath, "string(/domain/devices/graphics[ type='%s']"
> > +                "/@port)", scheme[iter]);
> > +        if (!xpath) {
> > +            virReportOOMError();
> > +            goto cleanup;
> > +        }
> > +
> > +        /* 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
> > +         */
> > +        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;
> > +        }
> > +
> > +        /* Attempt to get the listening addr if set for the current
> > +         * graphics scheme
> > +         */
> > +        listen_addr = virXPathString(xpath, ctxt);
> > +        VIR_FREE(xpath);
> > +    
> 
> Adding a blank line ^^
> 
> > +        /* Per scheme data mangling */
> > +        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, "daemon")) {
> 
> s/daemon/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]);
> > +
> > +        /* Then host name or IP */
> > +        if (!listen_addr || STREQ((const char *)listen_addr, "0.0.0.0"))
> > +            virBufferAddLit(&buf, "localhost");
> > +        else
> > +            virBufferAsprintf(&buf, "%s", listen_addr);
> > +
> > +        /* Clean up our memory */
> > +        if (listen_addr)
> > +            VIR_FREE(listen_addr);
> 
> This check is redundant. free(NULL) is safe.
> 
> > +
> > +        /* Add the port */
> > +        if (STREQ(scheme[iter], "spice"))
> > +            virBufferAsprintf(&buf, "?port=%d", port);
> > +        else
> > +            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);
> > +        }
> > +
> > +        /* Ensure we can print our URI */
> > +        if (virBufferError(&buf)) {
> > +            vshPrint(ctl, "%s", _("Failed to create display URI"));
> > +            goto cleanup;
> > +        }
> > +
> > +        /* 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;
> > +        break;
> > +    }
> > +
> > +cleanup:
> > +    xmlXPathFreeContext(ctxt);
> > +    xmlFreeDoc(xml);
> > +    virDomainFree(dom);
> > +    return ret;
> > +}
> > +
> > +/*
> >   * "vncdisplay" command
> >   */
> >  static const vshCmdInfo info_vncdisplay[] = {
> > @@ -17974,6 +18146,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},
> > diff --git a/tools/virsh.pod b/tools/virsh.pod
> > index f83a29d..558105f 100644
> > --- a/tools/virsh.pod
> > +++ b/tools/virsh.pod
> > @@ -820,6 +820,12 @@ I<size> is a scaled integer (see B<NOTES> above) which defaults to KiB
> >  "B" to get bytes (note that for historical reasons, this differs from
> >  B<vol-resize> which defaults to bytes without a suffix).
> >  
> > +=item B<domdisplay> I<domain-id> [I<--include-password>]
> > +
> > +Output a URI which can be used to connect to the graphical display of the
> > +domain via VNC, SPICE or RDP. If I<--include-password> is specified, the
> > +SPICE channel password will be included in the URI.
> > +
> >  =item B<dominfo> I<domain-id>
> >  
> >  Returns basic information about the domain.
> > 
> 
> Otherwise looking good. ACK. I would pushed this but we are in freeze
> phase. On the other hand - v1 was sent before freeze. Combined with fact
> that number of patches sent into the list has fallen down I guess we can
> push this in. But I'd like to hear a second opinion.

  it only touches virsh, so the risks are limited. And overall a new
  command is really low risk too, so ACK, let's push it now.

I will probably make the rc2 in 12hours if you could push before fine
:-)

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


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