Re: [PATCH v5 01/13] Usbredir channel: Introduce mutex for USB redirection

[Dmitry Fleytman <dmitry@xxxxxxxxxx>]

On 29 Feb 2016, at 13:16 PM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:

On 9 Feb 2016, at 17:14 PM, Jonathon Jongsma <jjongsma@xxxxxxxxxx> wrote:

On Thu, 2015-10-29 at 17:26 +0200, Dmitry Fleytman wrote:

From: Kirill Moizik <kmoizik@xxxxxxxxxx>

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@xxxxxxxxxx>
Signed-off-by: Dmitry Fleytman <dfleytma@xxxxxxxxxx>
---
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@xxxxxxxxxx>

Frediano

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel