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

2016-11-03 Thread 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 
> > > > > > > 
> > > > > > > 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(>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
> > >

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

2016-11-02 Thread Jonathon Jongsma 
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 
> > > > > > 
> > > > > > 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(>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

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

2016-11-02 Thread Frediano Ziglio 
> 
> 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 
> > > > > 
> > > > > 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,
> > > > > SpiceCharDeviceInstance
> > > > > *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(>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

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

2016-11-02 Thread Jonathon Jongsma 
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 
> > > > 
> > > > 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,
> > > > SpiceCharDeviceInstance
> > > > *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(>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?

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...


> 
> > 
> > > 
> > > 
> > > > 
> > > > +}
> > > > +
> > > > +static void
> > > >  red_char_device_spicevmc_class_init(RedCharDeviceSpiceVmcClass
> > > > *klass)
> > > >  {
> > > > +GObjectClass *object_class = G_OBJECT_CLASS(klass);
> > > >  RedCharDeviceClass *char_dev_class =
> > > > RED_CHAR_DEVICE_CLASS(klass);
> > > > 
> > > > +object_class->dispose = red_char_device_spicevmc_dispose;
> > > > +
> > > >  char_dev_class->read_one_msg_from_device =
> > > >  

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

2016-11-02 Thread Frediano Ziglio 
> 
> Hi,
> 
> On Wed, Nov 02, 2016 at 04:34:47AM -0400, Frediano Ziglio wrote:
> > > 
> > > From: Jonathon Jongsma 
> > > 
> > > 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,
> > > SpiceCharDeviceInstance *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(>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.

> >
> > > +}
> > > +
> > > +static void
> > >  red_char_device_spicevmc_class_init(RedCharDeviceSpiceVmcClass *klass)
> > >  {
> > > +GObjectClass *object_class = G_OBJECT_CLASS(klass);
> > >  RedCharDeviceClass *char_dev_class = RED_CHAR_DEVICE_CLASS(klass);
> > >
> > > +object_class->dispose = red_char_device_spicevmc_dispose;
> > > +
> > >  char_dev_class->read_one_msg_from_device =
> > >  spicevmc_chardev_read_msg_from_dev;
> > >  char_dev_class->send_msg_to_client =
> > >  spicevmc_chardev_send_msg_to_client;
> > >  char_dev_class->send_tokens_to_client =
> > >  spicevmc_char_dev_send_tokens_to_client;
> >
> > Removing the double free from this patch will basically revert the
> > entire patch except the RED_CHAR_DEVICE_SPICEVMC_PRIVATE cleanup.
> >

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2016-11-02 Thread Victor Toso 
Hi,

On Wed, Nov 02, 2016 at 04:34:47AM -0400, Frediano Ziglio wrote:
> > 
> > From: Jonathon Jongsma 
> > 
> > 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,
> > SpiceCharDeviceInstance *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(>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.

>
> > +}
> > +
> > +static void
> >  red_char_device_spicevmc_class_init(RedCharDeviceSpiceVmcClass *klass)
> >  {
> > +GObjectClass *object_class = G_OBJECT_CLASS(klass);
> >  RedCharDeviceClass *char_dev_class = RED_CHAR_DEVICE_CLASS(klass);
> >
> > +object_class->dispose = red_char_device_spicevmc_dispose;
> > +
> >  char_dev_class->read_one_msg_from_device =
> >  spicevmc_chardev_read_msg_from_dev;
> >  char_dev_class->send_msg_to_client =
> >  spicevmc_chardev_send_msg_to_client;
> >  char_dev_class->send_tokens_to_client =
> >  spicevmc_char_dev_send_tokens_to_client;
>
> Removing the double free from this patch will basically revert the
> entire patch except the RED_CHAR_DEVICE_SPICEVMC_PRIVATE cleanup.
>
> Frediano
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2016-11-02 Thread Frediano Ziglio 
> 
> From: Jonathon Jongsma 
> 
> 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,
> SpiceCharDeviceInstance *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(>channel);

This call produce a double free.
The channel is already freed in spicevmc_device_disconnect.

> +}
> +
> +static void
>  red_char_device_spicevmc_class_init(RedCharDeviceSpiceVmcClass *klass)
>  {
> +GObjectClass *object_class = G_OBJECT_CLASS(klass);
>  RedCharDeviceClass *char_dev_class = RED_CHAR_DEVICE_CLASS(klass);
>  
> +object_class->dispose = red_char_device_spicevmc_dispose;
> +
>  char_dev_class->read_one_msg_from_device =
>  spicevmc_chardev_read_msg_from_dev;
>  char_dev_class->send_msg_to_client =
>  spicevmc_chardev_send_msg_to_client;
>  char_dev_class->send_tokens_to_client =
>  spicevmc_char_dev_send_tokens_to_client;

Removing the double free from this patch will basically revert the
entire patch except the RED_CHAR_DEVICE_SPICEVMC_PRIVATE cleanup.

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2016-11-02 Thread Frediano Ziglio 
From: Jonathon Jongsma 

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,
SpiceCharDeviceInstance *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(>channel);
+}
+
+static void
 red_char_device_spicevmc_class_init(RedCharDeviceSpiceVmcClass *klass)
 {
+GObjectClass *object_class = G_OBJECT_CLASS(klass);
 RedCharDeviceClass *char_dev_class = RED_CHAR_DEVICE_CLASS(klass);
 
+object_class->dispose = red_char_device_spicevmc_dispose;
+
 char_dev_class->read_one_msg_from_device = 
spicevmc_chardev_read_msg_from_dev;
 char_dev_class->send_msg_to_client = spicevmc_chardev_send_msg_to_client;
 char_dev_class->send_tokens_to_client = 
spicevmc_char_dev_send_tokens_to_client;
-- 
2.7.4

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel