[Spice-devel] [PATCH v5 01/13] Usbredir channel: Introduce mutex for USB redirection

[Jonathon Jongsma]

On Thu, 2015-10-29 at 17:26 +0200, Dmitry Fleytman wrote:
> From: Kirill Moizik <kmoizik at redhat.com>
> 
> This commit introduces channel mutex to allow usage of
> channel objects in mutithreaded environments.
> 
> This mutex will be used to protect non thread safe
> usbredir functions and data structures.
> 
> Signed-off-by: Kirill Moizik <kmoizik at redhat.com>
> Signed-off-by: Dmitry Fleytman <dfleytma at redhat.com>
> ---
>  src/channel-usbredir-priv.h |  4 ++++
>  src/channel-usbredir.c      | 19 +++++++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/src/channel-usbredir-priv.h b/src/channel-usbredir-priv.h
> index 2c4c6f7..c987474 100644
> --- a/src/channel-usbredir-priv.h
> +++ b/src/channel-usbredir-priv.h
> @@ -51,6 +51,10 @@ void
> spice_usbredir_channel_disconnect_device(SpiceUsbredirChannel *channel);
>  
>  libusb_device *spice_usbredir_channel_get_device(SpiceUsbredirChannel
> *channel);
>  
> +void spice_usbredir_channel_lock(SpiceUsbredirChannel *channel);
> +
> +void spice_usbredir_channel_unlock(SpiceUsbredirChannel *channel);
> +
>  void spice_usbredir_channel_get_guest_filter(
>                            SpiceUsbredirChannel               *channel,
>                            const struct usbredirfilter_rule  **rules_ret,
> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> index c236ee7..8cf3387 100644
> --- a/src/channel-usbredir.c
> +++ b/src/channel-usbredir.c
> @@ -79,6 +79,7 @@ struct _SpiceUsbredirChannelPrivate {
>      GSimpleAsyncResult *result;
>      SpiceUsbAclHelper *acl_helper;
>  #endif
> +    GMutex *flows_mutex;
>  };
>  
>  static void channel_set_handlers(SpiceChannelClass *klass);
> @@ -107,6 +108,8 @@ static void
> spice_usbredir_channel_init(SpiceUsbredirChannel *channel)
>  {
>  #ifdef USE_USBREDIR
>      channel->priv = SPICE_USBREDIR_CHANNEL_GET_PRIVATE(channel);
> +    channel->priv->flows_mutex = g_new0(GMutex, 1);
> +    g_mutex_init(channel->priv->flows_mutex);
>  #endif
>  }
>  
> @@ -182,6 +185,10 @@ static void spice_usbredir_channel_finalize(GObject *obj)
>  
>      if (channel->priv->host)
>          usbredirhost_close(channel->priv->host);
> +#ifdef USE_USBREDIR
> +    g_mutex_clear(channel->priv->flows_mutex);
> +    g_free(channel->priv->flows_mutex);
> +#endif
>  
>      /* Chain up to the parent class */
>      if (G_OBJECT_CLASS(spice_usbredir_channel_parent_class)->finalize)
> @@ -561,6 +568,18 @@ static void *usbredir_alloc_lock(void) {
>  #endif
>  }
>  
> +void spice_usbredir_channel_lock(SpiceUsbredirChannel *channel)
> +{
> +    g_return_if_fail(SPICE_IS_USBREDIR_CHANNEL(channel));
> +    g_mutex_lock(channel->priv->flows_mutex);
> +}
> +
> +void spice_usbredir_channel_unlock(SpiceUsbredirChannel *channel)
> +{
> +  g_return_if_fail(SPICE_IS_USBREDIR_CHANNEL(channel));
> +  g_mutex_unlock(channel->priv->flows_mutex);
> +}
> +
>  static void usbredir_lock_lock(void *user_data) {
>      GMutex *mutex = user_data;
>  



The code itself looks fine, but again, it's hard to judge the design or
necessity of the mutex without seeing how it's used. I'll come back to this
after I've reviewed the rest of the patches. Also, why did you choose the name
"flows_mutex"?  The term "flow" is not used elsewhere in this file as far as I
can tell.

Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>