[Spice-devel] [PATCH spice-server v2] Manage lifetime of RedVmcChannel

[Frediano Ziglio]

> On Wed, 2016-11-02 at 13:05 -0400, Frediano Ziglio wrote:
> > > 
> > > 
> > > On Wed, 2016-11-02 at 08:31 -0400, Frediano Ziglio wrote:
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > On Wed, Nov 02, 2016 at 04:34:47AM -0400, Frediano Ziglio
> > > > > wrote:
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > From: Jonathon Jongsma <jjongsma at redhat.com>
> > > > > > > 
> > > > > > > Store the channel in the char device object, and unref it
> > > > > > > in
> > > > > > > dispose.
> > > > > > > Previously, we were just creating a channel and allowing it
> > > > > > > to
> > > > > > > leak.
> > > > > > > There may be better long-term solutions, but this works for
> > > > > > > now.
> > > > > > > ---
> > > > > > >  server/spicevmc.c | 27 ++++++++++++++++++++-------
> > > > > > >  1 file changed, 20 insertions(+), 7 deletions(-)
> > > > > > > 
> > > > > > > Changes:
> > > > > > > - squashed fixup.
> > > > > > > 
> > > > > > > diff --git a/server/spicevmc.c b/server/spicevmc.c
> > > > > > > index 7067bc7..a77e868 100644
> > > > > > > --- a/server/spicevmc.c
> > > > > > > +++ b/server/spicevmc.c
> > > > > > > @@ -47,6 +47,9 @@
> > > > > > >  #define BUF_SIZE (64 * 1024 + 32)
> > > > > > >  #define COMPRESS_THRESHOLD 1000
> > > > > > >  
> > > > > > > +typedef struct RedVmcChannel RedVmcChannel;
> > > > > > > +typedef struct RedVmcChannelClass RedVmcChannelClass;
> > > > > > > +
> > > > > > >  typedef struct RedVmcPipeItem {
> > > > > > >      RedPipeItem base;
> > > > > > >  
> > > > > > > @@ -75,6 +78,7 @@ typedef struct RedCharDeviceSpiceVmcClass
> > > > > > > RedCharDeviceSpiceVmcClass;
> > > > > > >  
> > > > > > >  struct RedCharDeviceSpiceVmc {
> > > > > > >      RedCharDevice parent;
> > > > > > > +    RedVmcChannel *channel;
> > > > > > >  };
> > > > > > >  
> > > > > > >  struct RedCharDeviceSpiceVmcClass
> > > > > > > @@ -89,9 +93,6 @@ static RedCharDevice
> > > > > > > *red_char_device_spicevmc_new(SpiceCharDeviceInstance *sin,
> > > > > > >  
> > > > > > >  G_DEFINE_TYPE(RedCharDeviceSpiceVmc,
> > > > > > > red_char_device_spicevmc,
> > > > > > >  RED_TYPE_CHAR_DEVICE)
> > > > > > >  
> > > > > > > -#define RED_CHAR_DEVICE_SPICEVMC_PRIVATE(o) \
> > > > > > > -    (G_TYPE_INSTANCE_GET_PRIVATE ((o),
> > > > > > > RED_TYPE_CHAR_DEVICE_SPICEVMC,
> > > > > > > RedCharDeviceSpiceVmcPrivate))
> > > > > > > -
> > > > > > >  #define RED_TYPE_VMC_CHANNEL red_vmc_channel_get_type()
> > > > > > >  
> > > > > > >  #define RED_VMC_CHANNEL(obj) \
> > > > > > > @@ -103,9 +104,6 @@ G_DEFINE_TYPE(RedCharDeviceSpiceVmc,
> > > > > > > red_char_device_spicevmc, RED_TYPE_CHAR_DEV
> > > > > > >  #define RED_VMC_CHANNEL_GET_CLASS(obj) \
> > > > > > >      (G_TYPE_INSTANCE_GET_CLASS((obj),
> > > > > > > RED_TYPE_VMC_CHANNEL,
> > > > > > >      RedVmcChannelClass))
> > > > > > >  
> > > > > > > -typedef struct RedVmcChannel RedVmcChannel;
> > > > > > > -typedef struct RedVmcChannelClass RedVmcChannelClass;
> > > > > > > -
> > > > > > >  struct RedVmcChannel
> > > > > > >  {
> > > > > > >      RedChannel parent;
> > > > > > > @@ -855,9 +853,13 @@ RedCharDevice
> > > > > > > *spicevmc_device_connect(RedsState
> > > > > > > *reds,
> > > > > > >                                         SpiceCharDeviceInst
> > > > > > > ance
> > > > > > > *sin,
> > > > > > >                                         uint8_t
> > > > > > > channel_type)
> > > > > > >  {
> > > > > > > +    RedCharDeviceSpiceVmc *dev_state;
> > > > > > >      RedVmcChannel *state = red_vmc_channel_new(reds,
> > > > > > > channel_type, sin);
> > > > > > >  
> > > > > > > -    return state->chardev;
> > > > > > > +    dev_state = RED_CHAR_DEVICE_SPICEVMC(state->chardev);
> > > > > > > +    dev_state->channel = state;
> > > > > > > +
> > > > > > > +    return RED_CHAR_DEVICE(dev_state);
> > > > > > >  }
> > > > > > >  
> > > > > > >  /* Must be called from RedClient handling thread. */
> > > > > > > @@ -904,10 +906,21 @@ SPICE_GNUC_VISIBLE void
> > > > > > > spice_server_port_event(SpiceCharDeviceInstance *sin, ui
> > > > > > >  }
> > > > > > >  
> > > > > > >  static void
> > > > > > > +red_char_device_spicevmc_dispose(GObject *object)
> > > > > > > +{
> > > > > > > +    RedCharDeviceSpiceVmc *self =
> > > > > > > RED_CHAR_DEVICE_SPICEVMC(object);
> > > > > > > +
> > > > > > > +    g_clear_object(&self->channel);
> > > > > > 
> > > > > > This call produce a double free.
> > > > > > The channel is already freed in spicevmc_device_disconnect.
> > > > > 
> > > > > But g_clear_object() verify if channel is null before the unref
> > > > > and
> > > > > set
> > > > > it to NULL afterwards.
> > > > > 
> > > > 
> > > > Doing a NULL check on a dangling pointer is not much helpful.
> > > 
> > > Hmm, you're right. If we're going to remove the 'opaque' property
> > > (in a
> > > different patch), I think we do need to keep a pointer to the
> > > channel
> > > inside of the char device. But perhaps it should be a weak
> > > reference
> > > instead. So maybe we could just remove the dispose changes from
> > > this
> > > patch and squash it with that patch?
> > > 
> > 
> > Yes, I think we should kind of merge part of this patch in the other
> > one.
> > This could also be fixed setting the pointer to NULL after calling
> > spicevmc_device_disconnect.
> > 
> > > 
> > > Also: perhaps the RedsState object should hold a reference to the
> > > channels added via reds_register_channel() and unref the channel in
> > > reds_unregister_channel(). It seems that it shouldn't be necessary
> > > to
> > > call red_channel_destroy() explicitly. This should just happen in
> > > the
> > > RedChannel destructor when the last ref is dropped...
> > > 
> > > 
> > 
> > There is already a patch in the queue that does this; you wrote it :)
> > (cfr "RedsState: keep weak reference to channels").
> 
> Yeah, I actually wrote a follow-up email saying the exact same thing
> right after I wrote the one you're quoting  ;) Except I forgot to send
> it. It's been too long since I wrote these patches, I don't even
> remember what they are anymore :)
> 
> 
> > This patch does not seem really in line with your proposal for
> > spice_server_destroy/reds_exit changes.
> 
> Yeah, perhaps I need to take a step back and evaluate things anew now
> that we're basically at the end of the refactory merging.
> 

Honestly I consider the refactory finished. The big last patches were
GObject ones, now the "missing" one are just follow ups not in the
initial 400 (more or less) patches.

So time to celebrate :)

Frediano