On Wed, Nov 02, 2011 at 06:00:46PM +0200, Yonit Halperin wrote:
> 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 :)
Should have added it in the CC.
>
> >---
> > 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)
>
I'll do it. I think I'll either expose Dispatcher or add a
"red_dispatcher_register_callback" that hides it.
> > 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.
I thought about it, but the whole point of the table is to avoid a
switch, seems kind of counterproductive. Another option would have been
to register a second callback, or to add a second table. Too many ways
to solve a little problem. I think the second callback registration is
the least magical / complex. So Dispatcher will just:
if handler:
call handler
if ack:
send ack
else:
if async_complete:
call async_complete
And I'll add dispatcher_register_async_complete
>
>
> Thanks,
> Yonit.
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
