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

[Dmitry Fleytman]

> On 29 Feb 2016, at 13:16 PM, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
>> 
>> 
>>> 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) ?

Hi Frediano,

Right, this is an artefact from previous versions of patches.

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

We did not intend to drop support for older Glib versions so it is a bug in our code that should be fixed.
Do you want me to drop support of pre-2.32.0 GLib instead?

Thanks,
Dmitry

> 
>>>>    /* 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160306/e60e5f40/attachment-0001.html>