On Thu, 2016-11-24 at 11:12 -0500, Frediano Ziglio wrote:
> >
> > 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?
Can be an improvement (from the maintenance perspective), may be
overkill (and the work "was done").
> - is Qemu going to provide a TLS/websocket library for us? Not in
> the
> near future;
It is not just about qemu - there are tools like XSpice, x11spice that
are working inside a machine (bare metal). (And we have the replay
utility which can be used for testing)
> - 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.
me too, it is very convenient from the client perspective
> I would propose a websocket=same
or wsport and wssport (secure)
> 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;
yes
> - 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.
- we also discussed that there are tools like websockify "translating"
tcp to websockets - my feeling is that it is not as efficient as a
direct websocket connection - I don't have numbers. I wonder if such
tool could work with the spice unix connection instead of tcp,
performance could be better (again i have no numbers but it would be 1
tcp connection instead of 2)
Personally I like the idea of providing the websocket
Pavel
>
> 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