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

[Frediano Ziglio]

> 
> 
> > On 9 Feb 2016, at 17:14 PM, Jonathon Jongsma <jjongsma at redhat.com> wrote:
> > 
> > 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;

Why not just GMutext flows_mutex; (or whichever name) ?

> >> };
> >> 
> >> 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
> >> 

Looks like mutex allocation in GLib changed a bit, in another section of
code you have

static void *usbredir_alloc_lock(void) {
#if GLIB_CHECK_VERSION(2,32,0)
    GMutex *mutex;

    mutex = g_new0(GMutex, 1);
    g_mutex_init(mutex);

    return mutex;
#else
    return g_mutex_new();
#endif
}

if now we just support first version (as your g_mutex_init call seems to
suggest) why not dropping old compatibility ?

> >>     /* 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.
> 
> Indeed the name may be better. Renaming device_connect_mutex.
> 
> 
> > 
> > Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
> 

Frediano