[Spice-devel] [PATCH spice-server 0/4] Refactor RedChannel ClientCbs

[Jonathon Jongsma]

On Wed, 2017-08-30 at 03:31 -0400, Frediano Ziglio wrote:
> > This is an alternate proposal to the patch Frediano sent recently
> > which
> > includes the data argument in the ClientCbs struct and removes the
> > g_object_get|set_data() calls.
> > 
> > This series also removes the GObject data stuff, but also does some
> > deeper refactoring.
> > 
> 
> 
> Honestly does not seem that great.
> 
> It assumes CursorChannel and DisplayChannel will always have an
> associated QXL device. There are different patches that remove this
> needs for CursorChannel.

Yes, it does assume that DisplayChannel and CursorChannel will have an
associated QXL device, but that's not that surprising since these
classes were essentially designed around QXL devices. I know your
recent patch removed that requirement from CursorChannel, but I'm not
sure that's the best design, to be honest. Although it works, it feels
a bit like we're trying to squeeze this class into a shape that it
wasn't designed to fill. 


> 
> The original design was that these callbacks could be overridden
> to make any channel (type+id meaning) run in a separate thread.
> Now we are forced to either use from the main thread or from a
> separate one.
> 
> For instance in my streaming patches I use the CursorChannel
> from the main thread, now I would have to:
> - write a fake QXL device to make CursorChannel happy;
> - create a new Dispatcher to handle requests from CursorChannel;
> - create another thread to handle synchronous calls (like
>   disconnect)

Or we could modify this patch to work without QXL as well. Something
like:

diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index d3c1dcc71..363fb0d76 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -442,16 +442,20 @@ static void red_qxl_set_cursor_peer(RedChannel
*channel, RedClient *client, Reds
 {
     RedWorkerMessageCursorConnect payload = {0,};
     QXLInstance *qxl =
common_graphics_channel_get_qxl(COMMON_GRAPHICS_CHANNEL(channel));
-    Dispatcher *dispatcher = red_qxl_get_dispatcher(qxl);
-    spice_printerr("");
-    payload.client = client;
-    payload.stream = stream;
-    payload.migration = migration;
-    red_channel_capabilities_init(&payload.caps, caps);
-
-    dispatcher_send_message(dispatcher,
-                            RED_WORKER_MESSAGE_CURSOR_CONNECT,
-                            &payload);
+    if (qxl) {
+        Dispatcher *dispatcher = red_qxl_get_dispatcher(qxl);
+        spice_printerr("");
+        payload.client = client;
+        payload.stream = stream;
+        payload.migration = migration;
+        red_channel_capabilities_init(&payload.caps, caps);
+
+        dispatcher_send_message(dispatcher,
+                                RED_WORKER_MESSAGE_CURSOR_CONNECT,
+                                &payload);
+    } else {
+        cursor_channel_connect((CURSOR_CHANNEL(channel), client,
stream, migration, caps);
+    }
 }
 
 static void red_qxl_disconnect_cursor_peer(RedChannel *channel,
RedChannelClient *rcc)


> 
> Also make harder to understand why these calls are separate
> as they are just some different methods while before were
> contained in a different structure.

I don't really see this as a disadvantage. Why does it matter that they
are visually separate?

> 
> I really Nack this approach.
> 
> Frediano
> 
> > Jonathon Jongsma (4):
> >   red-worker: don't pass 'dispatcher' as data for ClientCbs
> >   red-qxl: remove use of g_object_get_data()
> >   Move Cursor and Display client cbs to channel file
> >   Convert ClientCbs to channel virtual functions
> > 
> >  server/common-graphics-channel.c |   1 -
> >  server/cursor-channel.c          |  63 +++++++++++++++++++
> >  server/display-channel.c         |  69 +++++++++++++++++++++
> >  server/inputs-channel.c          |  11 ++--
> >  server/main-channel-client.c     |   8 ---
> >  server/main-channel-client.h     |   1 -
> >  server/main-channel.c            |  13 ++--
> >  server/red-channel.c             |  54 +++++++---------
> >  server/red-channel.h             |  25 ++++----
> >  server/red-qxl.c                 | 129
> >  +--------------------------------------
> >  server/red-worker.c              |  11 +---
> >  server/red-worker.h              |   4 +-
> >  server/smartcard.c               |   6 +-
> >  server/sound.c                   |  21 +++----
> >  server/spicevmc.c                |   7 +--
> >  15 files changed, 196 insertions(+), 227 deletions(-)
> >