On Mon, 2016-10-10 at 12:20 -0400, Frediano Ziglio wrote:
> >
> >
> > From: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> >
> > 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.
>
> >
> > ---
> > 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel