[Spice-devel] [PATCH spice-server 10/30] Move RedsWebSocket to header

Frediano Ziglio fziglio at redhat.com
Thu Nov 24 16:12:13 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.
> 

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);
> >  


More information about the Spice-devel mailing list