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

[Frediano Ziglio]

> 
> > On 30 Aug 2017, at 09:31, Frediano Ziglio <fziglio at redhat.com> 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.
> 
> I believe you meant “that remove this requirement”. As written, it means
> that you don’t need a CursorChannel, but I believe the CursorChannel
> is still there, only the QXL device in it is gone, right?
> 

I mean that CursorChannel does not need a QXL instance to run.

> > 
> > 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.
> 
> Unfortunately, I’m confused by your explanation. Please clarify:
> 
> - By “the original design”, you mean your patch series, or the code before?
> (I believe it’s your patch series)
> 

Before, even years ago.

> - By “now”, you mean “Jonathon’s patch”, or your patch?
> (I believe you mean Jonathon’s patch)
> 

Yes, after this series

> - Why is being able to use the main thread or a separate one more “forced”
> than (always?) making a channel run in a separate thread? Naively, it seems
> like there are more choices in the “forced” case (namely, the main thread).
> So I think you wanted to write something else than what you wrote. But that
> part I was unable to make sense of by myself, so that’s really my primary
> question :-)
> 

After these patches the CursorChannel and DisplayChannel hardcoded the
need for a Dispatcher which means that there's a separate thread
receiving the requests. This as the callbacks/methods have only the
implementation calling the dispatcher.

> In any case, if you do another iteration of your patch series, would it be
> possible
> to make sure this explanation is part of the envelope or commit message?
> 
> 
> > 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)
> 
> Is the CursorChannel the only one running from the main thread?
> If so, what would be the rationale / trade-off for choosing one approach
> vs. the other? I.e. if I add a new channel, what do I pick?
> 

The current (master) design is flexible. It allows to write
channels that can run in the main thread or a separate one.
Currently (master) CursorChannel and DisplayChannel run on
separate threads (the reason is that image compression could
take time and is not a good idea to suspend the main thread
that long) but this is not in these channel implementation
but on the RedQXL/RedWorker implementation (RedQxl and
RedWorker are close friends basically implementing this
thread separation, RedQxl implements the marshalling part
of the dispatcher while RedWorker implements the unmarshalling
one).

I think the possibility to have a single implementation
that could run in either the main thread or in another is
a good feature. If in the future one channel like SpiceVMC
get expensive to run inside the main thread we could move
easily or decide where to run dynamically.

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