Re: [Qemu-devel] [PATCH] chardev: Add websocket support

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Aug 13, 2018 at 01:20:37PM +0300, Julia Suvorova via Qemu-devel wrote:
> New option "websock" added to allow using websocket protocol for
> chardev socket backend.
> Example:
>     -chardev socket,websock,id=...
> 
> Signed-off-by: Julia Suvorova <jusual@mail.ru>
> ---
>  chardev/char-socket.c | 75 ++++++++++++++++++++++++++++++++++++-------
>  chardev/char.c        |  3 ++
>  qapi/char.json        |  3 ++
>  3 files changed, 70 insertions(+), 11 deletions(-)
> 


> @@ -699,6 +706,45 @@ cont:
>  }
>  
>  
> +static void tcp_chr_websock_handshake(QIOTask *task, gpointer user_data)
> +{
> +    Chardev *chr = user_data;
> +
> +    if (qio_task_propagate_error(task, NULL)) {
> +        tcp_chr_disconnect(chr);
> +    } else {
> +        tcp_chr_connect(chr);
> +    }
> +}
> +
> +
> +static void tcp_chr_websock_init(Chardev *chr)
> +{
> +    SocketChardev *s = SOCKET_CHARDEV(chr);
> +    QIOChannelWebsock *wioc;
> +    gchar *name;
> +
> +    if (s->is_listen) {
> +        wioc = qio_channel_websock_new_server(s->ioc);
> +    } else {
> +        /* Websocket client is not yet implemented */
> +        return;
> +    }
> +    if (wioc == NULL) {
> +        tcp_chr_disconnect(chr);
> +        return;
> +    }
> +
> +    name = g_strdup_printf("chardev-websock-server-%s", chr->label);
> +    qio_channel_set_name(QIO_CHANNEL(wioc), name);
> +    g_free(name);
> +    object_unref(OBJECT(s->ioc));
> +    s->ioc = QIO_CHANNEL(wioc);
> +
> +    qio_channel_websock_handshake(wioc, tcp_chr_websock_handshake, chr, NULL);
> +}
> +
> +
>  static void tcp_chr_tls_handshake(QIOTask *task,
>                                    gpointer user_data)
>  {
> @@ -710,6 +756,8 @@ static void tcp_chr_tls_handshake(QIOTask *task,
>      } else {
>          if (s->do_telnetopt) {
>              tcp_chr_telnet_init(chr);
> +        } else if (s->is_websock) {
> +            tcp_chr_websock_init(chr);
>          } else {
>              tcp_chr_connect(chr);
>          }
> @@ -799,12 +847,12 @@ static int tcp_chr_new_client(Chardev *chr, QIOChannelSocket *sioc)
>  
>      if (s->tls_creds) {
>          tcp_chr_tls_init(chr);
> +    } else if (s->do_telnetopt) {
> +        tcp_chr_telnet_init(chr);
> +    } else if (s->is_websock) {
> +        tcp_chr_websock_init(chr);
>      } else {
> -        if (s->do_telnetopt) {
> -            tcp_chr_telnet_init(chr);
> -        } else {
> -            tcp_chr_connect(chr);
> -        }
> +        tcp_chr_connect(chr);


I don't think the ordering of init calls is correct when we have multiple options
enabled.

With tls=y & telnet=y we initialize TLS, then initialize telnet.

With tls=y & telnet=y & websock=y, we  initialize TLS, then initialize telnet and
then websock.

This means we're setting up a telnet session, and then running websocket over the top
of it.  This is wrong because there's no telnet client that exists which would be
able to use that.

The purpose of TLS & websock options in combination with telnet is to allow a normal
telnet session to be tunnelled over a TLS+websocket proxy.

So we must initailize TLS, then websock, then telnet


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|