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

[Christophe Fergeau]

On Wed, Aug 30, 2017 at 06:22:38AM -0400, Frediano Ziglio wrote:
> > Something which probably would work would be to introduce a
> > RedChannel::dispatcher, and change
> > red_channel_disconnect_client/red_channel_migrate_client:
> > if RedChannel::dispatcher is set, then you invoke the vfunc in a
> > separate thread, if RedChannel::dispatcher is not set, then you invoke
> > the vfunc directly.
> > 
> 
> This move the thread responsibility to RedChannel and does not
> allows to change the dispatching implementation.

This moves the decision of whether to dispatch in the current thread, or
in the channel thread to RedChannel yes. If we need different
dispatching implementations, I don't think it would be too hard to have
some generic dispatcher interface we would use..

> I think the definition "RedChannel can run in a separate single
> thread except build and destroy" is easy to understand.

Yup, dunno if this is written clearly somewhere at the moment.
I'd prefer destruction to happen in the channel thread to be honest,
otherwise you always have the possibility that the channel thread is
going to do 'something' while it's being destroyed.

>   This was basically the idea behind ClientCbs which was lost in all
>   the changes done. The idea behind was that RedClient should access
>   RCC only using ClientCbs method which were supposed to
>   be thread safe. But having this interface just as
>   RedChannelClient method make easier to introduce a call to
>   an unsafe method.

This was the idea, but this was not documented, so nobody paid closed
attention to that while refactoring code, so this was totally lost :-/
So in the end, it's not much different from not having this at all..
Also, you have these ClientCbs, but nothing prevents you from directly
calling RedChannelClient methods directly. So you have to know and
remember you should be using ClientCbs.
In other words, this needs to be documented, and we can just as well
document RedChannelClient external API and achieve the same effect.
This is similar to what spice-gtk does with annotations whether code
runs in the main context or in coroutine context.

And this also gets rid of some layers of indirection when trying to
follow the code. With ClientCbs + a dispatcher, takes a bit of jumping
around to get to handle_dev_cursor_disconnect() in red-worker.c, while
the corresponding code in eg the MainChannelClient is directly in the
main-channel-client.c file (or main-channel.c).
You'd have to make sure you don't forget to keep that code from
red-worker.c when you move CursorChannel to the main thread, while with
my suggestion, the disconnect_client/migrate_client/connect_client code
all lives in the CursorChannelClient implementation, so nothing to
change when you move it from running in a separate thread.

For what it's worth, a very rough (compile-only, would not run) proof of
concept would be https://cgit.freedesktop.org/~teuf/spice/commit/?h=clientcbs&id=6a0311831f8752e9854b0d683c361ac5b5a38525

Christophe