Re: [Qemu-devel] [PATCH v3 2/3] chardev: Add websocket support

[Daniel P . Berrangé]

On Fri, Oct 19, 2018 at 02:15:10PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Oct 19, 2018 at 2:35 AM Julia Suvorova <address@hidden> wrote:
> >
> > New option "websocket" added to allow using WebSocket protocol for
> > chardev socket backend.
> > Example:
> >     -chardev socket,websocket,server,id=...
> >
> > Signed-off-by: Julia Suvorova <address@hidden>
> > ---
> >  chardev/char-socket.c | 64 ++++++++++++++++++++++++++++++++++++++-----
> >  chardev/char.c        |  8 +++++-
> >  qapi/char.json        |  3 ++
> >  qemu-options.hx       | 13 +++++++--
> >  4 files changed, 77 insertions(+), 11 deletions(-)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index effde65a89..ba4ae9dfb0 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"
> > @@ -68,6 +69,8 @@ typedef struct {
> >      GSource *telnet_source;
> >      TCPChardevTelnetInit *telnet_init;
> >
> > +    bool is_websock;
> > +
> >      GSource *reconnect_timer;
> >      int64_t reconnect_time;
> >      bool connect_err_reported;
> > @@ -394,7 +397,7 @@ static const char 
> > *qemu_chr_socket_protocol(SocketChardev *s)
> >      if (s->is_telnet) {
> >          return "telnet";
> >      }
> > -    return "tcp";
> > +    return s->is_websock ? "websocket" : "tcp";
> >  }
> >
> >  static char *qemu_chr_socket_address(SocketChardev *s, const char *prefix)
> > @@ -714,6 +717,41 @@ cont:
> >  }
> >
> >
> > +static void tcp_chr_websock_handshake(QIOTask *task, gpointer user_data)
> > +{
> > +    Chardev *chr = user_data;
> > +    SocketChardev *s = user_data;
> > +
> > +    if (qio_task_propagate_error(task, NULL)) {
> > +        tcp_chr_disconnect(chr);
> 
> What about error_report() the error?

None of the rest of the code (telnet, tls, and other i/o related
funcs) uses error_report(). In some ways this is good as it
prevents a client app from flooding the qemu log file. It does
make debugging harder though when things are not working as
expected.

I wonder if we should add a tracing probe point in the
error_setg* methods in util/error.c ? There's quite a few
places in QEMU that discard errors which could benefit from
this when debugging.

I don't think it needs to blck this patch though.

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