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

[Christophe de Dinechin]

> 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?

> 
> 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel