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