On 27.06.2012 15:28, Daniel Veillard wrote:
> 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@xxxxxxxxxx>
>>> ---
>>> 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
>
Okay, pushed now. Thanks.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list