>
> This callback was only executed for message types that were registered
> with DISPATCHER_ASYNC ack type. However, the async_done handler was
> called immediately after the message-specific handler and was called in
> the same thread, so the async_done stuff can just as easily be done from
> within the message-specific handler. This allows to simplify the
> dispatcher_register_handler() method to simply require a boolean
> argument for whether the message type requires an ACK or not.
> ---
>
> I notice Frediano kept the enum type, but in this patch I changed it to
> a simple boolean. Is there any reason to keep the enum?
>
Main reason was faster to not change :-)
Somebody could complain that reading the code a "DISPATCHER_ACK" make
understand the purpose more then a "true" but I'm fine with the
true. Note that on main-dispatcher the register is called with
a "0 /* no ack */" parameter. I would surely change (follow up).
> server/dispatcher.c | 19 +++-------
> server/dispatcher.h | 25 ++------------
> server/red-worker.c | 99
> +++++++++++++++++++++++++----------------------------
> 3 files changed, 53 insertions(+), 90 deletions(-)
>
> diff --git a/server/dispatcher.c b/server/dispatcher.c
> index 6f2c4d85e..97ebe32a6 100644
> --- a/server/dispatcher.c
> +++ b/server/dispatcher.c
> @@ -40,7 +40,7 @@ static void setup_dummy_signal_handler(void);
>
> typedef struct DispatcherMessage {
> size_t size;
OT and paranoid. If we use uint32_t we reduce the structure to
16 bytes (64 bit) which lead to use indexing like [base+16*index]
on Intel so more optimizations.
> - int ack;
> + bool ack;
> dispatcher_handle_message handler;
> } DispatcherMessage;
>
> @@ -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;
> };
>
> @@ -303,14 +302,12 @@ static int dispatcher_handle_single_read(Dispatcher
> *dispatcher)
> } else {
> spice_printerr("error: no handler for message type %d", type);
> }
> - if (msg->ack == DISPATCHER_ACK) {
> + if (msg->ack) {
> if (write_safe(dispatcher->priv->recv_fd,
> (uint8_t*)&ack, sizeof(ack)) == -1) {
> spice_printerr("error writing ack for message %d", type);
> /* TODO: close socketpair? */
> }
> - } else if (msg->ack == DISPATCHER_ASYNC &&
> dispatcher->priv->handle_async_done) {
> - dispatcher->priv->handle_async_done(dispatcher->priv->opaque, type,
> payload);
> }
> return 1;
> }
> @@ -346,7 +343,7 @@ void dispatcher_send_message(Dispatcher *dispatcher,
> uint32_t message_type,
> message_type);
> goto unlock;
> }
> - if (msg->ack == DISPATCHER_ACK) {
> + if (msg->ack) {
> if (read_safe(send_fd, (uint8_t*)&ack, sizeof(ack), 1) == -1) {
> spice_printerr("error: failed to read ack");
> } else if (ack != ACK) {
> @@ -359,17 +356,9 @@ 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, int ack)
> + size_t size, bool ack)
> {
> DispatcherMessage *msg;
>
> diff --git a/server/dispatcher.h b/server/dispatcher.h
> index 97b01de9c..eb93c1358 100644
> --- a/server/dispatcher.h
> +++ b/server/dispatcher.h
> @@ -72,38 +72,17 @@ typedef void (*dispatcher_handle_async_done)(void
> *opaque,
> void dispatcher_send_message(Dispatcher *dispatcher, uint32_t message_type,
> void *payload);
>
> -enum {
> - DISPATCHER_NONE = 0,
> - DISPATCHER_ACK,
> - DISPATCHER_ASYNC
> -};
> -
> /*
> * dispatcher_register_handler
> * @dispatcher: dispatcher
> * @messsage_type: message type
> * @handler: message handler
> * @size: message size. Each type has a fixed associated size.
> - * @ack: One of DISPATCHER_NONE, DISPATCHER_ACK,
> DISPATCHER_ASYNC.
> - * DISPATCHER_NONE - only send the message
> - * DISPATCHER_ACK - send an ack after the message
> - * DISPATCHER_ASYNC - call send an ack. This is per message
> type - you can't send the
> - * same message type with and without. Register two
> different
> - * messages if that is what you want.
> + * @ack: whether the dispatcher should send an ACK to the sender
> */
> void dispatcher_register_handler(Dispatcher *dispatcher, uint32_t
> message_type,
> dispatcher_handle_message handler, size_t
> size,
> - int ack);
> -
> -/*
> - * dispatcher_register_async_done_callback
> - * @dispatcher: dispatcher
> - * @handler: callback on the receiver side called *after* the
> - * message callback in case ack == DISPATCHER_ASYNC.
> - */
> -void dispatcher_register_async_done_callback(
> - Dispatcher *dispatcher,
> - dispatcher_handle_async_done handler);
> + bool ack);
>
> /*
> * Hack to allow red_record to see the message being sent so it can record
I think the dispatcher_handle_async_done handler declaration in
the header can be changed too.
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 675c232e7..36bcef796 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)
> @@ -581,14 +582,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)
> @@ -688,15 +692,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)
> @@ -705,6 +712,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)
> @@ -801,6 +809,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);
> }
>
I think you get handle_dev_monitors_config_async wrong, there are
a lot of returns in the function missing the complete call which could
lead in caller (Qemu) leaks.
> /* TODO: special, perhaps use another dispatcher? */
> @@ -927,6 +936,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)
> @@ -999,17 +1009,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;
> @@ -1019,196 +1018,192 @@ 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_done);
> -
> /* TODO: register cursor & display specific msg in respective channel
> files */
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_DISPLAY_CONNECT,
> handle_dev_display_connect,
> sizeof(RedWorkerMessageDisplayConnect),
> - DISPATCHER_NONE);
> + false);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_DISPLAY_DISCONNECT,
> handle_dev_display_disconnect,
> sizeof(RedWorkerMessageDisplayDisconnect),
> - DISPATCHER_ACK);
> + true);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_DISPLAY_MIGRATE,
> handle_dev_display_migrate,
> sizeof(RedWorkerMessageDisplayMigrate),
> - DISPATCHER_NONE);
> + false);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_CURSOR_CONNECT,
> handle_dev_cursor_connect,
> sizeof(RedWorkerMessageCursorConnect),
> - DISPATCHER_NONE);
> + false);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_CURSOR_DISCONNECT,
> handle_dev_cursor_disconnect,
> sizeof(RedWorkerMessageCursorDisconnect),
> - DISPATCHER_ACK);
> + true);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_CURSOR_MIGRATE,
> handle_dev_cursor_migrate,
> sizeof(RedWorkerMessageCursorMigrate),
> - DISPATCHER_NONE);
> + false);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_UPDATE,
> handle_dev_update,
> sizeof(RedWorkerMessageUpdate),
> - DISPATCHER_ACK);
> + true);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_UPDATE_ASYNC,
> handle_dev_update_async,
> sizeof(RedWorkerMessageUpdateAsync),
> - DISPATCHER_ASYNC);
> + false);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_ADD_MEMSLOT,
> handle_dev_add_memslot,
> sizeof(RedWorkerMessageAddMemslot),
> - DISPATCHER_ACK);
> + true);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_ADD_MEMSLOT_ASYNC,
> handle_dev_add_memslot_async,
> sizeof(RedWorkerMessageAddMemslotAsync),
> - DISPATCHER_ASYNC);
> + false);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_DEL_MEMSLOT,
> handle_dev_del_memslot,
> sizeof(RedWorkerMessageDelMemslot),
> - DISPATCHER_NONE);
> + false);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_DESTROY_SURFACES,
> handle_dev_destroy_surfaces,
> sizeof(RedWorkerMessageDestroySurfaces),
> - DISPATCHER_ACK);
> + true);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_DESTROY_SURFACES_ASYNC,
> handle_dev_destroy_surfaces_async,
> sizeof(RedWorkerMessageDestroySurfacesAsync),
> - DISPATCHER_ASYNC);
> + false);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_DESTROY_PRIMARY_SURFACE,
> handle_dev_destroy_primary_surface,
>
> sizeof(RedWorkerMessageDestroyPrimarySurface),
> - DISPATCHER_ACK);
> + true);
> dispatcher_register_handler(dispatcher,
>
> RED_WORKER_MESSAGE_DESTROY_PRIMARY_SURFACE_ASYNC,
> handle_dev_destroy_primary_surface_async,
>
> sizeof(RedWorkerMessageDestroyPrimarySurfaceAsync),
> - DISPATCHER_ASYNC);
> + false);
> dispatcher_register_handler(dispatcher,
>
> RED_WORKER_MESSAGE_CREATE_PRIMARY_SURFACE_ASYNC,
> handle_dev_create_primary_surface_async,
>
> sizeof(RedWorkerMessageCreatePrimarySurfaceAsync),
> - DISPATCHER_ASYNC);
> + false);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_CREATE_PRIMARY_SURFACE,
> handle_dev_create_primary_surface,
> sizeof(RedWorkerMessageCreatePrimarySurface),
> - DISPATCHER_ACK);
> + true);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_RESET_IMAGE_CACHE,
> handle_dev_reset_image_cache,
> sizeof(RedWorkerMessageResetImageCache),
> - DISPATCHER_ACK);
> + true);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_RESET_CURSOR,
> handle_dev_reset_cursor,
> sizeof(RedWorkerMessageResetCursor),
> - DISPATCHER_ACK);
> + true);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_WAKEUP,
> handle_dev_wakeup,
> sizeof(RedWorkerMessageWakeup),
> - DISPATCHER_NONE);
> + false);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_OOM,
> handle_dev_oom,
> sizeof(RedWorkerMessageOom),
> - DISPATCHER_NONE);
> + false);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_START,
> handle_dev_start,
> sizeof(RedWorkerMessageStart),
> - DISPATCHER_NONE);
> + false);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_FLUSH_SURFACES_ASYNC,
> handle_dev_flush_surfaces_async,
> sizeof(RedWorkerMessageFlushSurfacesAsync),
> - DISPATCHER_ASYNC);
> + false);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_STOP,
> handle_dev_stop,
> sizeof(RedWorkerMessageStop),
> - DISPATCHER_ACK);
> + true);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_LOADVM_COMMANDS,
> handle_dev_loadvm_commands,
> sizeof(RedWorkerMessageLoadvmCommands),
> - DISPATCHER_ACK);
> + true);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_SET_COMPRESSION,
> handle_dev_set_compression,
> sizeof(RedWorkerMessageSetCompression),
> - DISPATCHER_NONE);
> + false);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_SET_STREAMING_VIDEO,
> handle_dev_set_streaming_video,
> sizeof(RedWorkerMessageSetStreamingVideo),
> - DISPATCHER_NONE);
> + false);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_SET_VIDEO_CODECS,
> handle_dev_set_video_codecs,
> sizeof(RedWorkerMessageSetVideoCodecs),
> - DISPATCHER_NONE);
> + false);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_SET_MOUSE_MODE,
> handle_dev_set_mouse_mode,
> sizeof(RedWorkerMessageSetMouseMode),
> - DISPATCHER_NONE);
> + false);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_DESTROY_SURFACE_WAIT,
> handle_dev_destroy_surface_wait,
> sizeof(RedWorkerMessageDestroySurfaceWait),
> - DISPATCHER_ACK);
> + true);
> dispatcher_register_handler(dispatcher,
>
> RED_WORKER_MESSAGE_DESTROY_SURFACE_WAIT_ASYNC,
> handle_dev_destroy_surface_wait_async,
>
> sizeof(RedWorkerMessageDestroySurfaceWaitAsync),
> - DISPATCHER_ASYNC);
> + false);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_RESET_MEMSLOTS,
> handle_dev_reset_memslots,
> sizeof(RedWorkerMessageResetMemslots),
> - DISPATCHER_NONE);
> + false);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_MONITORS_CONFIG_ASYNC,
> handle_dev_monitors_config_async,
> sizeof(RedWorkerMessageMonitorsConfigAsync),
> - DISPATCHER_ASYNC);
> + false);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_DRIVER_UNLOAD,
> handle_dev_driver_unload,
> sizeof(RedWorkerMessageDriverUnload),
> - DISPATCHER_NONE);
> + false);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_GL_SCANOUT,
> handle_dev_gl_scanout,
> sizeof(RedWorkerMessageGlScanout),
> - DISPATCHER_NONE);
> + false);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_GL_DRAW_ASYNC,
> handle_dev_gl_draw_async,
> sizeof(RedWorkerMessageGlDraw),
> - DISPATCHER_NONE);
> + false);
> dispatcher_register_handler(dispatcher,
> RED_WORKER_MESSAGE_CLOSE_WORKER,
> handle_dev_close,
> sizeof(RedWorkerMessageClose),
> - DISPATCHER_NONE);
> + false);
> }
>
>
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
