[Spice-devel] [PATCH 3/3] Smartcard: store channel in device

[Frediano Ziglio]

> 
> Commit 542bc478 removed the global smartcard channel, but never stored the
> newly-created channel anywhere. To avoid leaking the channel, store the
> channel
> as a private member of the device it is associated with. It will be unreffed
> when its associated device is destroyed.
> ---
>  server/smartcard.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/server/smartcard.c b/server/smartcard.c
> index 5b18abe..bc1ada4 100644
> --- a/server/smartcard.c
> +++ b/server/smartcard.c
> @@ -115,6 +115,7 @@ struct RedCharDeviceSmartcardPrivate {
>  
>      SmartCardChannelClient    *scc; // client providing the remote card
>      int                  reader_added; // has reader_add been sent to the
>      device
> +    RedSmartcardChannel *channel;
>  };
>  
>  typedef struct RedMsgItem {
> @@ -136,7 +137,6 @@ static int smartcard_char_device_add_to_readers(RedsState
> *reds, SpiceCharDevice
>  static RedMsgItem *smartcard_char_device_on_message_from_device(
>      RedCharDeviceSmartcard *dev, VSCMsgHeader *header);
>  static RedCharDeviceSmartcard *smartcard_device_new(RedsState *reds,
>  SpiceCharDeviceInstance *sin);
> -static void smartcard_init(RedsState *reds);
>  
>  static void smartcard_read_buf_prepare(RedCharDeviceSmartcard *dev,
>  VSCMsgHeader *vheader)
>  {
> @@ -256,7 +256,7 @@ static int smartcard_char_device_add_to_readers(RedsState
> *reds, SpiceCharDevice
>      }
>      dev->priv->reader_id = g_smartcard_readers.num;
>      g_smartcard_readers.sin[g_smartcard_readers.num++] = char_device;
> -    smartcard_init(reds);
> +    dev->priv->channel = red_smartcard_channel_new(reds);
>      return 0;
>  }
>  
> @@ -602,13 +602,15 @@
> red_smartcard_channel_class_init(RedSmartcardChannelClass *klass)
>  
>  }
>  
> -static void smartcard_init(RedsState *reds)
> +static void
> +red_char_device_smartcard_dispose(GObject *object)
>  {
> -    spice_assert(!reds_find_channel(reds, SPICE_CHANNEL_SMARTCARD, 0));

Here you removed the check for singleton. Are we going to support more smartcards?

> +    RedCharDeviceSmartcard *self = RED_CHAR_DEVICE_SMARTCARD(object);
>  
> -    red_smartcard_channel_new(reds);
> -}
> +    g_clear_object(&self->priv->channel);
>  
> +    G_OBJECT_CLASS(red_char_device_smartcard_parent_class)->dispose(object);
> +}
>  
>  static void
>  red_char_device_smartcard_finalize(GObject *object)
> @@ -628,6 +630,7 @@
> red_char_device_smartcard_class_init(RedCharDeviceSmartcardClass *klass)
>  
>      g_type_class_add_private(klass, sizeof (RedCharDeviceSmartcardPrivate));
>  
> +    object_class->dispose = red_char_device_smartcard_dispose;
>      object_class->finalize = red_char_device_smartcard_finalize;
>  
>      char_dev_class->read_one_msg_from_device =
>      smartcard_read_msg_from_device;

Looks like this patch is only partial.
I think there is a missing part similar to spicevmc_device_disconnect.
Who is going to unregister the channel?
Note that is possible that channel will continue to stay alive even when
the device is deleted as long as clients are attached to the channel
(this is prevented by red_channel_destroy call for SpiceVmc).
I agree on the strong reference from device to channel; application asked
for a SpiceCharDeviceInstance and will release such device.

Frediano