[Spice-devel] [PATCH spice-server 10/30] Move RedsWebSocket to header
Frediano Ziglio
fziglio at redhat.com
Fri Nov 25 15:34:30 UTC 2016
>
> 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").
>
Yes, I had a look and the problem with these libraries is that
they tend to do everything. One problem is that they handle TLS
directly. So we would need an adapter code to integrate libwebsockets
in reds-stream.c and either an adapter code to adapt libwebsockets for
our TLS code or an adapter code to adapt libwebsockets TLS code to
our setting. So could be not so easy.
> > - 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)
>
You are right.
> > - 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)
Qemu already has a websocket option for VNC with an option (optional
port). The wsport and wssport are more for the 4 port specification.
But you are right, the current websocket=port don't allow to
distinguish is TLS or not.
> > 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)
>
I found this http://osnet.cs.binghamton.edu/publications/TR-20070820.pdf
(from http://bhavin.directi.com/unix-domain-sockets-vs-tcp-sockets/).
So yes, unix sockets are faster. But obviously they introduce latency and
different copies/kernel contexts compared to having websocket directly
into spice-server.
> Personally I like the idea of providing the websocket
> Pavel
acked :-)
I think we should have a test for websocket and possibly passing Autobahn
tests.
> >
> > > 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