On 04/30/2013 06:53 PM, John Ferlan wrote:
> On 04/30/2013 10:42 AM, Martin Kletzander wrote:
>> Adding a VNC WebSocket support for QEMU driver. This funcitonality is
>
> s/funcitonality/functionality
>
>> in upstream qemu from commit described as v1.3.0-982-g7536ee4, so the
>> capability is being recognized based on QEMU version for now.
>>
>> Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
>> ---
>> src/qemu/libvirtd_qemu.aug | 2 ++
>> src/qemu/qemu.conf | 7 +++++
>> src/qemu/qemu_capabilities.c | 11 +++++--
>> src/qemu/qemu_capabilities.h | 1 +
>> src/qemu/qemu_command.c | 60 ++++++++++++++++++++++++++++++++++++--
>> src/qemu/qemu_command.h | 5 +++-
>> src/qemu/qemu_conf.c | 32 ++++++++++++++++++++
>> src/qemu/qemu_conf.h | 6 ++++
>> src/qemu/qemu_driver.c | 5 ++++
>> src/qemu/qemu_process.c | 31 ++++++++++++++++----
>> src/qemu/test_libvirtd_qemu.aug.in | 2 ++
>> tests/qemuargv2xmltest.c | 1 +
>> tests/qemuxml2argvtest.c | 1 +
>> tests/qemuxml2xmltest.c | 1 +
>> 14 files changed, 153 insertions(+), 12 deletions(-)
>>
>> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
>> index a3dcb30..5344125 100644
>> --- a/src/qemu/libvirtd_qemu.aug
>> +++ b/src/qemu/libvirtd_qemu.aug
>> @@ -41,6 +41,8 @@ module Libvirtd_qemu =
>>
>> let remote_display_entry = int_entry "remote_display_port_min"
>> | int_entry "remote_display_port_max"
>> + | int_entry "remote_websocket_port_min"
>> + | int_entry "remote_websocket_port_max"
>>
>> let security_entry = str_entry "security_driver"
>> | bool_entry "security_default_confined"
>> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
>> index 0f0a24c..9257d3d 100644
>> --- a/src/qemu/qemu.conf
>> +++ b/src/qemu/qemu.conf
>> @@ -153,6 +153,13 @@
>> #remote_display_port_min = 5900
>> #remote_display_port_max = 65535
>>
>> +# VNC WebSocket port policies, same rules apply as with remote display
>> +# ports. VNC WebSockets use similar display <-> port mappings, with
>> +# the exception being that ports starts from 5700 instead of 5900.
>> +# This is what may have be changed here.
>> +#
>> +#remote_websocket_port_min = 5700
>> +#remote_websocket_port_max = 65535
>>
>> # The default security driver is SELinux. If SELinux is disabled
>> # on the host, then the security driver will automatically disable
>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>> index 2acf535..9e5eedf 100644
>> --- a/src/qemu/qemu_capabilities.c
>> +++ b/src/qemu/qemu_capabilities.c
>> @@ -222,9 +222,10 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>> "tpm-tis",
>>
>> "nvram", /* 140 */
>> - "pci-bridge", /* 141 */
>> - "vfio-pci", /* 142 */
>> - "vfio-pci.bootindex", /* 143 */
>> + "pci-bridge",
>> + "vfio-pci",
>> + "vfio-pci.bootindex",
>> + "vnc-websocket",
>
> This list doesn't match below... I also remember reading a comment
> recently about counting every 5th and leaving proper spacing between
> groups of 5.
>
This is actually counting only every 5th (but starting with 0, so it's
the "nvram" /* 140 */ here.
>> );
>>
>> struct _virQEMUCaps {
>> @@ -2520,6 +2521,10 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
>> if (qemuCaps->version >= 1003000)
>> virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACHINE_USB_OPT);
>>
>> + /* WebSockets were introduced between 1.3.0 and 1.3.1 */
>> + if (qemuCaps->version >= 1003001)
>> + virQEMUCapsSet(qemuCaps, QEMU_CAPS_VNC_WEBSOCKET);
>> +
>> if (!(archstr = qemuMonitorGetTargetArch(mon)))
>> goto cleanup;
>>
>> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
>> index 213f63c..c647274 100644
>> --- a/src/qemu/qemu_capabilities.h
>> +++ b/src/qemu/qemu_capabilities.h
>> @@ -182,6 +182,7 @@ enum virQEMUCapsFlags {
>> QEMU_CAPS_DEVICE_PCI_BRIDGE = 141, /* -device pci-bridge */
>> QEMU_CAPS_DEVICE_VFIO_PCI = 142, /* -device vfio-pci */
>> QEMU_CAPS_VFIO_PCI_BOOTINDEX = 143, /* bootindex param for vfio-pci device */
>> + QEMU_CAPS_VNC_WEBSOCKET = 144, /* bootindex param for vfio-pci device */
>
> This doesn't match above
>>
>> QEMU_CAPS_LAST, /* this must always be the last item */
>> };
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 3184e5b..f718434 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -5903,6 +5903,17 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
>> }
>> }
>>
>> + if (graphics->data.vnc.websocket) {
>> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNC_WEBSOCKET)) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("VNC websockets are not supported "
>
> Should it be "WebSockets" or does it matter?
>
I think it doesn't matter, but I changed it to WebSocket everywhere.
>> + "with this QEMU binary"));
>> + goto error;
>> + }
>> +
>> + virBufferAsprintf(&opt, ",websocket=%d", graphics->data.vnc.websocket);
>> + }
>> +
>> virCommandAddArg(cmd, "-vnc");
>> virCommandAddArgBuffer(cmd, &opt);
>> if (graphics->data.vnc.keymap)
>> @@ -9726,6 +9737,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps,
>> * -vnc some.host.name:4
>> */
>> char *opts;
>> + char *port;
>> const char *sep = ":";
>> if (val[0] == '[')
>> sep = "]:";
>> @@ -9736,11 +9748,12 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps,
>> _("missing VNC port number in '%s'"), val);
>> goto error;
>> }
>> - if (virStrToLong_i(tmp+strlen(sep), &opts, 10,
>> + port = tmp + strlen(sep);
>> + if (virStrToLong_i(port, &opts, 10,
>> &vnc->data.vnc.port) < 0) {
>> virDomainGraphicsDefFree(vnc);
>> virReportError(VIR_ERR_INTERNAL_ERROR,
>> - _("cannot parse VNC port '%s'"), tmp+1);
>> + _("cannot parse VNC port '%s'"), port);
>> goto error;
>> }
>> if (val[0] == '[')
>> @@ -9753,6 +9766,49 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps,
>> virDomainGraphicsDefFree(vnc);
>> goto no_memory;
>> }
>> +
>> + if (*opts == ',') {
>> + char *orig_opts = strdup(opts + 1);
>> + if (!orig_opts) {
>> + virDomainGraphicsDefFree(vnc);
>> + goto no_memory;
>> + }
>> + opts = orig_opts;
>> +
>> + while (opts && *opts) {
>> + char *nextopt = strchr(opts, ',');
>> + if (nextopt)
>> + *(nextopt++) = '\0';
>> +
>> + if (STRPREFIX(opts, "websocket")) {
>> + char *websocket = opts + strlen("websocket");
>> + if (*(websocket++) == '=' &&
>> + *websocket) {
>> + /* If the websocket continues with
>> + * '=<something>', we'll parse it */
>> + if (virStrToLong_i(websocket,
>> + NULL, 0,
>> + &vnc->data.vnc.websocket) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("cannot parse VNC "
>> + "websocket port '%s'"),
>
> Do we care to make it "WebSocket"?
>
Same here.
>> + websocket);
>> + virDomainGraphicsDefFree(vnc);
>> + VIR_FREE(orig_opts);
>> + }
>> + } else {
>> + /* Otherwise, we'll compute the port the same
>> + * way QEMU does, by adding a 5700 to the
>> + * display value. */
>> + vnc->data.vnc.websocket =
>> + vnc->data.vnc.port + 5700;
>
> s/5700/QEMU_WEBSOCKET_PORT_MIN
>
> I see there is a QEMU_REMOTE_PORT_MIN in qemu_command.h, but it's not
> used in qemu_command.c either....
>
Actually no. This is parsing the command line of qemu, which always
adds 5700, but not our minimum.
I failed to see your mail because you didn't reply-all, noticed it now.
I'll go through Eric's comments and push afterwards.
Martin
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list