> > > On 30 Aug 2017, at 09:31, Frediano Ziglio <fziglio@xxxxxxxxxx> 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(-) > >> _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel