[Spice-devel] [PATCH 7/9] worker: remove need for WorkerInitData
Frediano Ziglio
fziglio at redhat.com
Fri Oct 23 05:06:57 PDT 2015
>
> On Wed, 2015-10-21 at 13:00 +0100, Frediano Ziglio wrote:
> > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> >
> > Move code around to declare and place it where it fits better.
> > ---
> > server/red_dispatcher.c | 106 +++++++++++++-------------------------
> > ----------
> > server/red_dispatcher.h | 7 ++++
> > server/red_worker.c | 56 +++++++++++++------------
> > server/red_worker.h | 35 +---------------
> > server/reds.c | 44 +++++++++++++++++++-
> > server/reds.h | 16 ++++++++
> > 6 files changed, 125 insertions(+), 139 deletions(-)
> >
> > diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
> > index c43da7d..3e7a5e1 100644
> > --- a/server/red_dispatcher.c
> > +++ b/server/red_dispatcher.c
> > @@ -68,11 +68,6 @@ struct RedDispatcher {
> > unsigned int max_monitors;
> > };
> >
> > -extern uint32_t streaming_video;
> > -extern SpiceImageCompression image_compression;
> > -extern spice_wan_compression_t jpeg_state;
> > -extern spice_wan_compression_t zlib_glz_state;
> > -
> > static RedDispatcher *dispatchers = NULL;
> >
> > static int red_dispatcher_check_qxl_version(RedDispatcher *rd, int
> > major, int minor)
> > @@ -204,46 +199,6 @@ static void
> > red_dispatcher_cursor_migrate(RedChannelClient *rcc)
> > &payload);
> > }
> >
> > -typedef struct RendererInfo {
> > - int id;
> > - const char *name;
> > -} RendererInfo;
> > -
> > -static RendererInfo renderers_info[] = {
> > - {RED_RENDERER_SW, "sw"},
> > -#ifdef USE_OPENGL
> > - {RED_RENDERER_OGL_PBUF, "oglpbuf"},
> > - {RED_RENDERER_OGL_PIXMAP, "oglpixmap"},
> > -#endif
> > - {RED_RENDERER_INVALID, NULL},
> > -};
> > -
> > -static uint32_t renderers[RED_RENDERER_LAST];
> > -static uint32_t num_renderers = 0;
> > -
> > -static RendererInfo *find_renderer(const char *name)
> > -{
> > - RendererInfo *inf = renderers_info;
> > - while (inf->name) {
> > - if (strcmp(name, inf->name) == 0) {
> > - return inf;
> > - }
> > - inf++;
> > - }
> > - return NULL;
> > -}
> > -
> > -int red_dispatcher_add_renderer(const char *name)
> > -{
> > - RendererInfo *inf;
> > -
> > - if (num_renderers == RED_RENDERER_LAST || !(inf =
> > find_renderer(name))) {
> > - return FALSE;
> > - }
> > - renderers[num_renderers++] = inf->id;
> > - return TRUE;
> > -}
> > -
> > int red_dispatcher_qxl_count(void)
> > {
> > return num_active_workers;
> > @@ -623,14 +578,24 @@ static void qxl_worker_reset_memslots(QXLWorker
> > *qxl_worker)
> > red_dispatcher_reset_memslots((RedDispatcher*)qxl_worker);
> > }
> >
> > +static bool red_dispatcher_set_pending(RedDispatcher *dispatcher,
> > int pending)
> > +{
> > + // TODO: this is not atomic, do we need atomic?
> > + if (test_bit(pending, dispatcher->pending)) {
> > + return TRUE;
> > + }
> > +
> > + set_bit(pending, &dispatcher->pending);
> > + return FALSE;
> > +}
> > +
> > static void red_dispatcher_wakeup(RedDispatcher *dispatcher)
> > {
> > RedWorkerMessageWakeup payload;
> >
> > - if (test_bit(RED_WORKER_PENDING_WAKEUP, dispatcher->pending)) {
> > + if (red_dispatcher_set_pending(dispatcher,
> > RED_DISPATCHER_PENDING_WAKEUP))
> > return;
> > - }
> > - set_bit(RED_WORKER_PENDING_WAKEUP, &dispatcher->pending);
> > +
> > dispatcher_send_message(&dispatcher->dispatcher,
> > RED_WORKER_MESSAGE_WAKEUP,
> > &payload);
> > @@ -645,10 +610,9 @@ static void red_dispatcher_oom(RedDispatcher
> > *dispatcher)
> > {
> > RedWorkerMessageOom payload;
> >
> > - if (test_bit(RED_WORKER_PENDING_OOM, dispatcher->pending)) {
> > + if (red_dispatcher_set_pending(dispatcher,
> > RED_DISPATCHER_PENDING_OOM))
> > return;
> > - }
> > - set_bit(RED_WORKER_PENDING_OOM, &dispatcher->pending);
> > +
>
>
> These "pending" changes seem unrelated. I would split them into a separate
> commit.
>
Separate, I'll send another updated patchset.
>
> > dispatcher_send_message(&dispatcher->dispatcher,
> > RED_WORKER_MESSAGE_OOM,
> > &payload);
> > @@ -1063,12 +1027,11 @@ static RedChannel
> > *red_dispatcher_cursor_channel_create(RedDispatcher *dispatche
> > RedDispatcher *red_dispatcher_new(QXLInstance *qxl)
> > {
> > RedDispatcher *red_dispatcher;
> > - WorkerInitData init_data;
> > - QXLDevInitInfo init_info;
> > RedChannel *display_channel;
> > RedChannel *cursor_channel;
> > ClientCbs client_cbs = { NULL, };
> >
> > + spice_return_val_if_fail(qxl != NULL, NULL);
> > spice_return_val_if_fail(qxl->st->dispatcher == NULL, NULL);
> >
> > static gsize initialized = FALSE;
> > @@ -1082,22 +1045,11 @@ RedDispatcher *red_dispatcher_new(QXLInstance
> > *qxl)
> > }
> >
> > red_dispatcher = spice_new0(RedDispatcher, 1);
> > + red_dispatcher->qxl = qxl;
> > ring_init(&red_dispatcher->async_commands);
> > spice_debug("red_dispatcher->async_commands.next %p",
> > red_dispatcher->async_commands.next);
> > dispatcher_init(&red_dispatcher->dispatcher,
> > RED_WORKER_MESSAGE_COUNT, NULL);
> > - init_data.qxl = red_dispatcher->qxl = qxl;
> > - init_data.id = qxl->id;
> > - init_data.red_dispatcher = red_dispatcher;
> > - init_data.pending = &red_dispatcher->pending;
> > - init_data.num_renderers = num_renderers;
> > - memcpy(init_data.renderers, renderers,
> > sizeof(init_data.renderers));
> > -
> > pthread_mutex_init(&red_dispatcher->async_lock, NULL);
> > - init_data.image_compression = image_compression;
> > - init_data.jpeg_state = jpeg_state;
> > - init_data.zlib_glz_state = zlib_glz_state;
> > - init_data.streaming_video = streaming_video;
> > -
> > red_dispatcher->base.major_version = SPICE_INTERFACE_QXL_MAJOR;
> > red_dispatcher->base.minor_version = SPICE_INTERFACE_QXL_MINOR;
> > red_dispatcher->base.wakeup = qxl_worker_wakeup;
> > @@ -1111,7 +1063,6 @@ RedDispatcher *red_dispatcher_new(QXLInstance
> > *qxl)
> > red_dispatcher->base.destroy_surfaces =
> > qxl_worker_destroy_surfaces;
> > red_dispatcher->base.create_primary_surface =
> > qxl_worker_create_primary_surface;
> > red_dispatcher->base.destroy_primary_surface =
> > qxl_worker_destroy_primary_surface;
> > -
>
> This whitespace change is rather superfluous. I'd probably just omit
> it.
>
removed
> > red_dispatcher->base.reset_image_cache =
> > qxl_worker_reset_image_cache;
> > red_dispatcher->base.reset_cursor = qxl_worker_reset_cursor;
> > red_dispatcher->base.destroy_surface_wait =
> > qxl_worker_destroy_surface_wait;
> > @@ -1119,20 +1070,12 @@ RedDispatcher *red_dispatcher_new(QXLInstance
> > *qxl)
> >
> > red_dispatcher->max_monitors = UINT_MAX;
> >
> > - qxl->st->qif->get_init_info(qxl, &init_info);
> > -
> > - init_data.memslot_id_bits = init_info.memslot_id_bits;
> > - init_data.memslot_gen_bits = init_info.memslot_gen_bits;
> > - init_data.num_memslots = init_info.num_memslots;
> > - init_data.num_memslots_groups = init_info.num_memslots_groups;
> > - init_data.internal_groupslot_id =
> > init_info.internal_groupslot_id;
> > - init_data.n_surfaces = init_info.n_surfaces;
> > + // TODO: reference and free
> > + RedWorker *worker = red_worker_new(qxl, red_dispatcher);
> > + red_worker_run(worker);
> >
> > num_active_workers = 1;
> >
> > - // TODO: reference and free
> > - RedWorker *worker = red_worker_new(&init_data);
> > - red_worker_run(worker);
> > display_channel =
> > red_dispatcher_display_channel_create(red_dispatcher);
> >
> > if (display_channel) {
> > @@ -1173,8 +1116,15 @@ struct Dispatcher
> > *red_dispatcher_get_dispatcher(RedDispatcher *red_dispatcher)
> > return &red_dispatcher->dispatcher;
> > }
> >
> > -void red_dispatcher_set_dispatcher_opaque(struct RedDispatcher
> > *red_dispatcher,
> > +void red_dispatcher_set_dispatcher_opaque(RedDispatcher
> > *red_dispatcher,
> > void *opaque)
>
> unrelated style change. split?
>
split and merged
> > {
> > dispatcher_set_opaque(&red_dispatcher->dispatcher, opaque);
> > }
> > +
> > +void red_dispatcher_clear_pending(RedDispatcher *red_dispatcher, int
> > pending)
> > +{
> > + spice_return_if_fail(red_dispatcher != NULL);
> > +
> > + clear_bit(pending, &red_dispatcher->pending);
> > +}
> > diff --git a/server/red_dispatcher.h b/server/red_dispatcher.h
> > index f487317..3bdeac0 100644
> > --- a/server/red_dispatcher.h
> > +++ b/server/red_dispatcher.h
> > @@ -293,4 +293,11 @@ typedef struct
> > RedWorkerMessageMonitorsConfigAsync {
> > typedef struct RedWorkerMessageDriverUnload {
> > } RedWorkerMessageDriverUnload;
> >
> > +enum {
> > + RED_DISPATCHER_PENDING_WAKEUP,
> > + RED_DISPATCHER_PENDING_OOM,
> > +};
> > +
> > +void red_dispatcher_clear_pending(RedDispatcher *red_dispatcher, int
> > pending);
> > +
>
> Another "pending" change. Split to a separate commit.
>
> > #endif
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index 65961db..7f73409 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -869,10 +869,11 @@ typedef struct RedWorker {
> > int channel;
> > int id;
> > int running;
> > - uint32_t *pending;
>
> Again.
>
> > struct pollfd poll_fds[MAX_EVENT_SOURCES];
> > struct SpiceWatch watches[MAX_EVENT_SOURCES];
> > unsigned int event_timeout;
> > +
> > + gint timeout;
> > uint32_t repoll_cmd_ring;
> > uint32_t repoll_cursor_ring;
> > uint32_t num_renderers;
> > @@ -11271,8 +11272,8 @@ void handle_dev_wakeup(void *opaque, void
> > *payload)
> > {
> > RedWorker *worker = opaque;
> >
> > - clear_bit(RED_WORKER_PENDING_WAKEUP, worker->pending);
> > stat_inc_counter(worker->wakeup_counter, 1);
> > + red_dispatcher_clear_pending(worker->red_dispatcher,
> > RED_DISPATCHER_PENDING_WAKEUP);
> > }
> >
> > void handle_dev_oom(void *opaque, void *payload)
> > @@ -11305,7 +11306,7 @@ void handle_dev_oom(void *opaque, void
> > *payload)
> > worker->current_size,
> > worker->display_channel ?
> > red_channel_sum_pipes_size(display_red_channel) :
> > 0);
> > - clear_bit(RED_WORKER_PENDING_OOM, worker->pending);
> > + red_dispatcher_clear_pending(worker->red_dispatcher,
> > RED_DISPATCHER_PENDING_OOM);
>
> two more "pending" chunks to split.
>
> > }
> >
> > void handle_dev_reset_cursor(void *opaque, void *payload)
> > @@ -11852,15 +11853,20 @@ static void handle_dev_input(int fd, int
> > event, void *opaque)
> > dispatcher_handle_recv_read(red_dispatcher_get_dispatcher(worker
> > ->red_dispatcher));
> > }
> >
> > -RedWorker* red_worker_new(WorkerInitData *init_data)
> > +RedWorker* red_worker_new(QXLInstance *qxl, RedDispatcher
> > *red_dispatcher)
> > {
> > - RedWorker *worker = spice_new0(RedWorker, 1);
> > + QXLDevInitInfo init_info;
> > + RedWorker *worker;
> > Dispatcher *dispatcher;
> > int i;
> > const char *record_filename;
> >
> > spice_assert(sizeof(CursorItem) <= QXL_CURSUR_DEVICE_DATA_SIZE);
> >
> > + qxl->st->qif->get_init_info(qxl, &init_info);
> > +
> > + worker = spice_new0(RedWorker, 1);
> > +
> > record_filename = getenv("SPICE_WORKER_RECORD_FILENAME");
> > if (record_filename) {
> > static const char header[] = "SPICE_REPLAY 1\n";
> > @@ -11873,27 +11879,27 @@ RedWorker* red_worker_new(WorkerInitData
> > *init_data)
> > spice_error("failed to write replay header");
> > }
> > }
> > - dispatcher = red_dispatcher_get_dispatcher(init_data
> > ->red_dispatcher);
> > + dispatcher = red_dispatcher_get_dispatcher(red_dispatcher);
> > dispatcher_set_opaque(dispatcher, worker);
> > - worker->red_dispatcher = init_data->red_dispatcher;
> > - worker->qxl = init_data->qxl;
> > - worker->id = init_data->id;
> > +
> > + worker->red_dispatcher = red_dispatcher;
> > + worker->qxl = qxl;
> > + worker->id = qxl->id;
> > worker->channel = dispatcher_get_recv_fd(dispatcher);
> > register_callbacks(dispatcher);
> > if (worker->record_fd) {
> > dispatcher_register_universal_handler(dispatcher,
> > worker_dispatcher_record);
> > }
> > - worker->pending = init_data->pending;
> > worker->cursor_visible = TRUE;
> > - spice_assert(init_data->num_renderers > 0);
> > - worker->num_renderers = init_data->num_renderers;
> > - memcpy(worker->renderers, init_data->renderers, sizeof(worker
> > ->renderers));
> > + spice_assert(num_renderers > 0);
> > + worker->num_renderers = num_renderers;
> > + memcpy(worker->renderers, renderers, sizeof(worker->renderers));
> > worker->renderer = RED_RENDERER_INVALID;
> > worker->mouse_mode = SPICE_MOUSE_MODE_SERVER;
> > - worker->image_compression = init_data->image_compression;
> > - worker->jpeg_state = init_data->jpeg_state;
> > - worker->zlib_glz_state = init_data->zlib_glz_state;
> > - worker->streaming_video = init_data->streaming_video;
> > + worker->image_compression = image_compression;
> > + worker->jpeg_state = jpeg_state;
> > + worker->zlib_glz_state = zlib_glz_state;
> > + worker->streaming_video = streaming_video;
> > worker->driver_cap_monitors_config = 0;
> > ring_init(&worker->current_list);
> > image_cache_init(&worker->image_cache);
> > @@ -11922,14 +11928,14 @@ RedWorker* red_worker_new(WorkerInitData
> > *init_data)
> > worker->watches[0].watch_func_opaque = worker;
> >
> > red_memslot_info_init(&worker->mem_slots,
> > - init_data->num_memslots_groups,
> > - init_data->num_memslots,
> > - init_data->memslot_gen_bits,
> > - init_data->memslot_id_bits,
> > - init_data->internal_groupslot_id);
> > -
> > - spice_warn_if(init_data->n_surfaces > NUM_SURFACES);
> > - worker->n_surfaces = init_data->n_surfaces;
> > + init_info.num_memslots_groups,
> > + init_info.num_memslots,
> > + init_info.memslot_gen_bits,
> > + init_info.memslot_id_bits,
> > + init_info.internal_groupslot_id);
> > +
> > + spice_warn_if(init_info.n_surfaces > NUM_SURFACES);
> > + worker->n_surfaces = init_info.n_surfaces;
> >
> > if (!spice_timer_queue_create()) {
> > spice_error("failed to create timer queue");
> > diff --git a/server/red_worker.h b/server/red_worker.h
> > index c935e0a..c1adca4 100644
> > --- a/server/red_worker.h
> > +++ b/server/red_worker.h
> > @@ -23,42 +23,9 @@
> > #include "red_common.h"
> > #include "red_dispatcher.h"
> >
> > -enum {
> > - RED_WORKER_PENDING_WAKEUP,
> > - RED_WORKER_PENDING_OOM,
> > -};
> > -
> > -enum {
> > - RED_RENDERER_INVALID,
> > - RED_RENDERER_SW,
> > - RED_RENDERER_OGL_PBUF,
> > - RED_RENDERER_OGL_PIXMAP,
> > -
> > - RED_RENDERER_LAST
> > -};
> > -
> > typedef struct RedWorker RedWorker;
> >
> > -typedef struct WorkerInitData {
> > - struct QXLInstance *qxl;
> > - int id;
> > - uint32_t *pending;
> > - uint32_t num_renderers;
> > - uint32_t renderers[RED_RENDERER_LAST];
> > - SpiceImageCompression image_compression;
> > - spice_wan_compression_t jpeg_state;
> > - spice_wan_compression_t zlib_glz_state;
> > - int streaming_video;
> > - uint32_t num_memslots;
> > - uint32_t num_memslots_groups;
> > - uint8_t memslot_gen_bits;
> > - uint8_t memslot_id_bits;
> > - uint8_t internal_groupslot_id;
> > - uint32_t n_surfaces;
> > - RedDispatcher *red_dispatcher;
> > -} WorkerInitData;
> > -
> > -RedWorker* red_worker_new(WorkerInitData *init_data);
> > +RedWorker* red_worker_new(QXLInstance *qxl, RedDispatcher
> > *red_dispatcher);
> > bool red_worker_run(RedWorker *worker);
> >
> > #endif
> > diff --git a/server/reds.c b/server/reds.c
> > index d53ea66..90d14b5 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -3364,6 +3364,46 @@ SPICE_GNUC_VISIBLE SpiceServer
> > *spice_server_new(void)
> > return reds;
> > }
> >
> > +typedef struct RendererInfo {
> > + int id;
> > + const char *name;
> > +} RendererInfo;
> > +
> > +static RendererInfo renderers_info[] = {
> > + {RED_RENDERER_SW, "sw"},
> > +#ifdef USE_OPENGL
> > + {RED_RENDERER_OGL_PBUF, "oglpbuf"},
> > + {RED_RENDERER_OGL_PIXMAP, "oglpixmap"},
> > +#endif
> > + {RED_RENDERER_INVALID, NULL},
> > +};
> > +
> > +uint32_t renderers[RED_RENDERER_LAST];
> > +uint32_t num_renderers = 0;
> > +
> > +static RendererInfo *find_renderer(const char *name)
> > +{
> > + RendererInfo *inf = renderers_info;
> > + while (inf->name) {
> > + if (strcmp(name, inf->name) == 0) {
> > + return inf;
> > + }
> > + inf++;
> > + }
> > + return NULL;
> > +}
> > +
> > +static int red_add_renderer(const char *name)
> > +{
> > + RendererInfo *inf;
> > +
> > + if (num_renderers == RED_RENDERER_LAST || !(inf =
> > find_renderer(name))) {
> > + return FALSE;
> > + }
> > + renderers[num_renderers++] = inf->id;
> > + return TRUE;
> > +}
> > +
> > SPICE_GNUC_VISIBLE int spice_server_init(SpiceServer *s,
> > SpiceCoreInterface *core)
> > {
> > int ret;
> > @@ -3371,7 +3411,7 @@ SPICE_GNUC_VISIBLE int
> > spice_server_init(SpiceServer *s, SpiceCoreInterface *cor
> > spice_assert(reds == s);
> > ret = do_spice_init(core);
> > if (default_renderer) {
> > - red_dispatcher_add_renderer(default_renderer);
> > + red_add_renderer(default_renderer);
> > }
> > return ret;
> > }
> > @@ -3664,7 +3704,7 @@ SPICE_GNUC_VISIBLE int
> > spice_server_is_server_mouse(SpiceServer *s)
> > SPICE_GNUC_VISIBLE int spice_server_add_renderer(SpiceServer *s,
> > const char *name)
> > {
> > spice_assert(reds == s);
> > - if (!red_dispatcher_add_renderer(name)) {
> > + if (!red_add_renderer(name)) {
> > return -1;
> > }
> > default_renderer = NULL;
> > diff --git a/server/reds.h b/server/reds.h
> > index a9c2df9..7937d2d 100644
> > --- a/server/reds.h
> > +++ b/server/reds.h
> > @@ -59,7 +59,23 @@ int reds_get_agent_mouse(void); // used by
> > inputs_channel
> > int reds_has_vdagent(void); // used by inputs channel
> > void reds_handle_agent_mouse_event(const VDAgentMouseState
> > *mouse_state); // used by inputs_channel
> >
> > +enum {
> > + RED_RENDERER_INVALID,
> > + RED_RENDERER_SW,
> > + RED_RENDERER_OGL_PBUF,
> > + RED_RENDERER_OGL_PIXMAP,
> > +
> > + RED_RENDERER_LAST
> > +};
> > +
> > +extern uint32_t renderers[RED_RENDERER_LAST];
> > +extern uint32_t num_renderers;
> > +
> > extern struct SpiceCoreInterface *core;
> > +extern uint32_t streaming_video;
> > +extern SpiceImageCompression image_compression;
> > +extern spice_wan_compression_t jpeg_state;
> > +extern spice_wan_compression_t zlib_glz_state;
> >
> > // Temporary measures to make splitting reds.c to inputs_channel.c
> > easier
> >
>
Frediano
More information about the Spice-devel
mailing list