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(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. 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