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

[Daniel P . Berrangé]

On Tue, Aug 28, 2018 at 01:18:19PM +0300, Julia Suvorova wrote:
> On 28.08.2018 13:09, Daniel P. Berrangé wrote:
> > On Tue, Aug 28, 2018 at 01:04:41PM +0300, Julia Suvorova wrote:
> > > 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 <address@hidden>
> > > > > ---
> > > > >    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.
> > 
> > Don't really know why you're saying the first patch code won't be used.
> > The code you're refactoring here is already used today, and you are just
> > renaming methods & changing what parameters are passed to it.
> 
> I meant that some code will be replaced with a second patch.
> For instance, in the first patch I will add this line:
> +    return "tcp";
> And in the second it will be replaced by:
> +    return s->is_websock ? "websock" : "tcp";
> So this is extra unnecessary code.

That is to be expected and isn't really considered unnecessary code.
It is best practice when separating work into a series of commits to
ensure each commit only covers one distinct task.

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 :|