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

[Dmitry Fleytman]

> On 9 Feb 2016, at 18:40 PM, Jonathon Jongsma <jjongsma at redhat.com> wrote:
> 
> On Thu, 2015-10-29 at 17:27 +0200, Dmitry Fleytman wrote:
>> From: Kirill Moizik <kmoizik at redhat.com>
>> 
>> This commit adds locking to ensure thread safety required
>> after start/stop redirection flows moved to separate threads.
>> 
>> Signed-off-by: Kirill Moizik <kmoizik at redhat.com>
>> Signed-off-by: Dmitry Fleytman <dfleytma at redhat.com>
>> ---
>> 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 at redhat.com <mailto:jjongsma at redhat.com>>

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