>
> On Wed, 2017-09-06 at 07:47 -0400, Frediano Ziglio wrote:
> > >
> > > dispatcher_register_handler() accepts a flag value that determines
> > > whether the message type requires an ACK or async_done handler.
> > > This
> > > parameter was previously just an int. It is now a typedef'ed enum:
> > > DispatcherFlag. The enum values were accordingly renamed:
> > >
> > > DISPATCHER_* -> DISPATCHER_FLAG_*
> > >
> > > Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> >
> > I honestly much prefer the "ack" field name instead of the "flag".
> > "flag" is too generic.
> >
> > Maybe when I see ACK I think at TCP and I understand what it
> > means. Why not instead using this enumeration?
> >
> > typedef enum {
> > DISPATCHER_ACK_NONE = 0,
> > DISPATCHER_ACK_SYNC,
> > DISPATCHER_ACK_ASYNC
> > } DispatcherAck;
> >
> > and
> >
> > DispatcherAck ack;
> >
> > (or ack_type)
>
>
> Yeah, I thought about that. But I felt like the async_done thing was a
> bit different than an 'ACK'. It allows you to call a function after the
> message has been handled. But it doesn't really place any restrictions
> on what that function does. In our case, it ends up calling the
> QXLInterface::async_complete() function, which might update some
This does not make sense if you are documenting just the Dispatcher,
what the caller does with the acknowledge is not related,
this interface just give the opportunity to the user to implement
any kind of completion. But the implementation yes, does not
allow the same method for acknowledge which is quite different.
> internal state and trigger an interrupt or something. But that seemed
> like a bit of a broader concept than an 'ACK' to me. It's more like an
> async callback.
>
> I'm not sure that I'm totally happy with the 'flag' terminology either,
> but at the time I thought it was better than ACK. But I don't feel too
> strongly.
>
Maybe we could use a long "complete_action".. so
typedef enum {
DISPATCHER_COMPLETE_ACTION_NONE = 0,
DISPATCHER_COMPLETE_ACTION_SEND_ACK,
DISPATCHER_COMPLETE_ACTION_ASYNC_CALL
} DispatcherCompleteAction;
and
DispatcherCompleteAction complete_action;
I know... it's long, but "flag" is way too much generic.
> Jonathon
>
>
> >
> > Frediano
> >
> >
> > > ---
> > > server/dispatcher.c | 12 ++++-----
> > > server/dispatcher.h | 20 +++++++--------
> > > server/red-worker.c | 74
> > > ++++++++++++++++++++++++++---------------------------
> > > 3 files changed, 53 insertions(+), 53 deletions(-)
> > >
> > > diff --git a/server/dispatcher.c b/server/dispatcher.c
> > > index 6f2c4d85e..76f5eeaf4 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;
> > > - int ack;
> > > + DispatcherFlag flag;
> > > dispatcher_handle_message handler;
> > > } DispatcherMessage;
> > >
> > > @@ -303,13 +303,13 @@ 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->flag == DISPATCHER_FLAG_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) {
> > > + } else if (msg->flag == DISPATCHER_FLAG_ASYNC &&
> > > dispatcher->priv->handle_async_done) {
> > > dispatcher->priv->handle_async_done(dispatcher->priv-
> > > >opaque, type,
> > > payload);
> > > }
> > > return 1;
> > > @@ -346,7 +346,7 @@ void dispatcher_send_message(Dispatcher
> > > *dispatcher,
> > > uint32_t message_type,
> > > message_type);
> > > goto unlock;
> > > }
> > > - if (msg->ack == DISPATCHER_ACK) {
> > > + if (msg->flag == DISPATCHER_FLAG_ACK) {
> > > if (read_safe(send_fd, (uint8_t*)&ack, sizeof(ack), 1) ==
> > > -1) {
> > > spice_printerr("error: failed to read ack");
> > > } else if (ack != ACK) {
> > > @@ -369,7 +369,7 @@ void dispatcher_register_async_done_callback(
> > >
> > > void dispatcher_register_handler(Dispatcher *dispatcher, uint32_t
> > > message_type,
> > > dispatcher_handle_message
> > > handler,
> > > - size_t size, int ack)
> > > + size_t size, DispatcherFlag flag)
> > > {
> > > DispatcherMessage *msg;
> > >
> > > @@ -378,7 +378,7 @@ void dispatcher_register_handler(Dispatcher
> > > *dispatcher,
> > > uint32_t message_type,
> > > msg = &dispatcher->priv->messages[message_type];
> > > msg->handler = handler;
> > > msg->size = size;
> > > - msg->ack = ack;
> > > + msg->flag = flag;
> > > if (msg->size > dispatcher->priv->payload_size) {
> > > dispatcher->priv->payload = realloc(dispatcher->priv-
> > > >payload,
> > > msg->size);
> > > dispatcher->priv->payload_size = msg->size;
> > > diff --git a/server/dispatcher.h b/server/dispatcher.h
> > > index 97b01de9c..862fc46b9 100644
> > > --- a/server/dispatcher.h
> > > +++ b/server/dispatcher.h
> > > @@ -72,11 +72,11 @@ 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
> > > -};
> > > +typedef enum {
> > > + DISPATCHER_FLAG_NONE = 0,
> > > + DISPATCHER_FLAG_ACK,
> > > + DISPATCHER_FLAG_ASYNC
> > > +} DispatcherFlag;
> > >
> > > /*
> > > * dispatcher_register_handler
> > > @@ -84,16 +84,16 @@ enum {
> > > * @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
> > > + * @flag: One of DISPATCHER_FLAG_NONE,
> > > DISPATCHER_FLAG_ACK,
> > > DISPATCHER_FLAG_ASYNC.
> > > + * DISPATCHER_FLAG_NONE - only send the message
> > > + * DISPATCHER_FLAG_ACK - send an ack after the
> > > message
> > > + * DISPATCHER_FLAG_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.
> > > */
> > > void dispatcher_register_handler(Dispatcher *dispatcher, uint32_t
> > > message_type,
> > > dispatcher_handle_message
> > > handler, size_t
> > > size,
> > > - int ack);
> > > + DispatcherFlag flag);
> > >
> > > /*
> > > * dispatcher_register_async_done_callback
> > > diff --git a/server/red-worker.c b/server/red-worker.c
> > > index 4d8566be7..c3f2dbfa0 100644
> > > --- a/server/red-worker.c
> > > +++ b/server/red-worker.c
> > > @@ -1024,187 +1024,187 @@ static void register_callbacks(Dispatcher
> > > *dispatcher)
> > > RED_WORKER_MESSAGE_DISPLAY_CONNECT
> > > ,
> > > handle_dev_display_connect,
> > > sizeof(RedWorkerMessageDisplayConn
> > > ect),
> > > - DISPATCHER_NONE);
> > > + DISPATCHER_FLAG_NONE);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_DISPLAY_DISCONN
> > > ECT,
> > > handle_dev_display_disconnect,
> > > sizeof(RedWorkerMessageDisplayDisc
> > > onnect),
> > > - DISPATCHER_ACK);
> > > + DISPATCHER_FLAG_ACK);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_DISPLAY_MIGRATE
> > > ,
> > > handle_dev_display_migrate,
> > > sizeof(RedWorkerMessageDisplayMigr
> > > ate),
> > > - DISPATCHER_NONE);
> > > + DISPATCHER_FLAG_NONE);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_CURSOR_CONNECT,
> > > handle_dev_cursor_connect,
> > > sizeof(RedWorkerMessageCursorConne
> > > ct),
> > > - DISPATCHER_NONE);
> > > + DISPATCHER_FLAG_NONE);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_CURSOR_DISCONNE
> > > CT,
> > > handle_dev_cursor_disconnect,
> > > sizeof(RedWorkerMessageCursorDisco
> > > nnect),
> > > - DISPATCHER_ACK);
> > > + DISPATCHER_FLAG_ACK);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_CURSOR_MIGRATE,
> > > handle_dev_cursor_migrate,
> > > sizeof(RedWorkerMessageCursorMigra
> > > te),
> > > - DISPATCHER_NONE);
> > > + DISPATCHER_FLAG_NONE);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_UPDATE,
> > > handle_dev_update,
> > > sizeof(RedWorkerMessageUpdate),
> > > - DISPATCHER_ACK);
> > > + DISPATCHER_FLAG_ACK);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_UPDATE_ASYNC,
> > > handle_dev_update_async,
> > > sizeof(RedWorkerMessageUpdateAsync
> > > ),
> > > - DISPATCHER_ASYNC);
> > > + DISPATCHER_FLAG_ASYNC);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_ADD_MEMSLOT,
> > > handle_dev_add_memslot,
> > > sizeof(RedWorkerMessageAddMemslot)
> > > ,
> > > - DISPATCHER_ACK);
> > > + DISPATCHER_FLAG_ACK);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_ADD_MEMSLOT_ASY
> > > NC,
> > > handle_dev_add_memslot_async,
> > > sizeof(RedWorkerMessageAddMemslotA
> > > sync),
> > > - DISPATCHER_ASYNC);
> > > + DISPATCHER_FLAG_ASYNC);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_DEL_MEMSLOT,
> > > handle_dev_del_memslot,
> > > sizeof(RedWorkerMessageDelMemslot)
> > > ,
> > > - DISPATCHER_NONE);
> > > + DISPATCHER_FLAG_NONE);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_DESTROY_SURFACE
> > > S,
> > > handle_dev_destroy_surfaces,
> > > sizeof(RedWorkerMessageDestroySurf
> > > aces),
> > > - DISPATCHER_ACK);
> > > + DISPATCHER_FLAG_ACK);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_DESTROY_SURFACE
> > > S_ASYNC,
> > > handle_dev_destroy_surfaces_async,
> > > sizeof(RedWorkerMessageDestroySurf
> > > acesAsync),
> > > - DISPATCHER_ASYNC);
> > > + DISPATCHER_FLAG_ASYNC);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_DESTROY_PRIMARY
> > > _SURFACE,
> > > handle_dev_destroy_primary_surface
> > > ,
> > > sizeof(RedWorkerMessageDestroyPrim
> > > arySurface),
> > > - DISPATCHER_ACK);
> > > + DISPATCHER_FLAG_ACK);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_DESTROY_PRIMARY
> > > _SURFACE_ASYNC,
> > > handle_dev_destroy_primary_surface
> > > _async,
> > > sizeof(RedWorkerMessageDestroyPrim
> > > arySurfaceAsync),
> > > - DISPATCHER_ASYNC);
> > > + DISPATCHER_FLAG_ASYNC);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_CREATE_PRIMARY_
> > > SURFACE_ASYNC,
> > > handle_dev_create_primary_surface_
> > > async,
> > > sizeof(RedWorkerMessageCreatePrima
> > > rySurfaceAsync),
> > > - DISPATCHER_ASYNC);
> > > + DISPATCHER_FLAG_ASYNC);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_CREATE_PRIMARY_
> > > SURFACE,
> > > handle_dev_create_primary_surface,
> > > sizeof(RedWorkerMessageCreatePrima
> > > rySurface),
> > > - DISPATCHER_ACK);
> > > + DISPATCHER_FLAG_ACK);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_RESET_IMAGE_CAC
> > > HE,
> > > handle_dev_reset_image_cache,
> > > sizeof(RedWorkerMessageResetImageC
> > > ache),
> > > - DISPATCHER_ACK);
> > > + DISPATCHER_FLAG_ACK);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_RESET_CURSOR,
> > > handle_dev_reset_cursor,
> > > sizeof(RedWorkerMessageResetCursor
> > > ),
> > > - DISPATCHER_ACK);
> > > + DISPATCHER_FLAG_ACK);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_WAKEUP,
> > > handle_dev_wakeup,
> > > sizeof(RedWorkerMessageWakeup),
> > > - DISPATCHER_NONE);
> > > + DISPATCHER_FLAG_NONE);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_OOM,
> > > handle_dev_oom,
> > > sizeof(RedWorkerMessageOom),
> > > - DISPATCHER_NONE);
> > > + DISPATCHER_FLAG_NONE);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_START,
> > > handle_dev_start,
> > > sizeof(RedWorkerMessageStart),
> > > - DISPATCHER_NONE);
> > > + DISPATCHER_FLAG_NONE);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_FLUSH_SURFACES_
> > > ASYNC,
> > > handle_dev_flush_surfaces_async,
> > > sizeof(RedWorkerMessageFlushSurfac
> > > esAsync),
> > > - DISPATCHER_ASYNC);
> > > + DISPATCHER_FLAG_ASYNC);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_STOP,
> > > handle_dev_stop,
> > > sizeof(RedWorkerMessageStop),
> > > - DISPATCHER_ACK);
> > > + DISPATCHER_FLAG_ACK);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_LOADVM_COMMANDS
> > > ,
> > > handle_dev_loadvm_commands,
> > > sizeof(RedWorkerMessageLoadvmComma
> > > nds),
> > > - DISPATCHER_ACK);
> > > + DISPATCHER_FLAG_ACK);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_SET_COMPRESSION
> > > ,
> > > handle_dev_set_compression,
> > > sizeof(RedWorkerMessageSetCompress
> > > ion),
> > > - DISPATCHER_NONE);
> > > + DISPATCHER_FLAG_NONE);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_SET_STREAMING_V
> > > IDEO,
> > > handle_dev_set_streaming_video,
> > > sizeof(RedWorkerMessageSetStreamin
> > > gVideo),
> > > - DISPATCHER_NONE);
> > > + DISPATCHER_FLAG_NONE);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_SET_VIDEO_CODEC
> > > S,
> > > handle_dev_set_video_codecs,
> > > sizeof(RedWorkerMessageSetVideoCod
> > > ecs),
> > > - DISPATCHER_NONE);
> > > + DISPATCHER_FLAG_NONE);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_SET_MOUSE_MODE,
> > > handle_dev_set_mouse_mode,
> > > sizeof(RedWorkerMessageSetMouseMod
> > > e),
> > > - DISPATCHER_NONE);
> > > + DISPATCHER_FLAG_NONE);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_DESTROY_SURFACE
> > > _WAIT,
> > > handle_dev_destroy_surface_wait,
> > > sizeof(RedWorkerMessageDestroySurf
> > > aceWait),
> > > - DISPATCHER_ACK);
> > > + DISPATCHER_FLAG_ACK);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_DESTROY_SURFACE
> > > _WAIT_ASYNC,
> > > handle_dev_destroy_surface_wait_as
> > > ync,
> > > sizeof(RedWorkerMessageDestroySurf
> > > aceWaitAsync),
> > > - DISPATCHER_ASYNC);
> > > + DISPATCHER_FLAG_ASYNC);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_RESET_MEMSLOTS,
> > > handle_dev_reset_memslots,
> > > sizeof(RedWorkerMessageResetMemslo
> > > ts),
> > > - DISPATCHER_NONE);
> > > + DISPATCHER_FLAG_NONE);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_MONITORS_CONFIG
> > > _ASYNC,
> > > handle_dev_monitors_config_async,
> > > sizeof(RedWorkerMessageMonitorsCon
> > > figAsync),
> > > - DISPATCHER_ASYNC);
> > > + DISPATCHER_FLAG_ASYNC);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_DRIVER_UNLOAD,
> > > handle_dev_driver_unload,
> > > sizeof(RedWorkerMessageDriverUnloa
> > > d),
> > > - DISPATCHER_NONE);
> > > + DISPATCHER_FLAG_NONE);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_GL_SCANOUT,
> > > handle_dev_gl_scanout,
> > > sizeof(RedWorkerMessageGlScanout),
> > > - DISPATCHER_NONE);
> > > + DISPATCHER_FLAG_NONE);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_GL_DRAW_ASYNC,
> > > handle_dev_gl_draw_async,
> > > sizeof(RedWorkerMessageGlDraw),
> > > - DISPATCHER_NONE);
> > > + DISPATCHER_FLAG_NONE);
> > > dispatcher_register_handler(dispatcher,
> > > RED_WORKER_MESSAGE_CLOSE_WORKER,
> > > handle_dev_close,
> > > sizeof(RedWorkerMessageClose),
> > > - DISPATCHER_NONE);
> > > + DISPATCHER_FLAG_NONE);
> > > }
> > >
> > >
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel