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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel