[Spice-devel,v6,2/9] server: api: add spice_qxl_* calls based on QXLWorker contents

Submitted by Alon Levy on July 20, 2011, 3:19 p.m.

Details

Message ID 1311149999-13216-3-git-send-email-alevy@redhat.com
State New, archived
Headers show

Not browsing as part of any series.

Commit Message

Alon Levy July 20, 2011, 3:19 p.m.
For each callback in QXLWorker, for example QXLWorker::update_area, add
a direct call named spice_qxl_update_area.

This will (a) remove the pointless indirection and (b) make shared
library versioning alot easier as we'll get new linker symbols which
we can tag with the version they appeared in the shared library.
---
 server/red_dispatcher.c  |  223 +++++++++++++++++++++++++++++++++++++++-------
 server/spice-server.syms |   15 +++
 server/spice.h           |   19 ++++
 3 files changed, 225 insertions(+), 32 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
index 56446ab..aa1990e 100644
--- a/server/red_dispatcher.c
+++ b/server/red_dispatcher.c
@@ -209,11 +209,10 @@  static void update_client_mouse_allowed(void)
     }
 }
 
-static void qxl_worker_update_area(QXLWorker *qxl_worker, uint32_t surface_id,
+static void red_dispatcher_update_area(RedDispatcher *dispatcher, uint32_t surface_id,
                                    QXLRect *qxl_area, QXLRect *qxl_dirty_rects,
                                    uint32_t num_dirty_rects, uint32_t clear_dirty_region)
 {
-    RedDispatcher *dispatcher = (RedDispatcher *)qxl_worker;
     RedWorkerMessage message = RED_WORKER_MESSAGE_UPDATE;
     SpiceRect *dirty_rects = spice_new0(SpiceRect, num_dirty_rects);
     SpiceRect *area = spice_new0(SpiceRect, 1);
@@ -241,9 +240,16 @@  static void qxl_worker_update_area(QXLWorker *qxl_worker, uint32_t surface_id,
     free(area);
 }
 
-static void qxl_worker_add_memslot(QXLWorker *qxl_worker, QXLDevMemSlot *mem_slot)
+static void qxl_worker_update_area(QXLWorker *qxl_worker, uint32_t surface_id,
+                                   QXLRect *qxl_area, QXLRect *qxl_dirty_rects,
+                                   uint32_t num_dirty_rects, uint32_t clear_dirty_region)
+{
+    red_dispatcher_update_area((RedDispatcher*)qxl_worker, surface_id, qxl_area,
+                               qxl_dirty_rects, num_dirty_rects, clear_dirty_region);
+}
+
+static void red_dispatcher_add_memslot(RedDispatcher *dispatcher, QXLDevMemSlot *mem_slot)
 {
-    RedDispatcher *dispatcher = (RedDispatcher *)qxl_worker;
     RedWorkerMessage message = RED_WORKER_MESSAGE_ADD_MEMSLOT;
 
     write_message(dispatcher->channel, &message);
@@ -252,9 +258,13 @@  static void qxl_worker_add_memslot(QXLWorker *qxl_worker, QXLDevMemSlot *mem_slo
     ASSERT(message == RED_WORKER_MESSAGE_READY);
 }
 
-static void qxl_worker_del_memslot(QXLWorker *qxl_worker, uint32_t slot_group_id, uint32_t slot_id)
+static void qxl_worker_add_memslot(QXLWorker *qxl_worker, QXLDevMemSlot *mem_slot)
+{
+    red_dispatcher_add_memslot((RedDispatcher*)qxl_worker, mem_slot);
+}
+
+static void red_dispatcher_del_memslot(RedDispatcher *dispatcher, uint32_t slot_group_id, uint32_t slot_id)
 {
-    RedDispatcher *dispatcher = (RedDispatcher *)qxl_worker;
     RedWorkerMessage message = RED_WORKER_MESSAGE_DEL_MEMSLOT;
 
     write_message(dispatcher->channel, &message);
@@ -262,9 +272,13 @@  static void qxl_worker_del_memslot(QXLWorker *qxl_worker, uint32_t slot_group_id
     send_data(dispatcher->channel, &slot_id, sizeof(uint32_t));
 }
 
-static void qxl_worker_destroy_surfaces(QXLWorker *qxl_worker)
+static void qxl_worker_del_memslot(QXLWorker *qxl_worker, uint32_t slot_group_id, uint32_t slot_id)
+{
+    red_dispatcher_del_memslot((RedDispatcher*)qxl_worker, slot_group_id, slot_id);
+}
+
+static void red_dispatcher_destroy_surfaces(RedDispatcher *dispatcher)
 {
-    RedDispatcher *dispatcher = (RedDispatcher *)qxl_worker;
     RedWorkerMessage message = RED_WORKER_MESSAGE_DESTROY_SURFACES;
 
     write_message(dispatcher->channel, &message);
@@ -272,9 +286,13 @@  static void qxl_worker_destroy_surfaces(QXLWorker *qxl_worker)
     ASSERT(message == RED_WORKER_MESSAGE_READY);
 }
 
-static void qxl_worker_destroy_primary(QXLWorker *qxl_worker, uint32_t surface_id)
+static void qxl_worker_destroy_surfaces(QXLWorker *qxl_worker)
+{
+    red_dispatcher_destroy_surfaces((RedDispatcher*)qxl_worker);
+}
+
+static void red_dispatcher_destroy_primary(RedDispatcher *dispatcher, uint32_t surface_id)
 {
-    RedDispatcher *dispatcher = (RedDispatcher *)qxl_worker;
     RedWorkerMessage message = RED_WORKER_MESSAGE_DESTROY_PRIMARY_SURFACE;
 
     write_message(dispatcher->channel, &message);
@@ -290,10 +308,14 @@  static void qxl_worker_destroy_primary(QXLWorker *qxl_worker, uint32_t surface_i
     update_client_mouse_allowed();
 }
 
-static void qxl_worker_create_primary(QXLWorker *qxl_worker, uint32_t surface_id,
+static void qxl_worker_destroy_primary(QXLWorker *qxl_worker, uint32_t surface_id)
+{
+    red_dispatcher_destroy_primary((RedDispatcher*)qxl_worker, surface_id);
+}
+
+static void red_dispatcher_create_primary(RedDispatcher *dispatcher, uint32_t surface_id,
                                       QXLDevSurfaceCreate *surface)
 {
-    RedDispatcher *dispatcher = (RedDispatcher *)qxl_worker;
     RedWorkerMessage message = RED_WORKER_MESSAGE_CREATE_PRIMARY_SURFACE;
 
     dispatcher->x_res = surface->width;
@@ -310,9 +332,14 @@  static void qxl_worker_create_primary(QXLWorker *qxl_worker, uint32_t surface_id
     update_client_mouse_allowed();
 }
 
-static void qxl_worker_reset_image_cache(QXLWorker *qxl_worker)
+static void qxl_worker_create_primary(QXLWorker *qxl_worker, uint32_t surface_id,
+                                      QXLDevSurfaceCreate *surface)
+{
+    red_dispatcher_create_primary((RedDispatcher*)qxl_worker, surface_id, surface);
+}
+
+static void red_dispatcher_reset_image_cache(RedDispatcher *dispatcher)
 {
-    RedDispatcher *dispatcher = (RedDispatcher *)qxl_worker;
     RedWorkerMessage message = RED_WORKER_MESSAGE_RESET_IMAGE_CACHE;
 
     write_message(dispatcher->channel, &message);
@@ -320,9 +347,13 @@  static void qxl_worker_reset_image_cache(QXLWorker *qxl_worker)
     ASSERT(message == RED_WORKER_MESSAGE_READY);
 }
 
-static void qxl_worker_reset_cursor(QXLWorker *qxl_worker)
+static void qxl_worker_reset_image_cache(QXLWorker *qxl_worker)
+{
+    red_dispatcher_reset_image_cache((RedDispatcher*)qxl_worker);
+}
+
+static void red_dispatcher_reset_cursor(RedDispatcher *dispatcher)
 {
-    RedDispatcher *dispatcher = (RedDispatcher *)qxl_worker;
     RedWorkerMessage message = RED_WORKER_MESSAGE_RESET_CURSOR;
 
     write_message(dispatcher->channel, &message);
@@ -330,9 +361,13 @@  static void qxl_worker_reset_cursor(QXLWorker *qxl_worker)
     ASSERT(message == RED_WORKER_MESSAGE_READY);
 }
 
-static void qxl_worker_destroy_surface_wait(QXLWorker *qxl_worker, uint32_t surface_id)
+static void qxl_worker_reset_cursor(QXLWorker *qxl_worker)
+{
+    red_dispatcher_reset_cursor((RedDispatcher*)qxl_worker);
+}
+
+static void red_dispatcher_destroy_surface_wait(RedDispatcher *dispatcher, uint32_t surface_id)
 {
-    RedDispatcher *dispatcher = (RedDispatcher *)qxl_worker;
     RedWorkerMessage message = RED_WORKER_MESSAGE_DESTROY_SURFACE_WAIT;
 
     write_message(dispatcher->channel, &message);
@@ -341,18 +376,25 @@  static void qxl_worker_destroy_surface_wait(QXLWorker *qxl_worker, uint32_t surf
     ASSERT(message == RED_WORKER_MESSAGE_READY);
 }
 
-static void qxl_worker_reset_memslots(QXLWorker *qxl_worker)
+static void qxl_worker_destroy_surface_wait(QXLWorker *qxl_worker, uint32_t surface_id)
+{
+    red_dispatcher_destroy_surface_wait((RedDispatcher*)qxl_worker, surface_id);
+}
+
+static void red_dispatcher_reset_memslots(RedDispatcher *dispatcher)
 {
-    RedDispatcher *dispatcher = (RedDispatcher *)qxl_worker;
     RedWorkerMessage message = RED_WORKER_MESSAGE_RESET_MEMSLOTS;
 
     write_message(dispatcher->channel, &message);
 }
 
-static void qxl_worker_wakeup(QXLWorker *qxl_worker)
+static void qxl_worker_reset_memslots(QXLWorker *qxl_worker)
 {
-    RedDispatcher *dispatcher = (RedDispatcher *)qxl_worker;
+    red_dispatcher_reset_memslots((RedDispatcher*)qxl_worker);
+}
 
+static void red_dispatcher_wakeup(RedDispatcher *dispatcher)
+{
     if (!test_bit(RED_WORKER_PENDING_WAKEUP, dispatcher->pending)) {
         RedWorkerMessage message = RED_WORKER_MESSAGE_WAKEUP;
         set_bit(RED_WORKER_PENDING_WAKEUP, &dispatcher->pending);
@@ -360,9 +402,13 @@  static void qxl_worker_wakeup(QXLWorker *qxl_worker)
     }
 }
 
-static void qxl_worker_oom(QXLWorker *qxl_worker)
+static void qxl_worker_wakeup(QXLWorker *qxl_worker)
+{
+    red_dispatcher_wakeup((RedDispatcher*)qxl_worker);
+}
+
+static void red_dispatcher_oom(RedDispatcher *dispatcher)
 {
-    RedDispatcher *dispatcher = (RedDispatcher *)qxl_worker;
     if (!test_bit(RED_WORKER_PENDING_OOM, dispatcher->pending)) {
         RedWorkerMessage message = RED_WORKER_MESSAGE_OOM;
         set_bit(RED_WORKER_PENDING_OOM, &dispatcher->pending);
@@ -370,17 +416,25 @@  static void qxl_worker_oom(QXLWorker *qxl_worker)
     }
 }
 
-static void qxl_worker_start(QXLWorker *qxl_worker)
+static void qxl_worker_oom(QXLWorker *qxl_worker)
+{
+    red_dispatcher_oom((RedDispatcher*)qxl_worker);
+}
+
+static void red_dispatcher_start(RedDispatcher *dispatcher)
 {
-    RedDispatcher *dispatcher = (RedDispatcher *)qxl_worker;
     RedWorkerMessage message = RED_WORKER_MESSAGE_START;
 
     write_message(dispatcher->channel, &message);
 }
 
-static void qxl_worker_stop(QXLWorker *qxl_worker)
+static void qxl_worker_start(QXLWorker *qxl_worker)
+{
+    red_dispatcher_start((RedDispatcher*)qxl_worker);
+}
+
+static void red_dispatcher_stop(RedDispatcher *dispatcher)
 {
-    RedDispatcher *dispatcher = (RedDispatcher *)qxl_worker;
     RedWorkerMessage message = RED_WORKER_MESSAGE_STOP;
 
     write_message(dispatcher->channel, &message);
@@ -388,11 +442,15 @@  static void qxl_worker_stop(QXLWorker *qxl_worker)
     ASSERT(message == RED_WORKER_MESSAGE_READY);
 }
 
-static void qxl_worker_loadvm_commands(QXLWorker *qxl_worker,
-                                       struct QXLCommandExt *ext,
-                                       uint32_t count)
+static void qxl_worker_stop(QXLWorker *qxl_worker)
+{
+    red_dispatcher_stop((RedDispatcher*)qxl_worker);
+}
+
+static void red_dispatcher_loadvm_commands(RedDispatcher *dispatcher,
+                                           struct QXLCommandExt *ext,
+                                           uint32_t count)
 {
-    RedDispatcher *dispatcher = (RedDispatcher *)qxl_worker;
     RedWorkerMessage message = RED_WORKER_MESSAGE_LOADVM_COMMANDS;
 
     red_printf("");
@@ -403,6 +461,13 @@  static void qxl_worker_loadvm_commands(QXLWorker *qxl_worker,
     ASSERT(message == RED_WORKER_MESSAGE_READY);
 }
 
+static void qxl_worker_loadvm_commands(QXLWorker *qxl_worker,
+                                       struct QXLCommandExt *ext,
+                                       uint32_t count)
+{
+    red_dispatcher_loadvm_commands((RedDispatcher*)qxl_worker, ext, count);
+}
+
 void red_dispatcher_set_mm_time(uint32_t mm_time)
 {
     RedDispatcher *now = dispatchers;
@@ -482,6 +547,100 @@  uint32_t red_dispatcher_qxl_ram_size(void)
     return qxl_info.qxl_ram_size;
 }
 
+SPICE_GNUC_VISIBLE
+void spice_qxl_wakeup(QXLInstance *instance)
+{
+    red_dispatcher_wakeup(instance->st->dispatcher);
+}
+
+SPICE_GNUC_VISIBLE
+void spice_qxl_oom(QXLInstance *instance)
+{
+    red_dispatcher_oom(instance->st->dispatcher);
+}
+
+SPICE_GNUC_VISIBLE
+void spice_qxl_start(QXLInstance *instance)
+{
+    red_dispatcher_start(instance->st->dispatcher);
+}
+
+SPICE_GNUC_VISIBLE
+void spice_qxl_stop(QXLInstance *instance)
+{
+    red_dispatcher_stop(instance->st->dispatcher);
+}
+
+SPICE_GNUC_VISIBLE
+void spice_qxl_update_area(QXLInstance *instance, uint32_t surface_id,
+                    struct QXLRect *area, struct QXLRect *dirty_rects,
+                    uint32_t num_dirty_rects, uint32_t clear_dirty_region)
+{
+    red_dispatcher_update_area(instance->st->dispatcher, surface_id, area, dirty_rects,
+                               num_dirty_rects, clear_dirty_region);
+}
+
+SPICE_GNUC_VISIBLE
+void spice_qxl_add_memslot(QXLInstance *instance, QXLDevMemSlot *slot)
+{
+    red_dispatcher_add_memslot(instance->st->dispatcher, slot);
+}
+
+SPICE_GNUC_VISIBLE
+void spice_qxl_del_memslot(QXLInstance *instance, uint32_t slot_group_id, uint32_t slot_id)
+{
+    red_dispatcher_del_memslot(instance->st->dispatcher, slot_group_id, slot_id);
+}
+
+SPICE_GNUC_VISIBLE
+void spice_qxl_reset_memslots(QXLInstance *instance)
+{
+    red_dispatcher_reset_memslots(instance->st->dispatcher);
+}
+
+SPICE_GNUC_VISIBLE
+void spice_qxl_destroy_surfaces(QXLInstance *instance)
+{
+    red_dispatcher_destroy_surfaces(instance->st->dispatcher);
+}
+
+SPICE_GNUC_VISIBLE
+void spice_qxl_destroy_primary_surface(QXLInstance *instance, uint32_t surface_id)
+{
+    red_dispatcher_destroy_primary(instance->st->dispatcher, surface_id);
+}
+
+SPICE_GNUC_VISIBLE
+void spice_qxl_create_primary_surface(QXLInstance *instance, uint32_t surface_id,
+                                QXLDevSurfaceCreate *surface)
+{
+    red_dispatcher_create_primary(instance->st->dispatcher, surface_id, surface);
+}
+
+SPICE_GNUC_VISIBLE
+void spice_qxl_reset_image_cache(QXLInstance *instance)
+{
+    red_dispatcher_reset_image_cache(instance->st->dispatcher);
+}
+
+SPICE_GNUC_VISIBLE
+void spice_qxl_reset_cursor(QXLInstance *instance)
+{
+    red_dispatcher_reset_cursor(instance->st->dispatcher);
+}
+
+SPICE_GNUC_VISIBLE
+void spice_qxl_destroy_surface_wait(QXLInstance *instance, uint32_t surface_id)
+{
+    red_dispatcher_destroy_surface_wait(instance->st->dispatcher, surface_id);
+}
+
+SPICE_GNUC_VISIBLE
+void spice_qxl_loadvm_commands(QXLInstance *instance, struct QXLCommandExt *ext, uint32_t count)
+{
+    red_dispatcher_loadvm_commands(instance->st->dispatcher, ext, count);
+}
+
 RedDispatcher *red_dispatcher_init(QXLInstance *qxl)
 {
     RedDispatcher *dispatcher;
diff --git a/server/spice-server.syms b/server/spice-server.syms
index da7a483..4e120ee 100644
--- a/server/spice-server.syms
+++ b/server/spice-server.syms
@@ -58,6 +58,21 @@  SPICE_SERVER_0.8.2 {
 global:
     spice_server_set_sasl;
     spice_server_set_sasl_appname;
+    spice_qxl_wakeup;
+    spice_qxl_oom;
+    spice_qxl_start;
+    spice_qxl_stop;
+    spice_qxl_update_area;
+    spice_qxl_add_memslot;
+    spice_qxl_del_memslot;
+    spice_qxl_reset_memslots;
+    spice_qxl_destroy_surfaces;
+    spice_qxl_destroy_primary_surface;
+    spice_qxl_create_primary_surface;
+    spice_qxl_reset_image_cache;
+    spice_qxl_reset_cursor;
+    spice_qxl_destroy_surface_wait;
+    spice_qxl_loadvm_commands;
 } SPICE_SERVER_0.8.1;
 
 SPICE_SERVER_0.10.0 {
diff --git a/server/spice.h b/server/spice.h
index f64ff41..86d9ffe 100644
--- a/server/spice.h
+++ b/server/spice.h
@@ -124,6 +124,25 @@  struct QXLWorker {
     void (*loadvm_commands)(QXLWorker *worker, struct QXLCommandExt *ext, uint32_t count);
 };
 
+void spice_qxl_wakeup(QXLInstance *instance);
+void spice_qxl_oom(QXLInstance *instance);
+void spice_qxl_start(QXLInstance *instance);
+void spice_qxl_stop(QXLInstance *instance);
+void spice_qxl_update_area(QXLInstance *instance, uint32_t surface_id,
+                   struct QXLRect *area, struct QXLRect *dirty_rects,
+                   uint32_t num_dirty_rects, uint32_t clear_dirty_region);
+void spice_qxl_add_memslot(QXLInstance *instance, QXLDevMemSlot *slot);
+void spice_qxl_del_memslot(QXLInstance *instance, uint32_t slot_group_id, uint32_t slot_id);
+void spice_qxl_reset_memslots(QXLInstance *instance);
+void spice_qxl_destroy_surfaces(QXLInstance *instance);
+void spice_qxl_destroy_primary_surface(QXLInstance *instance, uint32_t surface_id);
+void spice_qxl_create_primary_surface(QXLInstance *instance, uint32_t surface_id,
+                               QXLDevSurfaceCreate *surface);
+void spice_qxl_reset_image_cache(QXLInstance *instance);
+void spice_qxl_reset_cursor(QXLInstance *instance);
+void spice_qxl_destroy_surface_wait(QXLInstance *instance, uint32_t surface_id);
+void spice_qxl_loadvm_commands(QXLInstance *instance, struct QXLCommandExt *ext, uint32_t count);
+
 typedef struct QXLDrawArea {
     uint8_t *buf;
     uint32_t size;

Comments

On Wed, Jul 20, 2011 at 11:19:52AM +0300, Alon Levy wrote:
> -static void qxl_worker_update_area(QXLWorker *qxl_worker, uint32_t surface_id,
> +static void red_dispatcher_update_area(RedDispatcher *dispatcher, uint32_t surface_id,
>                                     QXLRect *qxl_area, QXLRect *qxl_dirty_rects,
>                                     uint32_t num_dirty_rects, uint32_t clear_dirty_region)
>  {
> -    RedDispatcher *dispatcher = (RedDispatcher *)qxl_worker;
>      RedWorkerMessage message = RED_WORKER_MESSAGE_UPDATE;
>      SpiceRect *dirty_rects = spice_new0(SpiceRect, num_dirty_rects);
>      SpiceRect *area = spice_new0(SpiceRect, 1);
> @@ -241,9 +240,16 @@ static void qxl_worker_update_area(QXLWorker *qxl_worker, uint32_t surface_id,
>      free(area);
>  }
>  
> -static void qxl_worker_add_memslot(QXLWorker *qxl_worker, QXLDevMemSlot *mem_slot)
> +static void qxl_worker_update_area(QXLWorker *qxl_worker, uint32_t surface_id,
> +                                   QXLRect *qxl_area, QXLRect *qxl_dirty_rects,
> +                                   uint32_t num_dirty_rects, uint32_t clear_dirty_region)
> +{
> +    red_dispatcher_update_area((RedDispatcher*)qxl_worker, surface_id, qxl_area,
> +                               qxl_dirty_rects, num_dirty_rects, clear_dirty_region);
> +}


Aren't red_dispatcher_update_area/qxl_worker_update_area redundant? Can't
we just keep the old qxl_worker_update_area and do the cast in there?

> diff --git a/server/spice.h b/server/spice.h
> index f64ff41..86d9ffe 100644
> --- a/server/spice.h
> +++ b/server/spice.h
> @@ -124,6 +124,25 @@ struct QXLWorker {
>      void (*loadvm_commands)(QXLWorker *worker, struct QXLCommandExt *ext, uint32_t count);
>  };

Will the function pointer above go away? My understanding is that the
functions added below are meant to replace these?

>  
> +void spice_qxl_wakeup(QXLInstance *instance);
> +void spice_qxl_oom(QXLInstance *instance);
> +void spice_qxl_start(QXLInstance *instance);
> +void spice_qxl_stop(QXLInstance *instance);
> +void spice_qxl_update_area(QXLInstance *instance, uint32_t surface_id,
> +                   struct QXLRect *area, struct QXLRect *dirty_rects,
> +                   uint32_t num_dirty_rects, uint32_t clear_dirty_region);
> +void spice_qxl_add_memslot(QXLInstance *instance, QXLDevMemSlot *slot);
> +void spice_qxl_del_memslot(QXLInstance *instance, uint32_t slot_group_id, uint32_t slot_id);
> +void spice_qxl_reset_memslots(QXLInstance *instance);
> +void spice_qxl_destroy_surfaces(QXLInstance *instance);
> +void spice_qxl_destroy_primary_surface(QXLInstance *instance, uint32_t surface_id);
> +void spice_qxl_create_primary_surface(QXLInstance *instance, uint32_t surface_id,
> +                               QXLDevSurfaceCreate *surface);
> +void spice_qxl_reset_image_cache(QXLInstance *instance);
> +void spice_qxl_reset_cursor(QXLInstance *instance);
> +void spice_qxl_destroy_surface_wait(QXLInstance *instance, uint32_t surface_id);
> +void spice_qxl_loadvm_commands(QXLInstance *instance, struct QXLCommandExt *ext, uint32_t count);

Christophe
On Wed, Jul 20, 2011 at 10:53:49AM +0200, Christophe Fergeau wrote:
> On Wed, Jul 20, 2011 at 11:19:52AM +0300, Alon Levy wrote:
> > -static void qxl_worker_update_area(QXLWorker *qxl_worker, uint32_t surface_id,
> > +static void red_dispatcher_update_area(RedDispatcher *dispatcher, uint32_t surface_id,
> >                                     QXLRect *qxl_area, QXLRect *qxl_dirty_rects,
> >                                     uint32_t num_dirty_rects, uint32_t clear_dirty_region)
> >  {
> > -    RedDispatcher *dispatcher = (RedDispatcher *)qxl_worker;
> >      RedWorkerMessage message = RED_WORKER_MESSAGE_UPDATE;
> >      SpiceRect *dirty_rects = spice_new0(SpiceRect, num_dirty_rects);
> >      SpiceRect *area = spice_new0(SpiceRect, 1);
> > @@ -241,9 +240,16 @@ static void qxl_worker_update_area(QXLWorker *qxl_worker, uint32_t surface_id,
> >      free(area);
> >  }
> >  
> > -static void qxl_worker_add_memslot(QXLWorker *qxl_worker, QXLDevMemSlot *mem_slot)
> > +static void qxl_worker_update_area(QXLWorker *qxl_worker, uint32_t surface_id,
> > +                                   QXLRect *qxl_area, QXLRect *qxl_dirty_rects,
> > +                                   uint32_t num_dirty_rects, uint32_t clear_dirty_region)
> > +{
> > +    red_dispatcher_update_area((RedDispatcher*)qxl_worker, surface_id, qxl_area,
> > +                               qxl_dirty_rects, num_dirty_rects, clear_dirty_region);
> > +}
> 
> 
> Aren't red_dispatcher_update_area/qxl_worker_update_area redundant? Can't
> we just keep the old qxl_worker_update_area and do the cast in there?

red_disptacher_update_area is used twice - once by qxl_worker_update_area which must remain
for backward compatibility (QXLWorker.update_area is set to it), and second by spice_qxl_update_area
which goes directly from the QXLInstance to the RedDispatcher without going through QXLWorker.

> 
> > diff --git a/server/spice.h b/server/spice.h
> > index f64ff41..86d9ffe 100644
> > --- a/server/spice.h
> > +++ b/server/spice.h
> > @@ -124,6 +124,25 @@ struct QXLWorker {
> >      void (*loadvm_commands)(QXLWorker *worker, struct QXLCommandExt *ext, uint32_t count);
> >  };
> 
> Will the function pointer above go away? My understanding is that the
> functions added below are meant to replace these?

They can't go away as long as we want to maintain backwards compatibility.

> 
> >  
> > +void spice_qxl_wakeup(QXLInstance *instance);
> > +void spice_qxl_oom(QXLInstance *instance);
> > +void spice_qxl_start(QXLInstance *instance);
> > +void spice_qxl_stop(QXLInstance *instance);
> > +void spice_qxl_update_area(QXLInstance *instance, uint32_t surface_id,
> > +                   struct QXLRect *area, struct QXLRect *dirty_rects,
> > +                   uint32_t num_dirty_rects, uint32_t clear_dirty_region);
> > +void spice_qxl_add_memslot(QXLInstance *instance, QXLDevMemSlot *slot);
> > +void spice_qxl_del_memslot(QXLInstance *instance, uint32_t slot_group_id, uint32_t slot_id);
> > +void spice_qxl_reset_memslots(QXLInstance *instance);
> > +void spice_qxl_destroy_surfaces(QXLInstance *instance);
> > +void spice_qxl_destroy_primary_surface(QXLInstance *instance, uint32_t surface_id);
> > +void spice_qxl_create_primary_surface(QXLInstance *instance, uint32_t surface_id,
> > +                               QXLDevSurfaceCreate *surface);
> > +void spice_qxl_reset_image_cache(QXLInstance *instance);
> > +void spice_qxl_reset_cursor(QXLInstance *instance);
> > +void spice_qxl_destroy_surface_wait(QXLInstance *instance, uint32_t surface_id);
> > +void spice_qxl_loadvm_commands(QXLInstance *instance, struct QXLCommandExt *ext, uint32_t count);
> 
> Christophe
On Wed, Jul 20, 2011 at 12:10:07PM +0300, Alon Levy wrote:
> On Wed, Jul 20, 2011 at 10:53:49AM +0200, Christophe Fergeau wrote:
> > On Wed, Jul 20, 2011 at 11:19:52AM +0300, Alon Levy wrote:
> > > -static void qxl_worker_update_area(QXLWorker *qxl_worker, uint32_t surface_id,
> > > +static void red_dispatcher_update_area(RedDispatcher *dispatcher, uint32_t surface_id,
> > >                                     QXLRect *qxl_area, QXLRect *qxl_dirty_rects,
> > >                                     uint32_t num_dirty_rects, uint32_t clear_dirty_region)
> > >  {
> > > -    RedDispatcher *dispatcher = (RedDispatcher *)qxl_worker;
> > >      RedWorkerMessage message = RED_WORKER_MESSAGE_UPDATE;
> > >      SpiceRect *dirty_rects = spice_new0(SpiceRect, num_dirty_rects);
> > >      SpiceRect *area = spice_new0(SpiceRect, 1);
> > > @@ -241,9 +240,16 @@ static void qxl_worker_update_area(QXLWorker *qxl_worker, uint32_t surface_id,
> > >      free(area);
> > >  }
> > >  
> > > -static void qxl_worker_add_memslot(QXLWorker *qxl_worker, QXLDevMemSlot *mem_slot)
> > > +static void qxl_worker_update_area(QXLWorker *qxl_worker, uint32_t surface_id,
> > > +                                   QXLRect *qxl_area, QXLRect *qxl_dirty_rects,
> > > +                                   uint32_t num_dirty_rects, uint32_t clear_dirty_region)
> > > +{
> > > +    red_dispatcher_update_area((RedDispatcher*)qxl_worker, surface_id, qxl_area,
> > > +                               qxl_dirty_rects, num_dirty_rects, clear_dirty_region);
> > > +}
> > 
> > 
> > Aren't red_dispatcher_update_area/qxl_worker_update_area redundant? Can't
> > we just keep the old qxl_worker_update_area and do the cast in there?
> 
> red_disptacher_update_area is used twice - once by qxl_worker_update_area which must remain
> for backward compatibility (QXLWorker.update_area is set to it), and second by spice_qxl_update_area
> which goes directly from the QXLInstance to the RedDispatcher without going through QXLWorker.

Given the (RedDispatcher*)qxl_worker cast, in this case a RedDispatcher
will also be a QXLWorker and vice versa, so I don't understand why we need
a wrapper whose only added value is an obvious cast.

> > Will the function pointer above go away? My understanding is that the
> > functions added below are meant to replace these?
> 
> They can't go away as long as we want to maintain backwards compatibility.

Ok, I'd have gone with using these to implement spice_qxl_wakeup et al:
void spice_qxl_wakeup(QXLInstance *instance)
{
    QXLWorker *worker = (QXLWorker *)instance->st->dispatcher;
    ASSERT(worker->wakeup != NULL)
    worker->wakeup(worker);
}

or something like that

But maybe that's something kraxel wanted to avoid.

Christophe
>> diff --git a/server/spice.h b/server/spice.h
>> index f64ff41..86d9ffe 100644
>> --- a/server/spice.h
>> +++ b/server/spice.h
>> @@ -124,6 +124,25 @@ struct QXLWorker {
>>       void (*loadvm_commands)(QXLWorker *worker, struct QXLCommandExt *ext, uint32_t count);
>>   };
>
> Will the function pointer above go away? My understanding is that the
> functions added below are meant to replace these?

I'd love to, but as this is part of the library API they have to stay 
for backward compatibility reasons.

cheers,
   Gerd
> Ok, I'd have gone with using these to implement spice_qxl_wakeup et al:
> void spice_qxl_wakeup(QXLInstance *instance)
> {
>      QXLWorker *worker = (QXLWorker *)instance->st->dispatcher;

QXLWorker *worker = &instance->st->dispatcher->base;

avoids the cast.

>      ASSERT(worker->wakeup != NULL)
>      worker->wakeup(worker);
> }
>
> or something like that
>
> But maybe that's something kraxel wanted to avoid.

Would work too.  Although the code is more consistent the way alon did 
it as the new spice_qxl_*_async() calls added a few patches later don't 
get a worker function pointer.

cheers,
   Gerd
On Wed, Jul 20, 2011 at 11:33:39AM +0200, Christophe Fergeau wrote:
> On Wed, Jul 20, 2011 at 12:10:07PM +0300, Alon Levy wrote:
> > On Wed, Jul 20, 2011 at 10:53:49AM +0200, Christophe Fergeau wrote:
> > > On Wed, Jul 20, 2011 at 11:19:52AM +0300, Alon Levy wrote:
> > > > -static void qxl_worker_update_area(QXLWorker *qxl_worker, uint32_t surface_id,
> > > > +static void red_dispatcher_update_area(RedDispatcher *dispatcher, uint32_t surface_id,
> > > >                                     QXLRect *qxl_area, QXLRect *qxl_dirty_rects,
> > > >                                     uint32_t num_dirty_rects, uint32_t clear_dirty_region)
> > > >  {
> > > > -    RedDispatcher *dispatcher = (RedDispatcher *)qxl_worker;
> > > >      RedWorkerMessage message = RED_WORKER_MESSAGE_UPDATE;
> > > >      SpiceRect *dirty_rects = spice_new0(SpiceRect, num_dirty_rects);
> > > >      SpiceRect *area = spice_new0(SpiceRect, 1);
> > > > @@ -241,9 +240,16 @@ static void qxl_worker_update_area(QXLWorker *qxl_worker, uint32_t surface_id,
> > > >      free(area);
> > > >  }
> > > >  
> > > > -static void qxl_worker_add_memslot(QXLWorker *qxl_worker, QXLDevMemSlot *mem_slot)
> > > > +static void qxl_worker_update_area(QXLWorker *qxl_worker, uint32_t surface_id,
> > > > +                                   QXLRect *qxl_area, QXLRect *qxl_dirty_rects,
> > > > +                                   uint32_t num_dirty_rects, uint32_t clear_dirty_region)
> > > > +{
> > > > +    red_dispatcher_update_area((RedDispatcher*)qxl_worker, surface_id, qxl_area,
> > > > +                               qxl_dirty_rects, num_dirty_rects, clear_dirty_region);
> > > > +}
> > > 
> > > 
> > > Aren't red_dispatcher_update_area/qxl_worker_update_area redundant? Can't
> > > we just keep the old qxl_worker_update_area and do the cast in there?
> > 
> > red_disptacher_update_area is used twice - once by qxl_worker_update_area which must remain
> > for backward compatibility (QXLWorker.update_area is set to it), and second by spice_qxl_update_area
> > which goes directly from the QXLInstance to the RedDispatcher without going through QXLWorker.
> 
> Given the (RedDispatcher*)qxl_worker cast, in this case a RedDispatcher
> will also be a QXLWorker and vice versa, so I don't understand why we need
> a wrapper whose only added value is an obvious cast.
> 
> > > Will the function pointer above go away? My understanding is that the
> > > functions added below are meant to replace these?
> > 
> > They can't go away as long as we want to maintain backwards compatibility.
> 
> Ok, I'd have gone with using these to implement spice_qxl_wakeup et al:
> void spice_qxl_wakeup(QXLInstance *instance)
> {
>     QXLWorker *worker = (QXLWorker *)instance->st->dispatcher;
>     ASSERT(worker->wakeup != NULL)
>     worker->wakeup(worker);
> }
> 
> or something like that
> 
> But maybe that's something kraxel wanted to avoid.
> 

I dunno, both would work. None look particularily pretty. Do you mind
if I don't do another rewrite?

> Christophe



> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
On Wed, Jul 20, 2011 at 12:53:14PM +0300, Alon Levy wrote:
> > But maybe that's something kraxel wanted to avoid.
> > 
> 
> I dunno, both would work. None look particularily pretty. Do you mind
> if I don't do another rewrite?

I'm just worried about things going bad down the road. Ie some of the pointers
being set to some new functions, the wrappers not being updated, and then
really weird bugs occurring.

Time will tell...

Christophe