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

[Christophe Fergeau]

On Mon, Oct 31, 2016 at 06:07:39AM -0400, Frediano Ziglio wrote:
> > 
> > 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.

Yeah, not disagreeing with any of this, my main point was mostly it
would be nice for the commit log to mention this. That there are no
issues at the moment, but it's preferrable to init the GQueue as early
as possible, ie in _init().

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20161102/1d5979cd/attachment.sig>