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

Re: [libvirt] [PATCH v2 2/2] qemu: Add VNC WebSocket support



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 redhat com>
>> ---
>>  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


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