Re: [Spice-devel] [PATCH 1/3] spicevmc: store channel in char device

[Frediano Ziglio]

> Store the channel in the char device object, and unref it in dispose.
> Previously, we were just creating a channel and potentially allowing it
> to leak.  There may be better long-term solutions, but this works for
> now.
> ---
> Changes from last version:
>  - set channel to NULL in spicevmc_device_disconnect() to avoid the
>  double-free
>mentioned by Frediano
> 

I was considering 1/3 and 3/3 patches. Would make sense to
have just the spicevmc_device_disconnect (and similar smartcard) to just
unref the device however g_clear_object on chardev is not enough:
- device destruction should call reds_unregister_channel to prevent
  possible future RedChannelClient to connect to the channel;
- destruction should call red_channel_destroy and not just g_object_unref
  (called by g_clear_object) to close current clients (no reasons to have
  client to handle closed devices).
Note that the above cannot/shouldn't be moved to the channel destructor
as there will be external (like from RedChannelClient) reference to
the channel that will prevent these stuff to be executed.

Possibly there could be a red_char_device_clear_channel(RedChannel **p_channel)
utility that check NULL pointer and do all these stuff to be called
by Smartcard/SpiceVmc instead of g_clear_pointer.

>  server/spicevmc.c | 46 ++
>  1 file changed, 30 insertions(+), 16 deletions(-)
> 
> diff --git a/server/spicevmc.c b/server/spicevmc.c
> index 7067bc7..8e23248 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,28 +853,33 @@ 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. */
>  void spicevmc_device_disconnect(RedsState *reds, SpiceCharDeviceInstance
>  *sin)
>  {
> -RedVmcChannel *state;
> +RedVmcChannel *channel;
> +RedCharDeviceSpiceVmc *vmc = RED_CHAR_DEVICE_SPICEVMC(sin->st);
>  
> -/* FIXME */
> -state = (RedVmcChannel
> *)red_char_device_opaque_get((RedCharDevice*)sin->st);
> +channel = vmc->channel;
> +vmc->channel = NULL;
>  
> -red_char_device_write_buffer_release(state->chardev,
> >recv_from_client_buf);
> +red_char_device_write_buffer_release(channel->chardev,
> >recv_from_client_buf);
>  /* FIXME */
> -red_char_device_destroy((RedCharDevice*)sin->st);
> -state->chardev = NULL;
> +red_char_device_destroy(RED_CHAR_DEVICE(vmc));
> +channel->chardev = NULL;
>  sin->st = NULL;
>  
> -reds_unregister_channel(reds, RED_CHANNEL(state));
> -free(state->pipe_item);
> -red_channel_destroy(RED_CHANNEL(state));
> +reds_unregister_channel(reds, RED_CHANNEL(channel));
> +free(channel->pipe_item);
> +red_channel_destroy(RED_CHANNEL(channel));
>  }
>  
>  SPICE_GNUC_VISIBLE void spice_server_port_event(SpiceCharDeviceInstance
>  *sin, uint8_t event)
> @@ -904,10 +907,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
> 

Re: [Spice-devel] [PATCH 1/3] spicevmc: store channel in char device

[Pavel Grunt]

On Wed, 2016-11-02 at 16:25 -0500, Jonathon Jongsma wrote:
> Store the channel in the char device object, and unref it in
> dispose.
> Previously, we were just creating a channel and potentially allowing
> it
> to leak.  There may be better long-term solutions, but this works
> for
> now.
Acked-by: Pavel Grunt 
> ---
> Changes from last version:
>  - set channel to NULL in spicevmc_device_disconnect() to avoid the
> double-free
>    mentioned by Frediano
> 
>  server/spicevmc.c | 46 ++
> 
>  1 file changed, 30 insertions(+), 16 deletions(-)
> 
> diff --git a/server/spicevmc.c b/server/spicevmc.c
> index 7067bc7..8e23248 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,28 +853,33 @@ 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. */
>  void spicevmc_device_disconnect(RedsState *reds,
> SpiceCharDeviceInstance *sin)
>  {
> -RedVmcChannel *state;
> +RedVmcChannel *channel;
> +RedCharDeviceSpiceVmc *vmc = RED_CHAR_DEVICE_SPICEVMC(sin->st);
>  
> -/* FIXME */
> -state = (RedVmcChannel
> *)red_char_device_opaque_get((RedCharDevice*)sin->st);
> +channel = vmc->channel;
> +vmc->channel = NULL;
>  
> -red_char_device_write_buffer_release(state->chardev, 
> >recv_from_client_buf);
> +red_char_device_write_buffer_release(channel->chardev,
> >recv_from_client_buf);
>  /* FIXME */
> -red_char_device_destroy((RedCharDevice*)sin->st);
> -state->chardev = NULL;
> +red_char_device_destroy(RED_CHAR_DEVICE(vmc));
> +channel->chardev = NULL;
>  sin->st = NULL;
>  
> -reds_unregister_channel(reds, RED_CHANNEL(state));
> -free(state->pipe_item);
> -red_channel_destroy(RED_CHANNEL(state));
> +reds_unregister_channel(reds, RED_CHANNEL(channel));
> +free(channel->pipe_item);
> +red_channel_destroy(RED_CHANNEL(channel));
>  }
>  
>  SPICE_GNUC_VISIBLE void
> spice_server_port_event(SpiceCharDeviceInstance *sin, uint8_t event)
> @@ -904,10 +907,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;
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH 1/3] spicevmc: store channel in char device

[Jonathon Jongsma]

Store the channel in the char device object, and unref it in dispose.
Previously, we were just creating a channel and potentially allowing it
to leak.  There may be better long-term solutions, but this works for
now.
---
Changes from last version:
 - set channel to NULL in spicevmc_device_disconnect() to avoid the double-free
   mentioned by Frediano

 server/spicevmc.c | 46 ++
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/server/spicevmc.c b/server/spicevmc.c
index 7067bc7..8e23248 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,28 +853,33 @@ 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. */
 void spicevmc_device_disconnect(RedsState *reds, SpiceCharDeviceInstance *sin)
 {
-RedVmcChannel *state;
+RedVmcChannel *channel;
+RedCharDeviceSpiceVmc *vmc = RED_CHAR_DEVICE_SPICEVMC(sin->st);
 
-/* FIXME */
-state = (RedVmcChannel 
*)red_char_device_opaque_get((RedCharDevice*)sin->st);
+channel = vmc->channel;
+vmc->channel = NULL;
 
-red_char_device_write_buffer_release(state->chardev, 
>recv_from_client_buf);
+red_char_device_write_buffer_release(channel->chardev, 
>recv_from_client_buf);
 /* FIXME */
-red_char_device_destroy((RedCharDevice*)sin->st);
-state->chardev = NULL;
+red_char_device_destroy(RED_CHAR_DEVICE(vmc));
+channel->chardev = NULL;
 sin->st = NULL;
 
-reds_unregister_channel(reds, RED_CHANNEL(state));
-free(state->pipe_item);
-red_channel_destroy(RED_CHANNEL(state));
+reds_unregister_channel(reds, RED_CHANNEL(channel));
+free(channel->pipe_item);
+red_channel_destroy(RED_CHANNEL(channel));
 }
 
 SPICE_GNUC_VISIBLE void spice_server_port_event(SpiceCharDeviceInstance *sin, 
uint8_t event)
@@ -904,10 +907,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