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

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(a)cardoe.com&gt; > --- > 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.