[Spice-devel] [PATCH 2/2] Convert RedChannelClient hierarchy to GObject

[Frediano Ziglio]

> 
> On Tue, 2016-09-27 at 05:50 -0400, Frediano Ziglio wrote:
> > > 
> > > 
> > > Convert the RedChannelClient heirarchy into GObjects. Since the
> > > existing
> > > constructors could fail and return NULL, I inherited the base
> > > channel
> > > client from GInitable, which introduces a dependency on gio.
> > > 
> > > When using private structs with GObject, there's a maximum size of
> > > (I
> > > think) 64k, which was exceeded by some of the private structs. To
> > > avoid
> > > this limitation I changed some members to dynamically allocated.
> > > ---
> > >  configure.ac                        |   4 +-
> > >  server/Makefile.am                  |   2 +
> > >  server/cursor-channel-client.c      |  74 ++--
> > >  server/cursor-channel-client.h      |  34 +-
> > >  server/dcc-private.h                |  14 +-
> > >  server/dcc.c                        | 202 ++++++++--
> > >  server/dcc.h                        |  37 +-
> > >  server/display-channel.c            |   4 +-
> > >  server/dummy-channel-client.c       | 156 ++++++++
> > >  server/dummy-channel-client.h       |  59 +++
> > >  server/inputs-channel-client.c      |  56 ++-
> > >  server/inputs-channel-client.h      |  47 ++-
> > >  server/main-channel-client.c        | 146 +++++++-
> > >  server/main-channel-client.h        |  34 +-
> > >  server/red-channel-client-private.h |   4 +-
> > >  server/red-channel-client.c         | 714
> > >  ++++++++++++++++++++++--------------
> > >  server/red-channel-client.h         |  80 +++-
> > >  server/red-channel.h                |  33 +-
> > >  server/reds.h                       |   1 +
> > >  server/smartcard-channel-client.c   | 120 +++++-
> > >  server/smartcard-channel-client.h   |  42 ++-
> > >  server/smartcard.c                  |  30 +-
> > >  server/sound.c                      |   9 +-
> > >  server/spice-server.h               |  16 +
> > >  server/spicevmc.c                   |   6 +-
> > >  server/tests/test_display_base.c    |   4 +-
> > >  26 files changed, 1441 insertions(+), 487 deletions(-)
> > >  create mode 100644 server/dummy-channel-client.c
> > >  create mode 100644 server/dummy-channel-client.h
> > > 
> > 
> > I didn't find much time to look a bit deeper at this patch as a
> > whole.
> > 
> > I tried to focus on the DummyChannelClient and the reason for this.
> > First, this "feature" can be easily implemented supporting the
> > case having stream == NULL in RedChannelClient.
> > But I don't understand the rationale for this feature. It's only
> > used in sound.c to handle record/playback. After a first read I
> > though was to handle message encapsulated in a different way but
> > sound.c use the same encapsulation. So I don't really understand
> > why sound.c have to handle RedsStream directly and cannot use
> > the same code provided by RedChannelClient as all other channels
> > do. SndChannel seems to contain lot of code copied and pasted
> > from RedChannelClient.
> > I personally think that the approach should use RedChannelClient
> > and not reinventing the wheel and the creation of DummyChannelClient
> > is just going in the wrong direction.
> > 
> > Does anybody is comfortable with sound.c and can explain why
> > not using ReChannelClient?
> > 
> > Frediano
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> 
> 
> Yes, I agree that it's strange, and I don't like it. But as part of the
> refactoring, I decided to keep the "dummy" channel client idea and just
> port it to a GObject style. I did not want to do a lot of intrusive re-
> designing as part of this refactory. I think we can make more
> improvements later.
> 
> Jonathon
> 

I was mainly hoping somebody step in, explaining the reason, agreeing
that now is easier to change the code and do it but seems that this is
another piece of code we don't understand :(

Fine, let post it. I'll write another todo... or start another branch...

Haven't spent much more time on a reasonable split beside the first one.

Frediano