Re: [Qemu-devel] [PATCH v2 1/2] chardev: Add websocket support

[Julia Suvorova <jusual@mail.ru>]

On 28.08.2018 12:02, Daniel P. Berrangé wrote:
> On Mon, Aug 27, 2018 at 09:41:02PM +0300, 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 <jusual@mail.ru>
>> ---
>>   chardev/char-socket.c | 124 +++++++++++++++++++++++++++++++-----------
>>   chardev/char.c        |   8 ++-
>>   qapi/char.json        |   3 +
>>   3 files changed, 103 insertions(+), 32 deletions(-)
> 
> Also needs changes to qemu-options.hx to document the new
> websock option.  This doc ends up included in the main
> qemu-doc.html published on the website, and also forms the
> man page for qemu.
> 
>> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>> index efbad6ee7c..1d3c14d85d 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;
>> @@ -385,30 +388,37 @@ static void tcp_chr_free_connection(Chardev *chr)
>>       s->connected = 0;
>>   }
>>   
>> -static char *SocketAddress_to_str(const char *prefix, SocketAddress *addr,
>> -                                  bool is_listen, bool is_telnet)
>> +static const char *qemu_chr_socket_protocol(SocketChardev *s)
>> +{
>> +    if (s->is_telnet) {
>> +        return "telnet";
>> +    }
>> +    return s->is_websock ? "websock" : "tcp";
>> +}
>> +
>> +static char *qemu_chr_socket_address(SocketChardev *s, const char *prefix)
>>   {
>> -    switch (addr->type) {
>> +    switch (s->addr->type) {
>>       case SOCKET_ADDRESS_TYPE_INET:
>>           return g_strdup_printf("%s%s:%s:%s%s", prefix,
>> -                               is_telnet ? "telnet" : "tcp",
>> -                               addr->u.inet.host,
>> -                               addr->u.inet.port,
>> -                               is_listen ? ",server" : "");
>> +                               qemu_chr_socket_protocol(s),
>> +                               s->addr->u.inet.host,
>> +                               s->addr->u.inet.port,
>> +                               s->is_listen ? ",server" : "");
>>           break;
>>       case SOCKET_ADDRESS_TYPE_UNIX:
>>           return g_strdup_printf("%sunix:%s%s", prefix,
>> -                               addr->u.q_unix.path,
>> -                               is_listen ? ",server" : "");
>> +                               s->addr->u.q_unix.path,
>> +                               s->is_listen ? ",server" : "");
>>           break;
>>       case SOCKET_ADDRESS_TYPE_FD:
>> -        return g_strdup_printf("%sfd:%s%s", prefix, addr->u.fd.str,
>> -                               is_listen ? ",server" : "");
>> +        return g_strdup_printf("%sfd:%s%s", prefix, s->addr->u.fd.str,
>> +                               s->is_listen ? ",server" : "");
>>           break;
>>       case SOCKET_ADDRESS_TYPE_VSOCK:
>>           return g_strdup_printf("%svsock:%s:%s", prefix,
>> -                               addr->u.vsock.cid,
>> -                               addr->u.vsock.port);
>> +                               s->addr->u.vsock.cid,
>> +                               s->addr->u.vsock.port);
>>       default:
>>           abort();
>>       }
>> @@ -419,8 +429,7 @@ static void update_disconnected_filename(SocketChardev *s)
>>       Chardev *chr = CHARDEV(s);
>>   
>>       g_free(chr->filename);
>> -    chr->filename = SocketAddress_to_str("disconnected:", s->addr,
>> -                                         s->is_listen, s->is_telnet);
>> +    chr->filename = qemu_chr_socket_address(s, "disconnected:");
>>   }
>>   
>>   /* NB may be called even if tcp_chr_connect has not been
>> @@ -506,10 +515,12 @@ static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len)
>>       return size;
>>   }
>>   
>> -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)
>> +static char *qemu_chr_compute_filename(SocketChardev *s)
>>   {
>> +    struct sockaddr_storage *ss = &s->sioc->localAddr;
>> +    struct sockaddr_storage *ps = &s->sioc->remoteAddr;
>> +    socklen_t ss_len = s->sioc->localAddrLen;
>> +    socklen_t ps_len = s->sioc->remoteAddrLen;
>>       char shost[NI_MAXHOST], sserv[NI_MAXSERV];
>>       char phost[NI_MAXHOST], pserv[NI_MAXSERV];
>>       const char *left = "", *right = "";
>> @@ -519,7 +530,7 @@ static char *sockaddr_to_str(struct sockaddr_storage *ss, socklen_t ss_len,
>>       case AF_UNIX:
>>           return g_strdup_printf("unix:%s%s",
>>                                  ((struct sockaddr_un *)(ss))->sun_path,
>> -                               is_listen ? ",server" : "");
>> +                               s->is_listen ? ",server" : "");
>>   #endif
>>       case AF_INET6:
>>           left  = "[";
>> @@ -531,9 +542,9 @@ 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",
>> +                               qemu_chr_socket_protocol(s),
>>                                  left, shost, right, sserv,
>> -                               is_listen ? ",server" : "",
>> +                               s->is_listen ? ",server" : "",
>>                                  left, phost, right, pserv);
>>   
>>       default:
>> @@ -547,10 +558,7 @@ static void tcp_chr_connect(void *opaque)
>>       SocketChardev *s = SOCKET_CHARDEV(opaque);
>>   
>>       g_free(chr->filename);
>> -    chr->filename = sockaddr_to_str(
>> -        &s->sioc->localAddr, s->sioc->localAddrLen,
>> -        &s->sioc->remoteAddr, s->sioc->remoteAddrLen,
>> -        s->is_listen, s->is_telnet);
>> +    chr->filename = qemu_chr_compute_filename(s);
>>   
>>       s->connected = 1;
>>       chr->gsource = io_add_watch_poll(chr, s->ioc,
> 
> Everything above this point is refactoring of existing code, mixed in with
> a few websock additions.
> 
> Everything below this is implementing the websocket feature.
> 
> These two distinct tasks should be separated, so that refactoring is in
> the first patch, and websocks impl is in a second patch.

Thus, I will have second patch based on the first patch, and the
first will contain code that will never be used. If it's okay, I will
divide the patch.

Best regards, Julia Suvorova.