Re: [PATCH v5 04/13] usbredir: Protect data accessed by asynchronous redirection flows

[Dmitry Fleytman <dmitry@xxxxxxxxxx>]

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

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

From: Kirill Moizik <kmoizik@xxxxxxxxxx>

This commit adds locking to ensure thread safety required
after start/stop redirection flows moved to separate threads.

Signed-off-by: Kirill Moizik <kmoizik@xxxxxxxxxx>
Signed-off-by: Dmitry Fleytman <dfleytma@xxxxxxxxxx>
---
src/channel-usbredir.c   |  6 ++++++
src/usb-device-manager.c | 12 ++++++++++--
2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
index 8cf3387..c88322a 100644
--- a/src/channel-usbredir.c
+++ b/src/channel-usbredir.c
@@ -660,12 +660,17 @@ static void usbredir_handle_msg(SpiceChannel *c,
SpiceMsgIn *in)
    priv->read_buf = buf;
    priv->read_buf_size = size;

+    g_mutex_lock(priv->flows_mutex);

so, here you use the direct call to g_mutex_lock(). But below, you use the
member function spice_usbredir_channel_lock() instead. I'd prefer to use one or
the other consistently.

Fixed in all patches.

    r = usbredirhost_read_guest_data(priv->host);
    if (r != 0) {
        SpiceUsbDevice *spice_device = priv->spice_device;
        gchar *desc;
        GError *err;

+        if (spice_device == NULL)
+        {
+            g_mutex_unlock(priv->flows_mutex);
+        }
        g_return_if_fail(spice_device != NULL);

it's a little odd to see an explicit null check and then a
g_return_if_fail(!NULL) immediately after. g_return_if_fail() should be used to
indicate programming errors (since it can be disabled by G_DISABLE_CHECKS). So
my feeling is that we should either handle it fully (by expecting that
spice_device can be NULL and calling 'return' explicitly), or we should just
assume it can only be NULL in the case of a programming error and therefore we
don't need to worry about unlocking the mutex since behavior is essentially
undefined at this point anyway. I think the first approach is probably safest.

Yes, g_return_if_fail(…) left here accidentally. Dropped.

        desc = spice_usb_device_get_description(spice_device, NULL);
@@ -703,6 +708,7 @@ static void usbredir_handle_msg(SpiceChannel *c,
SpiceMsgIn *in)

        g_error_free(err);
    }
+        g_mutex_unlock(priv->flows_mutex);
}

#endif /* USE_USBREDIR */
diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
index f4e48eb..4d376b6 100644
--- a/src/usb-device-manager.c
+++ b/src/usb-device-manager.c
@@ -1326,9 +1326,13 @@ static SpiceUsbredirChannel
*spice_usb_device_manager_get_channel_for_dev(

    for (i = 0; i < priv->channels->len; i++) {
        SpiceUsbredirChannel *channel = g_ptr_array_index(priv->channels, i);
+        spice_usbredir_channel_lock(channel);
        libusb_device *libdev = spice_usbredir_channel_get_device(channel);
-        if (spice_usb_manager_device_equal_libdev(manager, device, libdev))
+        if (spice_usb_manager_device_equal_libdev(manager, device, libdev)) {
+            spice_usbredir_channel_unlock(channel);
            return channel;
+        }
+        spice_usbredir_channel_unlock(channel);
    }
#endif
    return NULL;
@@ -1730,9 +1734,13 @@
spice_usb_device_manager_can_redirect_device(SpiceUsbDeviceManager  *self,
    /* Check if there are free channels */
    for (i = 0; i < priv->channels->len; i++) {
        SpiceUsbredirChannel *channel = g_ptr_array_index(priv->channels, i);
+        spice_usbredir_channel_lock(channel);

-        if (!spice_usbredir_channel_get_device(channel))
+        if (!spice_usbredir_channel_get_device(channel)){
+            spice_usbredir_channel_unlock(channel);
            break;
+        }
+        spice_usbredir_channel_unlock(channel);
    }
    if (i == priv->channels->len) {
        g_set_error_literal(err, SPICE_CLIENT_ERROR,
SPICE_CLIENT_ERROR_FAILED,

I suspect that you were probably asked to split this patch in a previous review,
but I find it a bit strange that we're introducing locking for data before we
actually introduce the multi-threaded access. It's OK, I guess, but at minimum
the commit log should indicate that threading changes are coming in future
commits.

Commit messages extended. Thanks.

Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>

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