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

[Frediano Ziglio]

> 
> 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 fact that something was designed in a given way does not
mean that cannot be changed. Currently the only reason for
QXL in CursorChannel is to free resources, stuff that can be
done easily in red_put_cursor_cmd.

Your patches move part of the responsibility to handle the dispatching
from RedQxl to CursorChannel and DisplayChannel. It's true that there's
lot of information in red-qxl.h but 80% of these are stuff for RedQxl
and RedWorker. Maybe would be a good idea to separate these part
in a red-qxl-private.h header.

> 
> > 
> > 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?
>

Well, in a separate structure are a bit more separate.

I don't disagree to have these callback moved to virtual functions,
only that they should not deal with dispatching and threading
but just deal with channel stuff.

I agree that the current design require to tweak some internal
callback to make the behaviour change but spread responsibility
freely looks like a move back when there was a single file with
all RedWorker, CursorChannel and DisplayChannel together.

Looking at the GStreamer issue was thinking about the possibility
to move the entire RedWorker to a separate process. Not easy
to achieve but I think a design that would suite this would be
more sensible to have.

> > 
> > 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(-)
> > > 
>