[Spice-devel] [PATCH spice-server v2 5/8] Add CommonGraphicsChannelPrivate struct

[Frediano Ziglio]

> 
> On Mon, 2016-10-10 at 12:20 -0400, Frediano Ziglio wrote:
> > > 
> > > 
> > > From: Jonathon Jongsma <jjongsma at redhat.com>
> > > 
> > > Encapsulate private data for CommonGraphicsChannel and prepare for
> > > GObject conversion.
> > 
> > This object has all the fields with accessors... not a really private
> > I would say. Is not possible to implement in a different way using
> > this structure in CursorChannelPrivate and DisplayChannelPrivate ?
> 
> Possibly, but I'm not sure that I see the advantage of doing so. Also
> note that the DisplayChannelClient currently uses some of these
> variables, so we still need to access/modify this data from outside of
> the DisplayChannel implementation.
> 

Well, dcc include display-channel-private.h so it's not an issue.
I was just wondering if in GObject there is such feature of sharing
some private behavior.

> 
> > 
> > > 
> > > ---
> > >  server/common-graphics-channel.c | 34
> > > ++++++++++++++++++++++++++++++++--
> > >  server/common-graphics-channel.h | 14 ++++++--------
> > >  server/cursor-channel-client.c   |  2 +-
> > >  server/cursor-channel.c          |  8 +++++---
> > >  server/dcc-send.c                |  2 +-
> > >  server/dcc.c                     |  8 ++++----
> > >  server/display-channel.c         |  6 +++---
> > >  server/red-worker.c              |  7 ++++---
> > >  8 files changed, 56 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/server/common-graphics-channel.c
> > > b/server/common-graphics-channel.c
> > > index bcf7279..6871540 100644
> > > --- a/server/common-graphics-channel.c
> > > +++ b/server/common-graphics-channel.c
> > > @@ -27,6 +27,20 @@
> > >  #include "dcc.h"
> > >  #include "main-channel-client.h"
> > >  
> > > +#define CHANNEL_RECEIVE_BUF_SIZE 1024
> > > +
> > > +struct CommonGraphicsChannelPrivate
> > > +{
> > > +    QXLInstance *qxl;
> > > +    uint8_t recv_buf[CHANNEL_RECEIVE_BUF_SIZE];
> > > +    uint32_t id_alloc; // bitfield. TODO - use this instead of
> > > shift scheme.
> > 
> > No reason to introduced again this unused field.
> 
> Oops, must have slipped back in during the rebase.
> 
> > 
> > > @@ -123,7 +137,23 @@ CommonGraphicsChannel*
> > > common_graphics_channel_new(RedsState *server,
> > >      spice_return_val_if_fail(channel, NULL);
> > >  
> > >      common = COMMON_GRAPHICS_CHANNEL(channel);
> > > -    common->qxl = qxl;
> > > +    common->priv = g_new0(CommonGraphicsChannelPrivate, 1);
> > > +    common->priv->qxl = qxl;
> > >      return common;
> > >  }
> > >  
> > 
> > new and no free, leak... but only till next patch so it's not a big
> > issue.
> 
> Hmm, yeah. We could do the 1-element array trick again, but that would
> require us to have the private struct definition available in the
> header. Not sure it's worth it.
> 
> 
> > 
> > > 
> > > +void
> > > common_graphics_channel_set_during_target_migrate(CommonGraphicsCha
> > > nnel
> > > *self, gboolean value)
> > > +{
> > > +    self->priv->during_target_migrate = value;
> > > +}
> > > +
> > > +gboolean
> > > common_graphics_channel_get_during_target_migrate(CommonGraphicsCha
> > > nnel
> > > *self)
> > > +{
> > > +    return self->priv->during_target_migrate;
> > > +}
> > > +
> > 
> > This field should really not be in this "private" structure
> 
> Can you expand on this comment? Where do you think it should be?
> 
> 
> Jonathon
> 

I was just thinking if staying in CommonGraphicsChannel instead of having
some fake private field is not better in this case.

Frediano