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

[Paolo Bonzini]

Thanks Julia, just a few cleanups to simplify the prototypes of some
functions.

On 13/08/2018 12:20, Julia Suvorova wrote:
> New option "websock" added to allow using websocket protocol for
> chardev socket backend.
> Example:
>     -chardev socket,websock,id=...
> 
> Signed-off-by: Julia Suvorova <address@hidden>
> ---
>  chardev/char-socket.c | 75 ++++++++++++++++++++++++++++++++++++-------
>  chardev/char.c        |  3 ++
>  qapi/char.json        |  3 ++
>  3 files changed, 70 insertions(+), 11 deletions(-)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index efbad6ee7c..4464446511 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -26,6 +26,7 @@
>  #include "chardev/char.h"
>  #include "io/channel-socket.h"
>  #include "io/channel-tls.h"
> +#include "io/channel-websock.h"
>  #include "io/net-listener.h"
>  #include "qemu/error-report.h"
>  #include "qemu/option.h"
> @@ -69,6 +70,8 @@ typedef struct {
>      GSource *telnet_source;
>      TCPChardevTelnetInit *telnet_init;
>  
> +    bool is_websock;
> +
>      GSource *reconnect_timer;
>      int64_t reconnect_time;
>      bool connect_err_reported;
> @@ -386,12 +389,14 @@ static void tcp_chr_free_connection(Chardev *chr)
>  }
>  
>  static char *SocketAddress_to_str(const char *prefix, SocketAddress *addr,
> -                                  bool is_listen, bool is_telnet)
> +                                  bool is_listen, bool is_telnet,
> +                                  bool is_websock)

The only caller passes the four arguments as "s->addr, s->is_listen,
s->is_telner, s->is_websock".  Can you change the arguments to
"SocketChardev *s, const char *prefix" and rename to
qemu_chr_socket_address?  Or even remove "const char *prefix" and rename
to "qemu_chr_compute_disconnected_filename", as you prefer.

>  {
>      switch (addr->type) {
>      case SOCKET_ADDRESS_TYPE_INET:
>          return g_strdup_printf("%s%s:%s:%s%s", prefix,
> -                               is_telnet ? "telnet" : "tcp",
> +                               is_telnet ? "telnet"
> +                                         : (is_websock ? "websock" : "tcp"),
>                                 addr->u.inet.host,
>                                 addr->u.inet.port,
>                                 is_listen ? ",server" : "");
> @@ -420,7 +425,8 @@ static void update_disconnected_filename(SocketChardev *s)
>  
>      g_free(chr->filename);
>      chr->filename = SocketAddress_to_str("disconnected:", s->addr,
> -                                         s->is_listen, s->is_telnet);
> +                                         s->is_listen, s->is_telnet,
> +                                         s->is_websock);
>  }
>  
>  /* NB may be called even if tcp_chr_connect has not been
> @@ -508,7 +514,7 @@ static int tcp_chr_sync_read(Chardev *chr, const uint8_t 
> *buf, int len)
>  
>  static char *sockaddr_to_str(struct sockaddr_storage *ss, socklen_t ss_len,
>                               struct sockaddr_storage *ps, socklen_t ps_len,
> -                             bool is_listen, bool is_telnet)
> +                             bool is_listen, bool is_telnet, bool is_websock)

Likewise here, the function only needs a single argument SocketChardev
*s and can be renamed to qemu_chr_compute_filename (since the result is
assigned to chr->filename).

>  {
>      char shost[NI_MAXHOST], sserv[NI_MAXSERV];
>      char phost[NI_MAXHOST], pserv[NI_MAXSERV];
> @@ -531,7 +537,8 @@ static char *sockaddr_to_str(struct sockaddr_storage *ss, 
> socklen_t ss_len,
>          getnameinfo((struct sockaddr *) ps, ps_len, phost, sizeof(phost),
>                      pserv, sizeof(pserv), NI_NUMERICHOST | NI_NUMERICSERV);
>          return g_strdup_printf("%s:%s%s%s:%s%s <-> %s%s%s:%s",
> -                               is_telnet ? "telnet" : "tcp",
> +                               is_telnet ? "telnet"
> +                                         : (is_websock ? "websock" : "tcp"),

... and finally add a new function
qemu_chr_socket_protocol(SocketChardev *s) that returns one of "telnet",
"websock" or "tcp".

> diff --git a/qapi/char.json b/qapi/char.json
> index b7b2a05766..135ccddaf7 100644
> --- a/qapi/char.json
> +++ b/qapi/char.json
> @@ -251,6 +251,8 @@
>  #          sockets (default: false)
>  # @tn3270: enable tn3270 protocol on server
>  #          sockets (default: false) (Since: 2.10)
> +# @websock: enable websocket protocol on server
> +#           sockets (default: false)

"(Since: 3.1)".

Thanks,

Paolo

>  # @reconnect: For a client socket, if a socket is disconnected,
>  #          then attempt a reconnect after the given number of seconds.
>  #          Setting this to zero disables this function. (default: 0)
> @@ -265,6 +267,7 @@
>                                       '*nodelay'   : 'bool',
>                                       '*telnet'    : 'bool',
>                                       '*tn3270'    : 'bool',
> +                                     '*websock'   : 'bool',
>                                       '*reconnect' : 'int' },
>    'base': 'ChardevCommon' }
>  
>