[Spice-devel] [PATCH spice-server 10/30] Move RedsWebSocket to header
Jeremy White
jwhite at codeweavers.com
Tue Nov 22 22:27:40 UTC 2016
Hi Frediano,
Thanks for the review, and all the hard work to tighten the code.
I've reviewed all 30 patches, and have done a quick dry run test, and it
still seems to work for my use case - thanks!
I have to confess I didn't have the time for as detailed a review as
perhaps it calls for; the PING implementation looks correct, but I have
not tested it.
The only thing that did catch my eye was this patch. It really seems
like it should be combined with the 'Propagate some variable' patch.
This patch feels like the first half of the overall job. With that
said, the net result looks correct to me, so I won't object to a push as
it stands.
So, you can consider the series Acked by me.
Cheers,
Jeremy
On 11/21/2016 06:51 AM, Frediano Ziglio wrote:
> Intention is to make private in websockets.c and reduce
> changes in reds-stream.c
> ---
> server/reds-stream.c | 40 ++++------------------------------------
> server/websocket.c | 42 ++++++++++++++++++++++++++++++++++--------
> server/websocket.h | 24 +++++++++++++++++-------
> 3 files changed, 55 insertions(+), 51 deletions(-)
>
> diff --git a/server/reds-stream.c b/server/reds-stream.c
> index b59586b..bf0cbbe 100644
> --- a/server/reds-stream.c
> +++ b/server/reds-stream.c
> @@ -78,17 +78,6 @@ typedef struct RedsSASL {
> } RedsSASL;
> #endif
>
> -typedef struct {
> - int closed;
> -
> - websocket_frame_t read_frame;
> - guint64 write_remainder;
> -
> - ssize_t (*raw_read)(RedsStream *s, void *buf, size_t nbyte);
> - ssize_t (*raw_write)(RedsStream *s, const void *buf, size_t nbyte);
> - ssize_t (*raw_writev)(RedsStream *s, const struct iovec *iov, int iovcnt);
> -} RedsWebSocket;
> -
> struct RedsStreamPrivate {
> SSL *ssl;
>
> @@ -1151,39 +1140,17 @@ error:
>
> static ssize_t stream_websocket_read(RedsStream *s, void *buf, size_t size)
> {
> - int rc;
> -
> - if (s->priv->ws->closed)
> - return 0;
> -
> - rc = websocket_read((void *)s, buf, size, &s->priv->ws->read_frame,
> - (websocket_write_cb_t) s->priv->ws->raw_read,
> - (websocket_write_cb_t) s->priv->ws->raw_write);
> -
> - if (rc == 0)
> - s->priv->ws->closed = 1;
> -
> - return rc;
> + return websocket_read(s->priv->ws, buf, size);
> }
>
> static ssize_t stream_websocket_write(RedsStream *s, const void *buf, size_t size)
> {
> - if (s->priv->ws->closed) {
> - errno = EPIPE;
> - return -1;
> - }
> - return websocket_write((void *)s, buf, size, &s->priv->ws->write_remainder,
> - (websocket_write_cb_t) s->priv->ws->raw_write);
> + return websocket_write(s->priv->ws, buf, size);
> }
>
> static ssize_t stream_websocket_writev(RedsStream *s, const struct iovec *iov, int iovcnt)
> {
> - if (s->priv->ws->closed) {
> - errno = EPIPE;
> - return -1;
> - }
> - return websocket_writev((void *)s, (struct iovec *) iov, iovcnt, &s->priv->ws->write_remainder,
> - (websocket_writev_cb_t) s->priv->ws->raw_writev);
> + return websocket_writev(s->priv->ws, (struct iovec *) iov, iovcnt);
> }
>
> /*
> @@ -1232,6 +1199,7 @@ bool reds_stream_is_websocket(RedsStream *stream, unsigned char *buf, int len)
> if (rc == strlen(outbuf)) {
> stream->priv->ws = spice_malloc0(sizeof(*stream->priv->ws));
>
> + stream->priv->ws->raw_stream = stream;
> stream->priv->ws->raw_read = stream->priv->read;
> stream->priv->ws->raw_write = stream->priv->write;
>
> diff --git a/server/websocket.c b/server/websocket.c
> index 1379487..533fc06 100644
> --- a/server/websocket.c
> +++ b/server/websocket.c
> @@ -216,20 +216,29 @@ static int relay_data(guint8* buf, size_t size, websocket_frame_t *frame)
> return n;
> }
>
> -int websocket_read(void *opaque, guchar *buf, int size, websocket_frame_t *frame,
> - websocket_read_cb_t read_cb,
> - websocket_write_cb_t write_cb)
> +int websocket_read(RedsWebSocket *ws, guchar *buf, int size)
> {
> int n = 0;
> int rc;
> + websocket_frame_t *frame = &ws->read_frame;
> + void *opaque = ws->raw_stream;
> + websocket_read_cb_t read_cb = (websocket_read_cb_t) ws->raw_read;
> + websocket_write_cb_t write_cb = (websocket_write_cb_t) ws->raw_write;
> +
> + if (ws->closed) {
> + return 0;
> + }
>
> while (size > 0) {
> if (! frame->frame_ready) {
> - rc = read_cb(opaque, frame->header + frame->header_pos, frame_bytes_needed(frame));
> + rc = read_cb(ws->raw_stream, frame->header + frame->header_pos, frame_bytes_needed(frame));
> if (rc <= 0) {
> if (n > 0 && rc == -1 && (errno == EINTR || errno == EAGAIN))
> return n;
>
> + if (rc == 0) {
> + ws->closed = TRUE;
> + }
> return rc;
> }
> frame->header_pos += rc;
> @@ -238,6 +247,7 @@ int websocket_read(void *opaque, guchar *buf, int size, websocket_frame_t *frame
> } else if (frame->type == CLOSE_FRAME) {
> websocket_ack_close(opaque, write_cb);
> websocket_clear_frame(frame);
> + ws->closed = TRUE;
> return 0;
> } else if (frame->type == BINARY_FRAME) {
> rc = read_cb(opaque, buf, MIN(size, frame->expected_len - frame->relayed));
> @@ -245,6 +255,9 @@ int websocket_read(void *opaque, guchar *buf, int size, websocket_frame_t *frame
> if (n > 0 && rc == -1 && (errno == EINTR || errno == EAGAIN))
> return n;
>
> + if (rc == 0) {
> + ws->closed = TRUE;
> + }
> return rc;
> }
>
> @@ -323,8 +336,7 @@ static void constrain_iov(struct iovec *iov, int iovcnt,
>
>
> /* Write a WebSocket frame with the enclosed data out. */
> -int websocket_writev(void *opaque, struct iovec *iov, int iovcnt, guint64 *remainder,
> - websocket_writev_cb_t writev_cb)
> +int websocket_writev(RedsWebSocket *ws, struct iovec *iov, int iovcnt)
> {
> guint8 header[WEBSOCKET_MAX_HEADER_SIZE];
> guint64 len;
> @@ -333,7 +345,14 @@ int websocket_writev(void *opaque, struct iovec *iov, int iovcnt, guint64 *remai
> int iov_out_cnt;
> int i;
> int header_len;
> + void *opaque = ws->raw_stream;
> + websocket_writev_cb_t writev_cb = (websocket_writev_cb_t) ws->raw_writev;
> + guint64 *remainder = &ws->write_remainder;
>
> + if (ws->closed) {
> + errno = EPIPE;
> + return -1;
> + }
> if (*remainder > 0) {
> constrain_iov(iov, iovcnt, &iov_out, &iov_out_cnt, *remainder);
> rc = writev_cb(opaque, iov_out, iov_out_cnt);
> @@ -376,12 +395,19 @@ int websocket_writev(void *opaque, struct iovec *iov, int iovcnt, guint64 *remai
> return rc;
> }
>
> -int websocket_write(void *opaque, const guchar *buf, int len, guint64 *remainder,
> - websocket_write_cb_t write_cb)
> +int websocket_write(RedsWebSocket *ws, const guchar *buf, int len)
> {
> guint8 header[WEBSOCKET_MAX_HEADER_SIZE];
> int rc;
> int header_len;
> + void *opaque = ws->raw_stream;
> + websocket_write_cb_t write_cb = (websocket_write_cb_t) ws->raw_write;
> + guint64 *remainder = &ws->write_remainder;
> +
> + if (ws->closed) {
> + errno = EPIPE;
> + return -1;
> + }
>
> if (*remainder == 0) {
> header_len = fill_header(header, len);
> diff --git a/server/websocket.h b/server/websocket.h
> index 2306492..2eb3431 100644
> --- a/server/websocket.h
> +++ b/server/websocket.h
> @@ -17,6 +17,8 @@
>
> #define WEBSOCKET_MAX_HEADER_SIZE (1 + 9 + 4)
>
> +struct RedsStream;
> +
> typedef struct {
> int type;
> int masked;
> @@ -32,14 +34,22 @@ typedef ssize_t (*websocket_read_cb_t)(void *opaque, const void *buf, size_t nby
> typedef ssize_t (*websocket_write_cb_t)(void *opaque, const void *buf, size_t nbyte);
> typedef ssize_t (*websocket_writev_cb_t)(void *opaque, struct iovec *iov, int iovcnt);
>
> +typedef struct {
> + int closed;
> +
> + websocket_frame_t read_frame;
> + guint64 write_remainder;
> +
> + struct RedsStream *raw_stream;
> + ssize_t (*raw_read)(struct RedsStream *s, void *buf, size_t nbyte);
> + ssize_t (*raw_write)(struct RedsStream *s, const void *buf, size_t nbyte);
> + ssize_t (*raw_writev)(struct RedsStream *s, const struct iovec *iov, int iovcnt);
> +} RedsWebSocket;
> +
> bool websocket_is_start(gchar *buf);
> void websocket_create_reply(gchar *buf, 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);
> +int websocket_read(RedsWebSocket *ws, guchar *buf, int len);
> +int websocket_write(RedsWebSocket *ws, const guchar *buf, int len);
> +int websocket_writev(RedsWebSocket *ws, struct iovec *iov, int iovcnt);
> void websocket_ack_close(void *opaque, websocket_write_cb_t write_cb);
>
>
More information about the Spice-devel
mailing list