[Spice-devel] [RFC 3/3] server/red_worker: reuse dispatcher

[Yonit Halperin]

On 11/01/2011 10:10 AM, Alon Levy wrote:
> This patch reuses Dispatcher in RedDispatcher. It adds two helpers
> to red_worker to keep RedWorker opaque to the outside. The dispatcher is
> abused in three places that use the underlying socket directly:
>   once sending a READY after red_init completes
>   once for each channel creation, replying with the RedChannel instance
>    for cursor and display.
>
exploit this opportunity to rename red_dispatcher? ->worker_dispatcher? 
display_dispatcher?
> Code is being added, not removed.
>
> Readability?
wrote a script to review the patch :)

> ---
>   server/red_dispatcher.c |  585 +++++++++++++++++++++++++++------------
>   server/red_dispatcher.h |  182 ++++++++++++
>   server/red_worker.c     |  716 +++++++++++++++++++++++++----------------------
>   server/red_worker.h     |    2 +
>   4 files changed, 979 insertions(+), 506 deletions(-)
>
> diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
> index 7a0033f..4680b50 100644
> --- a/server/red_dispatcher.c
> +++ b/server/red_dispatcher.c
> @@ -37,6 +37,7 @@
>   #include "reds_gl_canvas.h"
>   #endif // USE_OPENGL
>   #include "reds.h"
> +#include "dispatcher.h"
>   #include "red_dispatcher.h"
>   #include "red_parse_qxl.h"
>
> @@ -53,7 +54,7 @@ struct AsyncCommand {
>   struct RedDispatcher {
>       QXLWorker base;
>       QXLInstance *qxl;
> -    int channel;
> +    Dispatcher dispatcher;
>       pthread_t worker_thread;
>       uint32_t pending;
>       int primary_active;
> @@ -83,24 +84,196 @@ extern spice_wan_compression_t zlib_glz_state;
>
>   static RedDispatcher *dispatchers = NULL;
>
> +static void register_callbacks(RedDispatcher *dispatcher)
> +{
> +    dispatcher_register_handler(&dispatcher->dispatcher,
> +                                RED_WORKER_MESSAGE_DISPLAY_CONNECT,
> +                                handle_dev_display_connect,
> +                                sizeof(RedWorkerMessageDisplayConnect),
> +                                0);
> +    dispatcher_register_handler(&dispatcher->dispatcher,
> +                                RED_WORKER_MESSAGE_DISPLAY_DISCONNECT,
> +                                handle_dev_display_disconnect,
> +                                sizeof(RedWorkerMessageDisplayDisconnect),
> +                                1);
> +    dispatcher_register_handler(&dispatcher->dispatcher,
> +                                RED_WORKER_MESSAGE_DISPLAY_MIGRATE,
> +                                handle_dev_display_migrate,
> +                                sizeof(RedWorkerMessageDisplayMigrate),
> +                                0);
<snip>
.
.
.
> +    dispatcher_register_handler(&dispatcher->dispatcher,
> +                                RED_WORKER_MESSAGE_RESET_MEMSLOTS,
> +                                handle_dev_reset_memslots,
> +                                sizeof(RedWorkerMessageResetMemslots),
> +                                0);
> +}
> +
What do you think about moving the registering the cbs to red_worker? 
Then you will not need to add all the cbs prototype to red_dispatcher.h 
(but you will need to expose Dispatcher to red_worker)

>   static void red_dispatcher_set_display_peer(RedChannel *channel, RedClient *client,
>                                               RedsStream *stream, int migration,
>                                               int num_common_caps, uint32_t *common_caps, int num_caps,
>                                               uint32_t *caps)
>   {
> +    RedWorkerMessageDisplayConnect payload;
>       RedDispatcher *dispatcher;
>
>       red_printf("");
>       dispatcher = (RedDispatcher *)channel->data;
> -    RedWorkerMessage message = RED_WORKER_MESSAGE_DISPLAY_CONNECT;
> -    write_message(dispatcher->channel,&message);
> -    send_data(dispatcher->channel,&client, sizeof(RedClient *));
> -    send_data(dispatcher->channel,&stream, sizeof(RedsStream *));
> -    send_data(dispatcher->channel,&migration, sizeof(int));
> +    payload.client = client;
> +    payload.stream = stream;
> +    payload.migration = migration;
> +    dispatcher_send_message(&dispatcher->dispatcher,
> +                            RED_WORKER_MESSAGE_DISPLAY_CONNECT,
> +&payload);
>   }
>
<snip>

> diff --git a/server/red_worker.c b/server/red_worker.c
> index 00efc05..6e5a238 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -866,8 +866,8 @@ typedef struct RedWorker {
>       CursorChannel *cursor_channel;
>       QXLInstance *qxl;
>       RedDispatcher *dispatcher;
> -    int id;
>       int channel;
> +    int id;
>       int running;
>       uint32_t *pending;
>       int epoll;
> @@ -10102,21 +10102,19 @@ static void surface_dirty_region_to_rects(RedSurface *surface,
>       free(dirty_rects);
>   }
>
> -static inline void handle_dev_update_async(RedWorker *worker)
> +void handle_dev_update_async(void *opaque, void *payload)
>   {
> -    QXLRect qxl_rect;
> +    RedWorker *worker = opaque;
> +    RedWorkerMessageUpdateAsync *msg = payload;
>       SpiceRect rect;
> -    uint32_t surface_id;
> -    uint32_t clear_dirty_region;
>       QXLRect *qxl_dirty_rects;
>       uint32_t num_dirty_rects;
>       RedSurface *surface;
> +    uint32_t surface_id = msg->surface_id;
> +    QXLRect qxl_area = msg->qxl_area;
> +    uint32_t clear_dirty_region = msg->clear_dirty_region;
>
> -    receive_data(worker->channel,&surface_id, sizeof(uint32_t));
> -    receive_data(worker->channel,&qxl_rect, sizeof(QXLRect));
> -    receive_data(worker->channel,&clear_dirty_region, sizeof(uint32_t));
> -
> -    red_get_rect_ptr(&rect,&qxl_rect);
> +    red_get_rect_ptr(&rect,&qxl_area);
>       flush_display_commands(worker);
>
>       ASSERT(worker->running);
> @@ -10124,12 +10122,12 @@ static inline void handle_dev_update_async(RedWorker *worker)
>       validate_surface(worker, surface_id);
>       red_update_area(worker,&rect, surface_id);
>       if (!worker->qxl->st->qif->update_area_complete) {
> -        return;
> +        goto complete;
>       }
>       surface =&worker->surfaces[surface_id];
>       num_dirty_rects = pixman_region32_n_rects(&surface->draw_dirty_region);
>       if (num_dirty_rects == 0) {
> -        return;
> +        goto complete;
>       }
>       qxl_dirty_rects = spice_new0(QXLRect, num_dirty_rects);
>       surface_dirty_region_to_rects(surface, qxl_dirty_rects, num_dirty_rects,
> @@ -10137,26 +10135,25 @@ static inline void handle_dev_update_async(RedWorker *worker)
>       worker->qxl->st->qif->update_area_complete(worker->qxl, surface_id,
>                                             qxl_dirty_rects, num_dirty_rects);
>       free(qxl_dirty_rects);
> +
> +complete:
> +    red_dispatcher_async_complete(worker->dispatcher, msg->cmd);

You can register only one callback to all the async messages, and then
handle each one differently, but call red_dispatcher_async_complete for 
all of them.


Thanks,
Yonit.