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.
> 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.
>
> 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.
Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel