[Spice-devel] [PATCH spice-server] RFC: Remove DISPATCHER_ASYNC type
Jonathon Jongsma
jjongsma at redhat.com
Wed Sep 6 19:46:53 UTC 2017
And apparently I didn't look at mail for a while and Frediano decided
to do the same thing after our little discussion...
On Wed, 2017-09-06 at 14:03 -0500, Jonathon Jongsma wrote:
> ---
>
> From a previous conversation:
> On Wed, 2017-09-06 at 13:14 -0400, Frediano Ziglio wrote:
> > > [OT: it's a little bit unclear to me why the async_done handler
> > > is
> > > necessary at all. It is called immediately after the message-
> > > specific
> > > handler is called, and it is called from the same thread. So
> > > anything
> > > that is done in the async_done callback could just as easily be
> > > done in
> > > the message-specific handler, no? I suppose it allows you to
> > > reduce
> > > code duplication slightly, but that seems to be the only
> > > advantage.
> > > If
> > > this async_done feature did not exist, we could simply use a
> > > boolean
> > > argument for whether to send an ACK or not, and the enum would be
> > > unnecessary. But maybe I'm missing something.]
> > >
> >
> > Maybe you really got a nice idea! Remove the ASYNC and have
> > ACK/NONE
> > !
> > Would avoid the switch on the async call and also the code would
> > run
> > in the worker thread (that is code should be in a RedWorker or
> > {Display,Cursor}Channel but actually is in RedQXL which should not
> > have code that runs in the worker).
> >
> > Not checked how easy is...
> >
> > Frediano
>
> I guess an initial implementation would look something like this. I
> didn't
> really move any red-qxl code into red-worker, etc.
>
> server/dispatcher.c | 11 -----------
> server/dispatcher.h | 32 +-------------------------------
> server/red-worker.c | 41 ++++++++++++++++++-----------------------
> 3 files changed, 19 insertions(+), 65 deletions(-)
>
> diff --git a/server/dispatcher.c b/server/dispatcher.c
> index 76f5eeaf4..931c7efa0 100644
> --- a/server/dispatcher.c
> +++ b/server/dispatcher.c
> @@ -60,7 +60,6 @@ struct DispatcherPrivate {
> void *payload; /* allocated as max of message sizes */
> size_t payload_size; /* used to track realloc calls */
> void *opaque;
> - dispatcher_handle_async_done handle_async_done;
> dispatcher_handle_any_message any_handler;
> };
>
> @@ -309,8 +308,6 @@ static int
> dispatcher_handle_single_read(Dispatcher *dispatcher)
> spice_printerr("error writing ack for message %d",
> type);
> /* TODO: close socketpair? */
> }
> - } else if (msg->flag == DISPATCHER_FLAG_ASYNC && dispatcher-
> >priv->handle_async_done) {
> - dispatcher->priv->handle_async_done(dispatcher->priv-
> >opaque, type, payload);
> }
> return 1;
> }
> @@ -359,14 +356,6 @@ unlock:
> pthread_mutex_unlock(&dispatcher->priv->lock);
> }
>
> -void dispatcher_register_async_done_callback(
> - Dispatcher *dispatcher,
> - dispatcher_handle_async_done
> handler)
> -{
> - assert(dispatcher->priv->handle_async_done == NULL);
> - dispatcher->priv->handle_async_done = handler;
> -}
> -
> void dispatcher_register_handler(Dispatcher *dispatcher, uint32_t
> message_type,
> dispatcher_handle_message handler,
> size_t size, DispatcherFlag flag)
> diff --git a/server/dispatcher.h b/server/dispatcher.h
> index 28d78479e..837522243 100644
> --- a/server/dispatcher.h
> +++ b/server/dispatcher.h
> @@ -85,15 +85,6 @@ typedef void (*dispatcher_handle_any_message)(void
> *opaque,
> uint32_t message_type,
> void *payload);
>
> -/* The signature of a function that handles async done notifcations
> for all
> - * messages that are registered with DISPATCHER_FLAG_ASYNC flag. See
> - * dispatcher_register_async_done_callback() and
> dispatcher_register_handler()
> - * for more information. */
> -typedef void (*dispatcher_handle_async_done)(void *opaque,
> - uint32_t message_type,
> - void *payload);
> -
> -
> /* dispatcher_send_message
> *
> * Sends a message to the receiving thread. The message type must
> have been
> @@ -124,15 +115,10 @@ void dispatcher_send_message(Dispatcher
> *dispatcher, uint32_t message_type,
> * since the sender will block until it receives the ACK. This is
> useful when
> * the sender wants to ensure that the receiver has handled the
> message before
> * proceeding.
> - *
> - * DISPATCHER_FLAG_ASYNC - When a message is received, the message
> handler is
> - * called and then the dispatcher will call the registered
> async_done handler.
> - * See dispatcher_register_async_done_callback() for more
> information
> */
> typedef enum {
> DISPATCHER_FLAG_NONE = 0,
> - DISPATCHER_FLAG_ACK,
> - DISPATCHER_FLAG_ASYNC
> + DISPATCHER_FLAG_ACK
> } DispatcherFlag;
>
> /* dispatcher_register_handler
> @@ -155,22 +141,6 @@ void dispatcher_register_handler(Dispatcher
> *dispatcher, uint32_t message_type,
> dispatcher_handle_message handler,
> size_t size,
> DispatcherFlag flag);
>
> -/* dispatcher_register_async_done_callback
> - *
> - * register an async_done callback for this dispatcher. For all
> message types
> - * that were registered with a DISPATCHER_FLAG_ASYNC flag, this
> function will
> - * be called after the normal message handler, and the message will
> be passed
> - * to this function. Note that this function will execute within the
> receiving
> - * thread.
> - *
> - * @dispatcher: dispatcher
> - * @handler: callback on the receiver side called *after* the
> - * message callback in case flag ==
> DISPATCHER_FLAG_ASYNC.
> - */
> -void dispatcher_register_async_done_callback(
> - Dispatcher *dispatcher,
> - dispatcher_handle_async_done
> handler);
> -
> /* dispatcher_register_universal_handler
> *
> * Register a universal handler that will be called when *any*
> message is
> diff --git a/server/red-worker.c b/server/red-worker.c
> index c3f2dbfa0..cfeba7b35 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -431,6 +431,7 @@ static void handle_dev_update_async(void *opaque,
> void *payload)
> red_qxl_update_area_complete(worker->qxl, msg->surface_id,
> qxl_dirty_rects, num_dirty_rects);
> free(qxl_dirty_rects);
> + red_qxl_async_complete(worker->qxl, msg->base.cmd);
> }
>
> static void handle_dev_update(void *opaque, void *payload)
> @@ -577,14 +578,17 @@ static void
> handle_dev_destroy_primary_surface_async(void *opaque, void *payload
> uint32_t surface_id = msg->surface_id;
>
> destroy_primary_surface(worker, surface_id);
> + red_qxl_async_complete(worker->qxl, msg->base.cmd);
> }
>
> static void handle_dev_flush_surfaces_async(void *opaque, void
> *payload)
> {
> RedWorker *worker = opaque;
> + RedWorkerMessageFlushSurfacesAsync *msg = payload;
>
> flush_all_qxl_commands(worker);
> display_channel_flush_all_surfaces(worker->display_channel);
> + red_qxl_async_complete(worker->qxl, msg->base.cmd);
> }
>
> static void handle_dev_stop(void *opaque, void *payload)
> @@ -684,15 +688,18 @@ static void
> handle_dev_destroy_surface_wait_async(void *opaque, void *payload)
> RedWorker *worker = opaque;
>
> display_channel_destroy_surface_wait(worker->display_channel,
> msg->surface_id);
> + red_qxl_async_complete(worker->qxl, msg->base.cmd);
> }
>
> static void handle_dev_destroy_surfaces_async(void *opaque, void
> *payload)
> {
> RedWorker *worker = opaque;
> + RedWorkerMessageDestroySurfacesAsync *msg = payload;
>
> flush_all_qxl_commands(worker);
> display_channel_destroy_surfaces(worker->display_channel);
> cursor_channel_reset(worker->cursor_channel);
> + red_qxl_async_complete(worker->qxl, msg->base.cmd);
> }
>
> static void handle_dev_create_primary_surface_async(void *opaque,
> void *payload)
> @@ -701,6 +708,7 @@ static void
> handle_dev_create_primary_surface_async(void *opaque, void *payload)
> RedWorker *worker = opaque;
>
> dev_create_primary_surface(worker, msg->surface_id, msg-
> >surface);
> + red_qxl_async_complete(worker->qxl, msg->base.cmd);
> }
>
> static void handle_dev_display_connect(void *opaque, void *payload)
> @@ -797,6 +805,7 @@ static void handle_dev_monitors_config_async(void
> *opaque, void *payload)
> display_channel_update_monitors_config(worker->display_channel,
> dev_monitors_config,
> MIN(count, msg-
> >max_monitors),
> MIN(max_allowed, msg-
> >max_monitors));
> + red_qxl_async_complete(worker->qxl, msg->base.cmd);
> }
>
> /* TODO: special, perhaps use another dispatcher? */
> @@ -923,6 +932,7 @@ static void handle_dev_add_memslot_async(void
> *opaque, void *payload)
> RedWorker *worker = opaque;
>
> dev_add_memslot(worker, msg->mem_slot);
> + red_qxl_async_complete(worker->qxl, msg->base.cmd);
> }
>
> static void handle_dev_reset_memslots(void *opaque, void *payload)
> @@ -995,17 +1005,6 @@ static void handle_dev_loadvm_commands(void
> *opaque, void *payload)
> }
> }
>
> -static void worker_handle_dispatcher_async_done(void *opaque,
> - uint32_t
> message_type,
> - void *payload)
> -{
> - RedWorker *worker = opaque;
> - RedWorkerMessageAsync *msg_async = payload;
> -
> - spice_debug("trace");
> - red_qxl_async_complete(worker->qxl, msg_async->cmd);
> -}
> -
> static void worker_dispatcher_record(void *opaque, uint32_t
> message_type, void *payload)
> {
> RedWorker *worker = opaque;
> @@ -1015,10 +1014,6 @@ static void worker_dispatcher_record(void
> *opaque, uint32_t message_type, void *
>
> static void register_callbacks(Dispatcher *dispatcher)
> {
> - dispatcher_register_async_done_callback(
> - dispatcher,
> - worker_handle_dispatcher_async_d
> one);
> -
> /* TODO: register cursor & display specific msg in respective
> channel files */
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_DISPLAY_CONNECT,
> @@ -1059,7 +1054,7 @@ static void register_callbacks(Dispatcher
> *dispatcher)
> RED_WORKER_MESSAGE_UPDATE_ASYNC,
> handle_dev_update_async,
> sizeof(RedWorkerMessageUpdateAsync),
> - DISPATCHER_FLAG_ASYNC);
> + DISPATCHER_FLAG_NONE);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_ADD_MEMSLOT,
> handle_dev_add_memslot,
> @@ -1069,7 +1064,7 @@ static void register_callbacks(Dispatcher
> *dispatcher)
> RED_WORKER_MESSAGE_ADD_MEMSLOT_ASYNC
> ,
> handle_dev_add_memslot_async,
> sizeof(RedWorkerMessageAddMemslotAsy
> nc),
> - DISPATCHER_FLAG_ASYNC);
> + DISPATCHER_FLAG_NONE);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_DEL_MEMSLOT,
> handle_dev_del_memslot,
> @@ -1084,7 +1079,7 @@ static void register_callbacks(Dispatcher
> *dispatcher)
> RED_WORKER_MESSAGE_DESTROY_SURFACES_
> ASYNC,
> handle_dev_destroy_surfaces_async,
> sizeof(RedWorkerMessageDestroySurfac
> esAsync),
> - DISPATCHER_FLAG_ASYNC);
> + DISPATCHER_FLAG_NONE);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_DESTROY_PRIMARY_S
> URFACE,
> handle_dev_destroy_primary_surface,
> @@ -1094,12 +1089,12 @@ static void register_callbacks(Dispatcher
> *dispatcher)
> RED_WORKER_MESSAGE_DESTROY_PRIMARY_S
> URFACE_ASYNC,
> handle_dev_destroy_primary_surface_a
> sync,
> sizeof(RedWorkerMessageDestroyPrimar
> ySurfaceAsync),
> - DISPATCHER_FLAG_ASYNC);
> + DISPATCHER_FLAG_NONE);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_CREATE_PRIMARY_SU
> RFACE_ASYNC,
> handle_dev_create_primary_surface_as
> ync,
> sizeof(RedWorkerMessageCreatePrimary
> SurfaceAsync),
> - DISPATCHER_FLAG_ASYNC);
> + DISPATCHER_FLAG_NONE);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_CREATE_PRIMARY_SU
> RFACE,
> handle_dev_create_primary_surface,
> @@ -1134,7 +1129,7 @@ static void register_callbacks(Dispatcher
> *dispatcher)
> RED_WORKER_MESSAGE_FLUSH_SURFACES_AS
> YNC,
> handle_dev_flush_surfaces_async,
> sizeof(RedWorkerMessageFlushSurfaces
> Async),
> - DISPATCHER_FLAG_ASYNC);
> + DISPATCHER_FLAG_NONE);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_STOP,
> handle_dev_stop,
> @@ -1174,7 +1169,7 @@ static void register_callbacks(Dispatcher
> *dispatcher)
> RED_WORKER_MESSAGE_DESTROY_SURFACE_W
> AIT_ASYNC,
> handle_dev_destroy_surface_wait_asyn
> c,
> sizeof(RedWorkerMessageDestroySurfac
> eWaitAsync),
> - DISPATCHER_FLAG_ASYNC);
> + DISPATCHER_FLAG_NONE);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_RESET_MEMSLOTS,
> handle_dev_reset_memslots,
> @@ -1184,7 +1179,7 @@ static void register_callbacks(Dispatcher
> *dispatcher)
> RED_WORKER_MESSAGE_MONITORS_CONFIG_A
> SYNC,
> handle_dev_monitors_config_async,
> sizeof(RedWorkerMessageMonitorsConfi
> gAsync),
> - DISPATCHER_FLAG_ASYNC);
> + DISPATCHER_FLAG_NONE);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_DRIVER_UNLOAD,
> handle_dev_driver_unload,
More information about the Spice-devel
mailing list