[Spice-devel] [PATCH 4/5] server: add websockets support via libwebsockets
Christophe Fergeau
cfergeau at redhat.com
Tue Oct 23 03:07:02 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
> +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
> $(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.
> + }
> + 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?
> + }
> + 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?
> + if (ret != 0) {
> + if (errno == EAGAIN) {
> + spice_debug("libwebsocket_servide_fd EAGAIN, pfd->revents = %d",
typo: servide
Apart from these comments, this looks good (and the code is not invasive
anyway).
Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20121023/f668ddc0/attachment.pgp>
More information about the Spice-devel
mailing list