[Spice-devel] [PATCH 02/10] server: remove dispatching creation of worker channels
Frediano Ziglio
fziglio at redhat.com
Thu Nov 5 03:25:17 PST 2015
>
> On Thu, Nov 5, 2015 at 10:14 AM, Frediano Ziglio <fziglio at redhat.com> wrote:
> > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> >
> > ---
> > server/dispatcher.c | 6 ++--
> > server/red_dispatcher.c | 74
> > ++++++++++++++-------------------------------
> > server/red_dispatcher.h | 36 ++--------------------
> > server/red_worker.c | 68 ++++++++++++++---------------------------
> > server/red_worker.h | 2 ++
> > server/spice_server_utils.h | 1 -
> > 6 files changed, 51 insertions(+), 136 deletions(-)
> >
> > diff --git a/server/dispatcher.c b/server/dispatcher.c
> > index d334117..945edba 100644
> > --- a/server/dispatcher.c
> > +++ b/server/dispatcher.c
> > @@ -32,7 +32,6 @@
> > #include "common/mem.h"
> > #include "common/spice_common.h"
> > #include "dispatcher.h"
> > -#include "red_dispatcher.h"
> >
> > //#define DEBUG_DISPATCHER
> >
> > @@ -203,12 +202,13 @@ unlock:
> >
> > uint32_t dispatcher_read_message(Dispatcher *dispatcher)
> > {
> > - uint32_t message;
> > + uint32_t message = 0;
> >
> > spice_return_val_if_fail(dispatcher, 0);
> > spice_return_val_if_fail(dispatcher->send_fd != -1, 0);
> >
> > - receive_data(dispatcher->send_fd, &message, sizeof(message));
> > + if (read_safe(dispatcher->send_fd, (uint8_t*)&message,
> > sizeof(message), 1) == -1)
> > + spice_warn_if_reached();
> >
> > return message;
> > }
> > diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
> > index 43f061d..7855776 100644
> > --- a/server/red_dispatcher.c
> > +++ b/server/red_dispatcher.c
> > @@ -1001,35 +1001,10 @@ void red_dispatcher_async_complete(struct
> > RedDispatcher *dispatcher,
> > free(async_command);
> > }
> >
> > -static RedChannel *red_dispatcher_display_channel_create(RedDispatcher
> > *dispatcher)
> > -{
> > - RedWorkerMessageDisplayChannelCreate payload;
> > - RedChannel *display_channel;
> > -
> > - dispatcher_send_message(&dispatcher->dispatcher,
> > - RED_WORKER_MESSAGE_DISPLAY_CHANNEL_CREATE,
> > - &payload);
> > - receive_data(dispatcher->dispatcher.send_fd, &display_channel,
> > sizeof(RedChannel *));
> > - return display_channel;
> > -}
> > -
> > -static RedChannel *red_dispatcher_cursor_channel_create(RedDispatcher
> > *dispatcher)
> > -{
> > - RedWorkerMessageCursorChannelCreate payload;
> > - RedChannel *cursor_channel;
> > -
> > - dispatcher_send_message(&dispatcher->dispatcher,
> > - RED_WORKER_MESSAGE_CURSOR_CHANNEL_CREATE,
> > - &payload);
> > - receive_data(dispatcher->dispatcher.send_fd, &cursor_channel,
> > sizeof(RedChannel *));
> > - return cursor_channel;
> > -}
> > -
> > void red_dispatcher_init(QXLInstance *qxl)
> > {
> > RedDispatcher *red_dispatcher;
> > - RedChannel *display_channel;
> > - RedChannel *cursor_channel;
> > + RedChannel *channel;
> > ClientCbs client_cbs = { NULL, };
> >
> > spice_return_if_fail(qxl != NULL);
> > @@ -1074,34 +1049,29 @@ void red_dispatcher_init(QXLInstance *qxl)
> >
> > // TODO: reference and free
> > RedWorker *worker = red_worker_new(qxl, red_dispatcher);
> > - red_worker_run(worker);
> > -
> > - num_active_workers = 1;
> >
> > - display_channel =
> > red_dispatcher_display_channel_create(red_dispatcher);
> > -
> > - if (display_channel) {
> > - client_cbs.connect = red_dispatcher_set_display_peer;
> > - client_cbs.disconnect = red_dispatcher_disconnect_display_peer;
> > - client_cbs.migrate = red_dispatcher_display_migrate;
> > - red_channel_register_client_cbs(display_channel, &client_cbs);
> > - red_channel_set_data(display_channel, red_dispatcher);
> > - red_channel_set_cap(display_channel,
> > SPICE_DISPLAY_CAP_MONITORS_CONFIG);
> > - red_channel_set_cap(display_channel,
> > SPICE_DISPLAY_CAP_PREF_COMPRESSION);
> > - red_channel_set_cap(display_channel,
> > SPICE_DISPLAY_CAP_STREAM_REPORT);
> > - reds_register_channel(display_channel);
> > - }
> > -
> > - cursor_channel = red_dispatcher_cursor_channel_create(red_dispatcher);
> > + // TODO: move to their respective channel files
> > + channel = red_worker_get_cursor_channel(worker);
> > + client_cbs.connect = red_dispatcher_set_cursor_peer;
> > + client_cbs.disconnect = red_dispatcher_disconnect_cursor_peer;
> > + client_cbs.migrate = red_dispatcher_cursor_migrate;
> > + red_channel_register_client_cbs(channel, &client_cbs);
> > + red_channel_set_data(channel, red_dispatcher);
> > + reds_register_channel(channel);
> > +
> > + channel = red_worker_get_display_channel(worker);
> > + client_cbs.connect = red_dispatcher_set_display_peer;
> > + client_cbs.disconnect = red_dispatcher_disconnect_display_peer;
> > + client_cbs.migrate = red_dispatcher_display_migrate;
> > + red_channel_register_client_cbs(channel, &client_cbs);
> > + red_channel_set_data(channel, red_dispatcher);
> > + red_channel_set_cap(channel, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
> > + red_channel_set_cap(channel, SPICE_DISPLAY_CAP_PREF_COMPRESSION);
> > + red_channel_set_cap(channel, SPICE_DISPLAY_CAP_STREAM_REPORT);
> > + reds_register_channel(channel);
> >
> > - if (cursor_channel) {
> > - client_cbs.connect = red_dispatcher_set_cursor_peer;
> > - client_cbs.disconnect = red_dispatcher_disconnect_cursor_peer;
> > - client_cbs.migrate = red_dispatcher_cursor_migrate;
> > - red_channel_register_client_cbs(cursor_channel, &client_cbs);
> > - red_channel_set_data(cursor_channel, red_dispatcher);
> > - reds_register_channel(cursor_channel);
> > - }
> > + red_worker_run(worker);
> > + num_active_workers = 1;
> >
> > qxl->st->dispatcher = red_dispatcher;
> > red_dispatcher->next = dispatchers;
> > diff --git a/server/red_dispatcher.h b/server/red_dispatcher.h
> > index 242143a..02337b8 100644
> > --- a/server/red_dispatcher.h
> > +++ b/server/red_dispatcher.h
> > @@ -45,38 +45,6 @@ void
> > red_dispatcher_client_monitors_config(VDAgentMonitorsConfig
> > *monitors_confi
> >
> > typedef uint32_t RedWorkerMessage;
> >
> > -static inline void send_data(int fd, void *in_buf, int n)
> > -{
> > - uint8_t *buf = in_buf;
> > - do {
> > - int now;
> > - if ((now = write(fd, buf, n)) == -1) {
> > - if (errno == EINTR) {
> > - continue;
> > - }
> > - spice_error("%s", strerror(errno));
> > - }
> > - buf += now;
> > - n -= now;
> > - } while (n);
> > -}
> > -
> > -static inline void receive_data(int fd, void *in_buf, int n)
> > -{
> > - uint8_t *buf = in_buf;
> > - do {
> > - int now;
> > - if ((now = read(fd, buf, n)) == -1) {
> > - if (errno == EINTR) {
> > - continue;
> > - }
> > - spice_error("%s", strerror(errno));
> > - }
> > - buf += now;
> > - n -= now;
> > - } while (n);
> > -}
> > -
> > /* Keep message order, only append new messages!
> > * Replay code store enum values into save files.
> > */
> > @@ -119,8 +87,8 @@ enum {
> > /* suspend/windows resolution change command */
> > RED_WORKER_MESSAGE_FLUSH_SURFACES_ASYNC,
> >
> > - RED_WORKER_MESSAGE_DISPLAY_CHANNEL_CREATE,
> > - RED_WORKER_MESSAGE_CURSOR_CHANNEL_CREATE,
> > + RED_WORKER_MESSAGE_DISPLAY_CHANNEL_CREATE, /* unused */
> > + RED_WORKER_MESSAGE_CURSOR_CHANNEL_CREATE, /* unused */
> >
> > RED_WORKER_MESSAGE_MONITORS_CONFIG_ASYNC,
> > RED_WORKER_MESSAGE_DRIVER_UNLOAD,
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index 821ea75..d8a8a4a 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -436,8 +436,6 @@ typedef struct RedWorker {
> > clockid_t clockid;
> > QXLInstance *qxl;
> > RedDispatcher *red_dispatcher;
> > -
> > - int channel;
> > int running;
> > struct pollfd poll_fds[MAX_EVENT_SOURCES];
> > struct SpiceWatch watches[MAX_EVENT_SOURCES];
> > @@ -9903,23 +9901,6 @@ void handle_dev_create_primary_surface_async(void
> > *opaque, void *payload)
> > dev_create_primary_surface(worker, msg->surface_id, msg->surface);
> > }
> >
> > -/* exception for Dispatcher, data going from red_worker to main thread,
> > - * TODO: use a different dispatcher?
> > - * TODO: leave direct usage of channel(fd)? It's only used right after the
> > - * pthread is created, since the channel duration is the lifetime of the
> > spice
> > - * server. */
> > -
> > -void handle_dev_display_channel_create(void *opaque, void *payload)
> > -{
> > - RedWorker *worker = opaque;
> > -
> > - RedChannel *red_channel;
> > - // TODO: handle seemless migration. Temp, setting migrate to FALSE
> > - display_channel_create(worker, FALSE);
> > - red_channel = &worker->display_channel->common.base;
> > - send_data(worker->channel, &red_channel, sizeof(RedChannel *));
> > -}
> > -
> > void handle_dev_display_connect(void *opaque, void *payload)
> > {
> > RedWorkerMessageDisplayConnect *msg = payload;
> > @@ -10011,21 +9992,6 @@ static void handle_dev_monitors_config_async(void
> > *opaque, void *payload)
> > }
> >
> > /* TODO: special, perhaps use another dispatcher? */
>
> Remove the comment as well, please.
>
> > -void handle_dev_cursor_channel_create(void *opaque, void *payload)
> > -{
> > - RedWorker *worker = opaque;
> > - RedChannel *red_channel;
> > -
> > - if (!worker->cursor_channel) {
> > - worker->cursor_channel = cursor_channel_new(worker);
> > - } else {
> > - spice_warning("cursor channel already created");
> > - }
> > -
> > - red_channel = RED_CHANNEL(worker->cursor_channel);
> > - send_data(worker->channel, &red_channel, sizeof(RedChannel *));
> > -}
> > -
> > void handle_dev_cursor_connect(void *opaque, void *payload)
> > {
> > RedWorkerMessageCursorConnect *msg = payload;
> > @@ -10371,16 +10337,6 @@ static void register_callbacks(Dispatcher
> > *dispatcher)
> > sizeof(RedWorkerMessageSetMouseMode),
> > DISPATCHER_NONE);
> > dispatcher_register_handler(dispatcher,
> > - RED_WORKER_MESSAGE_DISPLAY_CHANNEL_CREATE,
> > - handle_dev_display_channel_create,
> > -
> > sizeof(RedWorkerMessageDisplayChannelCreate),
> > - DISPATCHER_NONE);
> > - dispatcher_register_handler(dispatcher,
> > - RED_WORKER_MESSAGE_CURSOR_CHANNEL_CREATE,
> > - handle_dev_cursor_channel_create,
> > -
> > sizeof(RedWorkerMessageCursorChannelCreate),
> > - DISPATCHER_NONE);
> > - dispatcher_register_handler(dispatcher,
> > RED_WORKER_MESSAGE_DESTROY_SURFACE_WAIT,
> > handle_dev_destroy_surface_wait,
> > sizeof(RedWorkerMessageDestroySurfaceWait),
> > @@ -10445,7 +10401,6 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> > RedDispatcher *red_dispatcher)
> >
> > worker->red_dispatcher = red_dispatcher;
> > worker->qxl = qxl;
> > - worker->channel = dispatcher_get_recv_fd(dispatcher);
> > register_callbacks(dispatcher);
> > if (worker->record_fd) {
> > dispatcher_register_universal_handler(dispatcher,
> > worker_dispatcher_record);
> > @@ -10474,7 +10429,7 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> > RedDispatcher *red_dispatcher)
> > worker->poll_fds[i].fd = -1;
> > }
> >
> > - worker->poll_fds[0].fd = worker->channel;
> > + worker->poll_fds[0].fd = dispatcher_get_recv_fd(dispatcher);
> > worker->poll_fds[0].events = POLLIN;
> > worker->watches[0].worker = worker;
> > worker->watches[0].watch_func = handle_dev_input;
> > @@ -10499,6 +10454,10 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> > RedDispatcher *red_dispatcher)
> > red_init_zlib(worker);
> > worker->event_timeout = INF_EVENT_WAIT;
> >
> > + worker->cursor_channel = cursor_channel_new(worker);
> > + // TODO: handle seemless migration. Temp, setting migrate to FALSE
> > + display_channel_create(worker, FALSE);
> > +
> > return worker;
> > }
> >
> > @@ -10525,6 +10484,9 @@ SPICE_GNUC_NORETURN static void
> > *red_worker_main(void *arg)
> > spice_warning("getcpuclockid failed");
> > }
> >
> > + RED_CHANNEL(worker->cursor_channel)->thread_id = pthread_self();
> > + RED_CHANNEL(worker->display_channel)->thread_id = pthread_self();
> > +
> > for (;;) {
> > int i, num_events;
> > unsigned int timeout;
> > @@ -10609,3 +10571,17 @@ bool red_worker_run(RedWorker *worker)
> >
> > return r == 0;
> > }
> > +
> > +RedChannel* red_worker_get_cursor_channel(RedWorker *worker)
> > +{
> > + spice_return_val_if_fail(worker, NULL);
> > +
> > + return RED_CHANNEL(worker->cursor_channel);
> > +}
> > +
> > +RedChannel* red_worker_get_display_channel(RedWorker *worker)
> > +{
> > + spice_return_val_if_fail(worker, NULL);
> > +
> > + return RED_CHANNEL(worker->display_channel);
> > +}
> > diff --git a/server/red_worker.h b/server/red_worker.h
> > index 33dd974..2995b8f 100644
> > --- a/server/red_worker.h
> > +++ b/server/red_worker.h
> > @@ -106,6 +106,8 @@ static inline void red_pipes_add_verb(RedChannel
> > *channel, uint16_t verb)
> > RedWorker* red_worker_new(QXLInstance *qxl, RedDispatcher
> > *red_dispatcher);
> > bool red_worker_run(RedWorker *worker);
> > QXLInstance* red_worker_get_qxl(RedWorker *worker);
> > +RedChannel* red_worker_get_cursor_channel(RedWorker *worker);
> > +RedChannel* red_worker_get_display_channel(RedWorker *worker);
> >
> > RedChannel *red_worker_new_channel(RedWorker *worker, int size,
> > const char *name,
> > diff --git a/server/spice_server_utils.h b/server/spice_server_utils.h
> > index 1f5b7f1..087ad6f 100644
> > --- a/server/spice_server_utils.h
> > +++ b/server/spice_server_utils.h
> > @@ -37,5 +37,4 @@ static inline int test_bit(int index, uint32_t val)
> > {
> > return val & (1u << index);
> > }
> > -
> > #endif
> > --
> > 2.4.3
> >
>
> Looks good to me. ACK!
>
Christophe proposed to split up the patch.
Christophe: are you going to split it up or propose how to do it?
Frediano
More information about the Spice-devel
mailing list