Re: [Qemu-devel] [PATCH FYI 13/46] io: add QIOChannelWebsock class

[Daniel P. Berrange]

On Mon, Sep 07, 2015 at 04:44:18PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (address@hidden) wrote:
> > Add a QIOChannel subclass that can run the websocket protocol over
> > the top of another QIOChannel instance. This initial implementation
> > is only capable of acting as a websockets server. There is no support
> > for acting as a websockets client yet.
> > 
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> >  include/io/channel-websock.h | 108 +++++
> >  io/Makefile.objs             |   1 +
> >  io/channel-websock.c         | 965 
> > +++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 1074 insertions(+)
> >  create mode 100644 include/io/channel-websock.h
> >  create mode 100644 io/channel-websock.c


> > diff --git a/io/channel-websock.c b/io/channel-websock.c
> > new file mode 100644
> > index 0000000..0345b90
> > --- /dev/null
> > +++ b/io/channel-websock.c
> > @@ -0,0 +1,965 @@
> > +
> > +#include "io/channel-websock.h"
> > +#include "crypto/hash.h"
> > +
> > +#include <glib/gi18n.h>
> > +
> > +/* #define DEBUG_IOC */
> > +
> > +#ifdef DEBUG_IOC
> > +#define DPRINTF(fmt, ...) \
> > +    do { fprintf(stderr, "channel-websock: " fmt, ## __VA_ARGS__); } while 
> > (0)
> > +#else
> > +#define DPRINTF(fmt, ...) \
> > +    do { } while (0)
> > +#endif
> > +
> > +/* Max amount to allow in rawinput/rawoutput buffers */
> > +#define QIO_CHANNEL_WEBSOCK_MAX_BUFFER 8192
> > +
> > +#define B64LEN(__x) (((__x + 2) / 3) * 12 / 3)
> 
> Where is that magic calculation used?

I copied these constants from the current ui/vnc-ws.h header file
originally. In there it was referenced by another constant
WS_ACCEPT_LEN, which was itself unused. I deleted WS_ACCEPT_LEN
when I found it unused, but forgot to delete this B64LEN too.

> > +static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc,
> > +                                              Error **errp)
> > +{
> > +    char *handshake_end;
> > +    ssize_t ret;
> > +    /* Typical HTTP headers from novnc are 512 bytes, so limiting
> > +     * total header size to 4096 is easily enough. */
> > +    size_t want = 4096 - ioc->encinput.offset;
> > +    qio_buffer_reserve(&ioc->encinput, want);
> > +    ret = qio_channel_read(ioc->master,
> > +                           (char *)qio_buffer_end(&ioc->encinput), want, 
> > errp);
> > +    if (ret < 0) {
> > +        return -1;
> > +    }
> > +    ioc->encinput.offset += ret;
> > +
> > +    handshake_end = g_strstr_len((char *)ioc->encinput.buffer,
> > +                                 ioc->encinput.offset,
> > +                                 QIO_CHANNEL_WEBSOCK_HANDSHAKE_END);
> > +    if (!handshake_end) {
> > +        if (ioc->encinput.offset >= 4096) {
> > +            error_setg(errp, "%s",
> > +                       _("End of headers not found in first 4096 bytes"));
> > +            return -1;
> > +        } else {
> > +            return 0;
> > +        }
> > +    }
> > +
> > +    if (qio_channel_websock_handshake_process(ioc,
> > +                                              (char *)ioc->encinput.buffer,
> > +                                              ioc->encinput.offset,
> > +                                              errp) < 0) {
> > +        return -1;
> > +    }
> > +
> > +    qio_buffer_advance(&ioc->encinput,
> > +                       handshake_end - (char *)ioc->encinput.buffer +
> > +                       strlen(QIO_CHANNEL_WEBSOCK_HANDSHAKE_END));
> > +    return 1;
> 
> Can you comment the return values for this function; I guess -1 is error,
> 1 is good, what's 0 ?

1 means we've read the full handshake response, 0 means we need to
receive more data still.


> > +static void qio_channel_websock_encode(QIOChannelWebsock *ioc)
> > +{
> > +    size_t header_size = 0;
> > +    unsigned char opcode = QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME;
> > +    union {
> > +        char buf[QIO_CHANNEL_WEBSOCK_HEAD_MAX_LEN];
> > +        QIOChannelWebsockHeader ws;
> > +    } header;
> > +
> > +    DPRINTF("Encoding pending raw output %zu %p\n",
> > +            ioc->rawoutput.offset, ioc);
> > +    if (!ioc->rawoutput.offset) {
> > +        return;
> > +    }
> > +
> > +    header.ws.b0 = 0x80 | (opcode & 0x0f);
> 
> There are quite a few magic header sizes 125, (and I think I saw
> some other sizes below) - some comments on them, or constants?

This code was derived from ui/vnc-ws.c which is full of magic,
so I've mostly just inherited decisions made by that original
author. I'll fix up this new code to be less magic and use
constants and/or comments to clarify

> 
> > +    if (ioc->rawoutput.offset <= 125) {
> 
> Dave
> 
> > +        header.ws.b1 = (uint8_t)ioc->rawoutput.offset;
> > +        header_size = 2;
> > +    } else if (ioc->rawoutput.offset < 65536) {
> > +        header.ws.b1 = 0x7e;
> > +        header.ws.u.s16.l16 = cpu_to_be16((uint16_t)ioc->rawoutput.offset);
> > +        header_size = 4;
> > +    } else {
> > +        header.ws.b1 = 0x7f;
> > +        header.ws.u.s64.l64 = cpu_to_be64(ioc->rawoutput.offset);
> > +        header_size = 10;
> > +    }
> > +
> > +    qio_buffer_reserve(&ioc->encoutput, header_size + 
> > ioc->rawoutput.offset);
> > +    qio_buffer_append(&ioc->encoutput, header.buf, header_size);
> > +    qio_buffer_append(&ioc->encoutput, ioc->rawoutput.buffer,
> > +                      ioc->rawoutput.offset);
> > +    qio_buffer_reset(&ioc->rawoutput);
> > +    DPRINTF("Have %zu bytes encoded output %p\n",
> > +            ioc->encoutput.offset, ioc);
> > +}

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|