[Spice-devel] [PATCH spice ] Add support for clients connecting with the WebSocket protocol.

[Pavel Grunt]

Hi Jeremy,

On Mon, 2015-11-02 at 15:20 -0600, Jeremy White wrote:
> Hi Pavel,
> 
> Thanks for the review.
> 
> > > 
> > > +static void reds_handle_new_link(RedLinkInfo *link);
> > >  static void reds_handle_read_header_done(void *opaque)
> > >  {
> > >      RedLinkInfo *link = (RedLinkInfo *)opaque;
> > > @@ -2182,6 +2183,11 @@ static void reds_handle_read_header_done(void
> > > *opaque)
> > >      header->size = GUINT32_FROM_LE(header->size);
> > >  
> > >      if (header->magic != SPICE_MAGIC) {
> > > +        if (reds_stream_is_websocket(link->stream,
> > > +            (guchar *) header, sizeof(*header))) {
I did not mentioned it, but please fix the indentation ^
> > > +            reds_handle_new_link(link);
> > > +            return;
> > > +        }
> > This looks hacky, it would be nice to detect that we are dealing with
> > websockets
> > before reds_handle_read_header_done() is called.
> 
> The problem is that we cannot know we're dealing with websockets until
> we've done an initial read.  And a websocket connection is going to have
> to write at least 16 bytes, so it seemed to me that we may as well read
> the 16 bytes in a SpiceLinkHeader.

ok, I think it deserves a comment in the code 
> 
> The alternate I considered was to inject a 3 byte read before
> read_header_done and then have that do an async read of
> sizeof(SpiceLinkHeader) - 3 if we do not have the 'GET' that signals a
> websocket connection.  That didn't seem any better to me; I'm happy to
> defer to others if they feel that would be superior (or if there is an
> altogether different approach I've missed).

I think your current approach is better.

> 
> In reviewing this code, there is a flaw I've skipped over.  That is, the
> current implementation makes the assumption that the websocket header
> will arrive in a single packet; that there is no chance we'll get a few
> bytes, and then need another read in order to get the remaining bytes.
> I think that, in practice, this will always be the case.  Further,
> afaik, we don't really have a mechanism to do a variable length/timed
> read in the current code base.  My instinct is to say that limitation is
> acceptable; that the complexity of a theoretically correct
> implementation would do more harm than accepting this assumption.  But I
> bring it up to give others the chance to correct me <grin>.
> 
You can mention the limitation in the commit message.
> [snip]
> > > +        websocket_create_reply(rbuf, len, outbuf);
> > > +        rc = stream->priv->write(stream, outbuf, strlen(outbuf));
> > > +        if (rc == strlen(outbuf)) {
> > > +            stream->priv->ws = spice_malloc0(sizeof(*stream->priv->ws));
> > 
> > It is not freed
> 
> Yes, right, will fix it.
> 
> > > +int websocket_is_start(gchar *buf, int len)
> > 
> > It sounds like boolean
> 
> Fair enough, I'll fix it.
> 
> > 
> > > +{
> > > +    if (len < 4)
> > > +        return 0;
> > > +
> > > +    if (find_str(buf, "GET", len) == (buf + 3) &&
> > > +            find_str(buf, "Sec-WebSocket-Protocol: binary", len) &&
> > > +            find_str(buf, "Sec-WebSocket-Key:", len) &&
> > > +            buf[len - 2] == '\r' && buf[len - 1] == '\n')
> > > +        return 1;
> > > +
> > > +    return 0;
> > > +}
> > > +
> > > +int websocket_create_reply(gchar *buf, int len, gchar *outbuf)
> > 
> > It always returns 0
> 
> Yep, should be a void, I'll fix it.
> 
> > 
> > > +{
> > > +    gchar *key;
> > > +
> > > +    key = generate_reply_key(buf, len);
> > 
> > Can NULL be valid value? Should we have a warning for it, or change the
> > return
> > value?
> 
> NULL is not a valid value; this code path should only be exercised if
> we've found the Sec-WebSocket-Key already.
> 
> > 
> > > +    sprintf(outbuf, "HTTP/1.1 101 Switching Protocols\r\n"
> > > +                    "Upgrade: websocket\r\n"
> > > +                    "Connection: Upgrade\r\n"
> > > +                    "Sec-WebSocket-Accept: %s\r\n"
> > > +                    "Sec-WebSocket-Protocol: binary\r\n\r\n", key);
> > > +    g_free(key);
> > > +    return 0;
> > > +}
> > > diff --git a/server/websocket.h b/server/websocket.h
> > > new file mode 100644
> > > index 0000000..b35cb5e
> > > --- /dev/null
> > > +++ b/server/websocket.h
> > > @@ -0,0 +1,62 @@
> > > +/*
> > > + *  Copyright (C) 2015 Jeremy White
> > > + *
> > > + *  This library is free software; you can redistribute it and/or
> > > + *  modify it under the terms of the GNU Lesser General Public
> > > + *  License as published by the Free Software Foundation; either
> > > + *  version 2.1 of the License, or (at your option) any later version.
> > > + *
> > > + *  This library is distributed in the hope that it will be useful,
> > > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > > + *  Lesser General Public License for more details.
> > > + *
> > > + *  You should have received a copy of the GNU Lesser General Public
> > > + *  License along with this library; if not, see <http://www.gnu.org/lice
> > > nses
> > > />.
> > > + */
> > > +
> > > +#define WEBSOCKET_MAX_HEADER_SIZE (1 + 9 + 4)
> > > +
> > > +#define FIN_FLAG        0x80
> > > +#define TYPE_MASK       0x0F
> > > +
> > > +#define BINARY_FRAME    0x2
> > > +#define CLOSE_FRAME     0x8
> > > +#define PING_FRAME      0x9
> > > +#define PONG_FRAME      0xA
> > > +
> > > +#define LENGTH_MASK     0x7F
> > > +#define LENGTH_16BIT    0x7E
> > > +#define LENGTH_64BIT    0x7F
> > > +
> > > +#define MASK_FLAG       0x80
> > > +
> > > +#define WEBSOCKET_GUID "258EAFA5-E914-47DA-95CA-C5AB0DC85B11"
> > > +
> > > +typedef struct
> > > +{
> > > +    int type;
> > > +    int masked;
> > > +    guint8 header[WEBSOCKET_MAX_HEADER_SIZE];
> > > +    int header_pos;
> > > +    int frame_ready:1;
> > > +    guint8 mask[4];
> > > +    guint64 relayed;
> > > +    guint64 expected_len;
> > > +} websocket_frame_t;
> > 
> > I would prefer to have these ^ definitions private.
> 
> I could probably use some guidance; there may be conventions I've
> missed.  It would be easy to move most of the websocket specific
> constants into websocket.c, I'm happy to do that.
thanks
> 
> It would be a bit more work to make the websocket_frame_t type be
> opaque; it would require an allocation and a free I'd find untidy. 

ok, it is not the expected standard. You can keep it as it is.

>  I'm
> willing to do the work if that's the expected standard; but I don't see
> the harm of having a non private WEBSOCKET_MAX_HEADER_SIZE and
> websocket_frame_t structure.

> > 
> > > +
> > > +typedef size_t (*websocket_writev_cb_t)(void *opaque, struct iovec *iov,
> > > int
> > > iovcnt);
> > > +typedef size_t (*websocket_write_cb_t)(void *opaque, const void *buf,
> > > size_t
> > > nbyte);
> > > +typedef size_t (*websocket_read_cb_t)(void *opaque, const void *buf,
> > > size_t
> > > nbyte);
> > 
> > RedsStreamPrivate has these ^ as ssize_t
> 
> Yes, right, I'll fix it.
> 
> > 
> > > +
> > > +int websocket_is_start(gchar *buf, int len);
> > > +int websocket_create_reply(gchar *buf, int len, gchar *outbuf);
> > > +int websocket_read(void *opaque, guchar *buf, int len, websocket_frame_t
> > > *frame,
> > > +         websocket_read_cb_t read_cb,
> > > +         websocket_write_cb_t write_cb);
> > > +int websocket_write(void *opaque, const guchar *buf, int len, guint64
> > > *remainder,
> > > +         websocket_write_cb_t write_cb);
> > > +int websocket_writev(void *opaque, struct iovec *iov, int iovcnt, guint64
> > > *remainder,
> > > +         websocket_writev_cb_t writev_cb);
> > > +void websocket_ack_close(void *opaque, websocket_write_cb_t write_cb);
> > > +
> > extra line
> 
> Right, will fix.
> 
> Thanks again for the review.
> 
> Cheers,
> 
> Jeremy

Thanks,
Pavel