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

[Jonathon Jongsma]

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.

>      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 at redhat.com>