>
> 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.
>
Yes, most of these patches could be squashed in some way.
> So, you can consider the series Acked by me.
>
> Cheers,
>
> Jeremy
>
We were discussing different things about websockets and spice-server:
- use some external library like libwebsockets?
- is Qemu going to provide a TLS/websocket library for us? Not in the
near future;
- Qemu options. Currently for VNC there is a websocket option that enable
websockets on a different port. You can also specify the port.
But I like the idea to use the same port. I would propose a websocket=same
option (currently same required) so can be compatible in the future
if somebody wants to have a different port. Currently VNC use a single
port for TLS or no TLS, simply when TLS is enabled only encrypted
connections are accepted. I honestly prefer having 2 ports one encrypted
and the other not encrypted;
- avoid changing reds.c? Doing all in reds-stream.c seems a right approach
for consistency. Looks like not so easy. Was proposed by Pavel but I
agree could be an improvement.
Frediano
> 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);
> >
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel