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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel