[Spice-devel] [PATCH 4/5] server: add websockets support via libwebsockets
Alon Levy
alevy at redhat.com
Tue Oct 23 05:31:33 PDT 2012
> On Fri, Oct 19, 2012 at 01:50:11PM +0200, Alon Levy wrote:
> > New API: spice_server_set_ws_ports
> >
> > This adds an optional dependency on libwebsockets. You need to get
> > my
> > patched 0.0.3 version here:
> > git://people.freedesktop.org/~alon/libwebsockets
> >
> > There is no qemu patches yet, to test change in reds.c the default
> > value
> > of spice_ws_port to 5959 (for the default of spice-html5).
> >
> > For testing there is an online client at
> > http://spice-space.org/spice-html5/spice.html
> >
> > Known issues:
> > 1. The tester (server/tests/test_display_no_ssl) gets into
> > dropping all
> > data after a few seconds, I think it's an issue with the
> > implemented
> > watches, but haven't figured it out.
> >
> > 2. libwebsocket's read interface is inverted to what our code
> > expects,
> > i.e. there is no libwebsocket_read, so there is an additional copy
> > involved (see RedsWebSocket). This can be fixed.
> >
> > 3. Listening on a separate port. Since the headers are different,
> > we
> > could listen on the same port (first three bytes RED/GET). I don't
> > know
> > if we want to?
> >
> > Todos:
> > 1. SSL not implemented yet. Needs some thought as to how.
> >
> > 2. Serve spice-html5 when accessed as a http server. Nice to have.
> > ---
> > configure.ac | 15 ++
> > server/Makefile.am | 4 +
> > server/reds-private.h | 47 +++++-
> > server/reds.c | 79 ++++++----
> > server/reds.h | 17 +++
> > server/reds_websockets.c | 311
> > +++++++++++++++++++++++++++++++++++++++
> > server/reds_websockets.h | 9 ++
> > server/spice-server.syms | 5 +
> > server/spice.h | 7 +
> > server/tests/test_display_base.c | 4 +-
> > 10 files changed, 466 insertions(+), 32 deletions(-)
> > create mode 100644 server/reds_websockets.c
> > create mode 100644 server/reds_websockets.h
> >
> > diff --git a/configure.ac b/configure.ac
> > index dff930d..5561d2c 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -160,6 +160,13 @@ AC_ARG_ENABLE(automated_tests,
> > AS_IF([test x"$enable_automated_tests" != "xno"],
> > [enable_automated_tests="yes"])
> > AM_CONDITIONAL(SUPPORT_AUTOMATED_TESTS, test
> > "x$enable_automated_tests" != "xno")
> >
> > +AC_ARG_ENABLE(libwebsockets,
> > +[ --enable-libwebsockets Enable websockets server support (no
> > need for proxy)],,
> > +[enable_libwebsockets="yes"])
>
> Would be better if this defaulted to "check" and used libwebsockets
> if
> available, and don't use it if it's not
I just wanted to avoid regressions for now - let it only be available if explicitly asked for.
>
> > +AS_IF([test x"enable_libwebsockets" != "xyes"],
> > [enable_websockets="no"])
> > +if test "x$enable_libwebsockets" = "xyes"; then
> > + AC_DEFINE([USE_LIBWEBSOCKETS], [1], [Define if supporting
> > websocket connections])
> > +fi
> >
> > dnl
> > =========================================================================
> > dnl Check deps
> > @@ -237,6 +244,12 @@ if test "x$enable_smartcard" = "xyes"; then
> > SPICE_REQUIRES+=" libcacard >= 0.1.2"
> > fi
> >
> > +if test "x$enable_libwebsockets" = "xyes"; then
> > + PKG_CHECK_MODULES(LIBWEBSOCKETS, libwebsockets >= 0.0.3)
> > + AC_SUBST(LIBWEBSOCKETS_LIBS)
> > + AC_SUBST(LIBWEBSOCKETS_CFLAGS)
> > + SPICE_REQUIRES+=" libwebsockets >= 0.0.3"
> > +fi
> >
> > PKG_CHECK_MODULES(PIXMAN, pixman-1 >= 0.17.7)
> > AC_SUBST(PIXMAN_CFLAGS)
> > @@ -536,6 +549,8 @@ echo "
> > SASL support: ${enable_sasl}
> >
> > Automated tests: ${enable_automated_tests}
> > +
> > + libwebsockets: ${enable_libwebsockets}
> > "
> >
> > if test $os_win32 == "yes" ; then
> > diff --git a/server/Makefile.am b/server/Makefile.am
> > index b62d98c..a7d56d4 100644
> > --- a/server/Makefile.am
> > +++ b/server/Makefile.am
> > @@ -10,6 +10,7 @@ AM_CPPFLAGS = \
> > $(SLIRP_CFLAGS) \
> > $(SMARTCARD_CFLAGS) \
> > $(SSL_CFLAGS) \
> > + $(LIBWEBSOCKETS_CFLAGS) \
> > $(VISIBILITY_HIDDEN_CFLAGS) \
> > $(WARN_CFLAGS) \
> > $(NULL)
> > @@ -38,6 +39,7 @@ libspice_server_la_LIBADD = \
> > $(SLIRP_LIBS) \
> > $(SSL_LIBS) \
> > $(Z_LIBS) \
> > + $(LIBWEBSOCKETS_LIBS) \
> > $(NULL)
> >
> > libspice_server_la_SOURCES = \
> > @@ -91,6 +93,8 @@ libspice_server_la_SOURCES = \
> > spicevmc.c \
> > zlib_encoder.c \
> > zlib_encoder.h \
> > + reds_websockets.c \
> > + reds_websockets.h \
>
> Compilation of this should depend on websockets being available or
> not
The files have an ifdef that makes them null, but I can also do it here, probably nicer, so ok.
>
>
> > $(NULL)
> >
> > if SUPPORT_TUNNEL
> > diff --git a/server/reds-private.h b/server/reds-private.h
> > index 3db6565..a5903b3 100644
> > --- a/server/reds-private.h
> > +++ b/server/reds-private.h
> > @@ -4,6 +4,16 @@
> > #include <time.h>
> >
> > #include <spice/protocol.h>
> > +#include <spice/stats.h>
> > +
> > +#if USE_LIBWEBSOCKETS
> > +#include <libwebsockets.h>
> > +#endif
> > +
> > +#include "reds.h"
> > +#include "char_device.h"
> > +#include "agent-msg-filter.h"
> > +#include "main_channel.h"
> >
> > #define MIGRATE_TIMEOUT (1000 * 10) /* 10sec */
> > #define MM_TIMER_GRANULARITY_MS (1000 / 30)
> > @@ -34,10 +44,6 @@ typedef struct VDIReadBuf {
> > uint8_t data[SPICE_AGENT_MAX_DATA_SIZE];
> > } VDIReadBuf;
> >
> > -static VDIReadBuf *vdi_port_read_buf_get(void);
> > -static VDIReadBuf *vdi_port_read_buf_ref(VDIReadBuf *buf);
> > -static void vdi_port_read_buf_unref(VDIReadBuf *buf);
> > -
> > enum {
> > VDI_PORT_READ_STATE_READ_HEADER,
> > VDI_PORT_READ_STATE_GET_BUFF,
> > @@ -125,9 +131,19 @@ typedef struct RedsClientMonitorsConfig {
> > int buffer_pos;
> > } RedsClientMonitorsConfig;
> >
> > +#ifdef USE_LIBWEBSOCKETS
> > +#define REDS_MAX_WEBSOCKETS 32
> > +#endif
> > +
> > typedef struct RedsState {
> > int listen_socket;
> > int secure_listen_socket;
> > +#ifdef USE_LIBWEBSOCKETS
> > + struct libwebsocket_context *ws_context;
> > + RedsWebSocket ws[REDS_MAX_WEBSOCKETS];
> > + int ws_in_service_fd;
> > + int ws_count;
> > +#endif
> > SpiceWatch *listen_watch;
> > SpiceWatch *secure_listen_watch;
> > VDIPortState agent_state;
> > @@ -179,4 +195,27 @@ typedef struct RedsState {
> > RedsClientMonitorsConfig client_monitors_config;
> > } RedsState;
> >
> > +typedef struct AsyncRead {
> > + RedsStream *stream;
> > + void *opaque;
> > + uint8_t *now;
> > + uint8_t *end;
> > + void (*done)(void *opaque);
> > + void (*error)(void *opaque, int err);
> > +} AsyncRead;
> > +
> > +typedef struct RedLinkInfo {
> > + RedsStream *stream;
> > + AsyncRead asyc_read;
> > + SpiceLinkHeader link_header;
> > + SpiceLinkMess *link_mess;
> > + int mess_pos;
> > + TicketInfo tiTicketing;
> > + SpiceLinkAuthMechanism auth_mechanism;
> > + int skip_auth;
> > +} RedLinkInfo;
> > +
> > +RedLinkInfo *spice_server_add_client_create_link(SpiceServer *s,
> > int socket, int skip_auth);
> > +void reds_handle_new_link(RedLinkInfo *link);
> > +
> > #endif
> > diff --git a/server/reds.c b/server/reds.c
> > index 98c8706..bd16764 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -74,9 +74,16 @@
> > #ifdef USE_SMARTCARD
> > #include "smartcard.h"
> > #endif
> > +#if USE_LIBWEBSOCKETS
> > +#include "reds_websockets.h"
> > +#endif
> >
> > #include "reds-private.h"
> >
> > +static VDIReadBuf *vdi_port_read_buf_get(void);
> > +static VDIReadBuf *vdi_port_read_buf_ref(VDIReadBuf *buf);
> > +static void vdi_port_read_buf_unref(VDIReadBuf *buf);
> > +
> > SpiceCoreInterface *core = NULL;
> > static SpiceCharDeviceInstance *vdagent = NULL;
> > static SpiceMigrateInstance *migration_interface = NULL;
> > @@ -99,6 +106,10 @@ static TicketAuthentication taTicket;
> >
> > static int spice_port = -1;
> > static int spice_secure_port = -1;
> > +#if USE_LIBWEBSOCKETS
> > +static int spice_ws_port = -1;
> > +static int spice_wss_port = -1;
> > +#endif
> > static int spice_listen_socket_fd = -1;
> > static char spice_addr[256];
> > static int spice_family = PF_UNSPEC;
> > @@ -127,26 +138,6 @@ static bool exit_on_disconnect = FALSE;
> >
> > static RedsState *reds = NULL;
> >
> > -typedef struct AsyncRead {
> > - RedsStream *stream;
> > - void *opaque;
> > - uint8_t *now;
> > - uint8_t *end;
> > - void (*done)(void *opaque);
> > - void (*error)(void *opaque, int err);
> > -} AsyncRead;
> > -
> > -typedef struct RedLinkInfo {
> > - RedsStream *stream;
> > - AsyncRead asyc_read;
> > - SpiceLinkHeader link_header;
> > - SpiceLinkMess *link_mess;
> > - int mess_pos;
> > - TicketInfo tiTicketing;
> > - SpiceLinkAuthMechanism auth_mechanism;
> > - int skip_auth;
> > -} RedLinkInfo;
> > -
> > typedef struct RedSSLParameters {
> > char keyfile_password[256];
> > char certs_file[256];
> > @@ -2718,7 +2709,7 @@ static void reds_handle_read_header_done(void
> > *opaque)
> > async_read_handler(0, 0, &link->asyc_read);
> > }
> >
> > -static void reds_handle_new_link(RedLinkInfo *link)
> > +void reds_handle_new_link(RedLinkInfo *link)
> > {
> > AsyncRead *obj = &link->asyc_read;
> > obj->opaque = link;
> > @@ -2882,7 +2873,6 @@ static void reds_accept_ssl_connection(int
> > fd, int event, void *data)
> > }
> > }
> >
> > -
> > static void reds_accept(int fd, int event, void *data)
> > {
> > int socket;
> > @@ -2896,25 +2886,33 @@ static void reds_accept(int fd, int event,
> > void *data)
> > close(socket);
> > }
> >
> > -
> > -SPICE_GNUC_VISIBLE int spice_server_add_client(SpiceServer *s, int
> > socket, int skip_auth)
> > +RedLinkInfo *spice_server_add_client_create_link(SpiceServer *s,
> > int socket, int skip_auth)
> > {
> > RedLinkInfo *link;
> > - RedsStream *stream;
> >
> > spice_assert(reds == s);
> > if (!(link = reds_init_client_connection(socket))) {
> > spice_warning("accept failed");
> > - return -1;
> > + return NULL;
> > }
> >
> > link->skip_auth = skip_auth;
> > + return link;
> > +}
> > +
> > +SPICE_GNUC_VISIBLE int spice_server_add_client(SpiceServer *s, int
> > socket, int skip_auth)
> > +{
> > + RedLinkInfo *link;
> > + RedsStream *stream;
> >
> > + link = spice_server_add_client_create_link(s, socket,
> > skip_auth);
> > + if (!link) {
> > + return -1;
> > + }
> > stream = link->stream;
> > stream->read = stream_read_cb;
> > stream->write = stream_write_cb;
> > stream->writev = stream_writev_cb;
> > -
> > reds_handle_new_link(link);
> > return 0;
> > }
> > @@ -3033,6 +3031,12 @@ static int reds_init_net(void)
> > return -1;
> > }
> > }
> > +
> > +#if USE_LIBWEBSOCKETS
> > + if (spice_ws_port != -1 || spice_wss_port != -1) {
> > + reds_init_websocket(reds, spice_addr, spice_ws_port,
> > spice_wss_port);
> > + }
> > +#endif
> > return 0;
> > }
> >
> > @@ -3949,6 +3953,27 @@ SPICE_GNUC_VISIBLE int
> > spice_server_set_port(SpiceServer *s, int port)
> > return 0;
> > }
> >
> > +
> > +SPICE_GNUC_VISIBLE int spice_server_set_ws_ports(SpiceServer *s,
> > int ws_port, int wss_port)
> > +{
> > +#if USE_LIBWEBSOCKETS
> > + spice_assert(reds == s);
> > + if ((ws_port < 1 || ws_port > 0xffff) && (wss_port < 1 ||
> > wss_port > 0xffff)) {
> > + return -1;
> > + }
> > + if (ws_port > 0 && ws_port < 0xffff) {
> > + spice_ws_port = ws_port;
> > + }
> > + if (wss_port > 0 && wss_port < 0xffff) {
> > + spice_wss_port = wss_port;
> > + }
> > + return 0;
> > +#else
> > + fprintf(stderr, "libwebsockets is unsupported in this spice
> > build\n");
> > + return -1;
> > +#endif
> > +}
> > +
> > SPICE_GNUC_VISIBLE void spice_server_set_addr(SpiceServer *s,
> > const char *addr, int flags)
> > {
> > spice_assert(reds == s);
> > diff --git a/server/reds.h b/server/reds.h
> > index f8e8d56..0d4f933 100644
> > --- a/server/reds.h
> > +++ b/server/reds.h
> > @@ -65,6 +65,20 @@ typedef struct RedsSASL {
> > } RedsSASL;
> > #endif
> >
> > +#ifdef USE_LIBWEBSOCKETS
> > +typedef struct RedsWebSocket {
> > + struct libwebsocket_context *context;
> > + struct libwebsocket *wsi;
> > + SpiceWatch *watch;
> > + int fd;
> > + unsigned events;
> > + /* buffer of available data to read, always starts at offset 0
> > to data_avail - 1. */
> > + unsigned char *data;
> > + unsigned data_len;
> > + unsigned data_avail;
> > +} RedsWebSocket;
> > +#endif
> > +
> > struct RedsStream {
> > int socket;
> > SpiceWatch *watch;
> > @@ -73,6 +87,9 @@ struct RedsStream {
> > receive may return data afterward. check the flag before
> > calling receive*/
> > int shutdown;
> > SSL *ssl;
> > +#ifdef USE_LIBWEBSOCKETS
> > + RedsWebSocket *ws;
> > +#endif
> >
> > #if HAVE_SASL
> > RedsSASL sasl;
> > diff --git a/server/reds_websockets.c b/server/reds_websockets.c
> > new file mode 100644
> > index 0000000..24df2e5
> > --- /dev/null
> > +++ b/server/reds_websockets.c
> > @@ -0,0 +1,311 @@
> > +#include "config.h"
> > +
> > +#ifdef USE_LIBWEBSOCKETS
> > +#include <unistd.h>
> > +#include <errno.h>
> > +#include <libwebsockets.h>
> > +
> > +#include "spice.h"
> > +#include "reds.h"
> > +#include "reds-private.h"
> > +#include "reds_websockets.h"
> > +
> > +static ssize_t stream_write_ws_cb(RedsStream *s, const void *buf,
> > size_t size)
> > +{
> > + /* TODO: better way to handle the requirement of libwebsocket,
> > perhaps
> > + * we should make a writev version for libwebsocket. Assuming
> > writev doesn't
> > + * cause a linearlizing copy itself. */
>
> Yep, writev would be better
>
> > + ssize_t ret;
> > + unsigned char *padded_buf = spice_malloc(size +
> > LWS_SEND_BUFFER_PRE_PADDING +
> > +
> > LWS_SEND_BUFFER_POST_PADDING);
> > + spice_assert(s && s->ws);
> > + memcpy(padded_buf + LWS_SEND_BUFFER_PRE_PADDING, buf, size);
> > + ret = libwebsocket_write(s->ws->wsi,
> > &padded_buf[LWS_SEND_BUFFER_PRE_PADDING], size,
> > + LWS_WRITE_BINARY);
> > + free(padded_buf);
> > + return ret == 0 ? size : -1; /* XXX exact bytes required? if
> > not this is
> > + good enough, else need to
> > change
> > + libwebsocket */
> > +}
> > +
> > +static void reds_websocket_append_data(RedsWebSocket *ws, unsigned
> > char *buf,
> > + size_t size)
> > +{
> > + if (!ws->data) {
> > + ws->data = spice_malloc(size);
> > + ws->data_len = size;
> > + ws->data_avail = 0;
> > + }
> > + if (ws->data_len < size + ws->data_avail) {
> > + ws->data_len = size + ws->data_avail;
> > + ws->data = realloc(ws->data, ws->data_len);
>
> spice_realloc, or you need to handle realloc failures.
>
Thanks, will fix.
> > + }
> > + memcpy(ws->data + ws->data_avail, buf, size);
> > + ws->data_avail += size;
> > +}
> > +
> > +static ssize_t reds_websocket_read_data(RedsWebSocket *ws,
> > unsigned char *buf,
> > + size_t size)
> > +{
> > + ssize_t ret;
> > +
> > + ret = ws->data_avail > size ? size : ws->data_avail;
> > + if (ret > 0) {
> > + memcpy(buf, ws->data, ret);
> > + }
> > + if (ret > 0 && ret < ws->data_avail) {
> > + memmove(ws->data, ws->data + ret, ws->data_avail - ret);
> > + }
> > + ws->data_avail -= ret;
> > + if (ws->data_avail == 0 && ret == size) {
> > + ws->data = NULL;
> > + ws->data_len = ws->data_avail = 0;
>
> Isn't this leaking ws->data?
It is, will fix.
>
> > + }
> > + return ret;
> > +}
> > +
> > +static int reds_libwebsocket_service_fd(RedsState *s, struct
> > pollfd *pfd)
> > +{
> > + int ret;
> > + if (s->ws_in_service_fd) {
> > + return 0;
> > + }
> > + s->ws_in_service_fd = 1;
> > + ret = libwebsocket_service_fd(s->ws_context, pfd);
> > + s->ws_in_service_fd = 0;
>
> Why do we need this ws_in_service_fd variable?
>
To avoid recursively calling libwebsocket_service_fd, which we don't need and I don't want to find out if it is supported the hard way.
> > + if (ret != 0) {
> > + if (errno == EAGAIN) {
> > + spice_debug("libwebsocket_servide_fd EAGAIN,
> > pfd->revents = %d",
>
> typo: servide
Thanks, wlil fix.
>
> Apart from these comments, this looks good (and the code is not
> invasive
> anyway).
>
> Christophe
>
More information about the Spice-devel
mailing list