Re: [libvirt] [PATCH v3 2/2] qemu: Add VNC WebSocket support
On 05/14/2013 09:15 AM, Guannan Ren wrote:
On 05/13/2013 09:10 PM, Martin Kletzander wrote:
[...]
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index eddc263..7d80e74 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6070,6 +6070,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 "
> + "with this QEMU binary"));
> + goto error;
> + }
> +
> + virBufferAsprintf(&opt, ",websocket=%d",
> graphics->data.vnc.websocket);
> + }
> +
I think the above block should be moved in
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNC_COLON)) {
if (graphics->data.vnc.websocket)
...
}
That would mean there will be no error reported in case WebSocket is
requested with old qemu, we just won't format the option. But that's
what we do with some other options as well and it should also be added
only if vnc is listening on an open port.
>
> @@ -9915,6 +9928,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'"),
> + websocket);
> + virDomainGraphicsDefFree(vnc);
> + VIR_FREE(orig_opts);
missing goto error, but still nice parsing
The rest is good for me. I use noVNC to connect it, it works well.
Right now, the 'autoport' attribute only manages automatic port allocation for
'port'.
'websocket' doesn't use it, -1 means auto port allocation itself.
If we should change doc to avoid confusing.
ACK with above fixed.
Guannan
I've added the goto, fixed the upper hunk as well and added info into
the documentation about autoport having no effect on websocket.
Thanks, pushed now,
Martin