> 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? > > 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) - By “now”, you mean “Jonathon’s patch”, or your patch? (I believe you mean Jonathon’s patch) - 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 :-) 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? > 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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel