[Spice-devel] [PATCH spice-server 3/6] red-channel-client: Init pipe field during init

[Frediano Ziglio]

> 
> On Fri, Oct 28, 2016 at 11:59:53AM +0100, Frediano Ziglio wrote:
> > There was a chance that on error GQueue were not
> > initialized but an attempt to destroy it is made.
> > This assure GQueue is initialized as soon as
> 
> s/assure/ensures
> 

Fixed, thanks

> > possible. Note that red_channel_client_initable_init
> > is called after all init and construction callbacks.
> 
> Did you hit an actual problem? Or did you notice this through
> code-review?
> 
> What g_queue_init() is doing is setting all GQueue fields to 0, which
> will be the case here anyway as the private data will be set to 0
> initially, so trying to use priv->pipe before use should not cause harm.
> 
> I agree it's undefined behaviour though, so I'm fine with the patch if
> this is something which was noticed during code reading. If you hit
> crashes or issues related to that, I'd prefer that we have more details
> about this in the log.
> 
> Christophe
> 

I was reviewing new patches from refactory and one touched initialization
then I dig more into order and came to find some difference.
Basically the order was red_channel_client_create which initialized
everything for RedChannelClient then some additional fields for parents
like CursorChannel. Now there are:
- GObject init
- GObject properties settings
- GObject constructed
- red_channel_client_initable_init. Note that if a parent override
  initable interface this could be never called while previously
  this was impossible (for "dummy" channel you had another function).
So in theory some constructed could set some item in the pipe and
then g_queue_init would leak these items (and this will avoid sending them).
Previously this couldn't happen so I decide that is safer to move
the field initialization. About calling g_queue_init or not as already
zeroed usually I assume that if there is an *_init I have to call to
maintain compatibility with future g_queue_init implementation (not that
I think will change but I don't like to assume it). Also g_queue_init
was called before.

Frediano


> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/red-channel-client.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> > index c562e8e..3b2c24c 100644
> > --- a/server/red-channel-client.c
> > +++ b/server/red-channel-client.c
> > @@ -354,6 +354,8 @@ red_channel_client_init(RedChannelClient *self)
> >      self->priv->send_data.urgent.marshaller = spice_marshaller_new();
> >  
> >      self->priv->send_data.marshaller =
> >      self->priv->send_data.main.marshaller;
> > +
> > +    g_queue_init(&self->priv->pipe);
> >  }
> >  
> >  RedChannel* red_channel_client_get_channel(RedChannelClient *rcc)
> > @@ -907,7 +909,6 @@ static gboolean
> > red_channel_client_initable_init(GInitable *initable,
> >      self->priv->outgoing.pos = 0;
> >      self->priv->outgoing.size = 0;
> >  
> > -    g_queue_init(&self->priv->pipe);
> >      if (self->priv->stream)
> >          self->priv->stream->watch =
> >              core->watch_add(core, self->priv->stream->socket,