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

[Jeremy White]

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))) {
>> +            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.

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).

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

[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/licenses
>> />.
>> + */
>> +
>> +#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.

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