[Spice-devel] [PATCH v2 6/6] Make usb redirection stop and surprise removal flows asynchronous

[Christophe Fergeau]

On Mon, Jul 06, 2015 at 08:59:06PM +0300, Kirill Moizik wrote:
> we will spawn a separate thread to disconnect usb device, when finish we will update widget to allow further
> usb redirection operations
> 1) expose spice_usbredir_channel_connect_device_async function for asynchronous disconnect
> 2) threads synchronization

Honestly, moving non-trivial existing functions from running in the main
thread to running in a separate thread with very little locking added,
and no high level explanation as to why everything will be fine and
nothing can be racy is very scary... One example below:

> 
> Signed-off-by: Kirill Moizik <kmoizik at redhat.com>
> ---
>  src/channel-usbredir-priv.h |  8 ++++-
>  src/channel-usbredir.c      | 79 ++++++++++++++++++++++++++++++++++++++-------
>  src/map-file                |  1 +
>  src/usb-device-manager.c    | 62 ++++++++++++++++++++++++++++++++++-
>  src/usb-device-manager.h    |  8 +++++
>  src/usb-device-widget.c     | 19 +++++++++--
>  6 files changed, 161 insertions(+), 16 deletions(-)
> 
> diff --git a/src/channel-usbredir-priv.h b/src/channel-usbredir-priv.h
> index 2c4c6f7..c3ff158 100644
> --- a/src/channel-usbredir-priv.h
> +++ b/src/channel-usbredir-priv.h
> @@ -33,6 +33,10 @@ G_BEGIN_DECLS
>  void spice_usbredir_channel_set_context(SpiceUsbredirChannel *channel,
>                                          libusb_context       *context);
>  
> +void spice_usbredir_channel_disconnect_device_async(SpiceUsbredirChannel *channel,
> +                                                    GSimpleAsyncResult *simple,
> +                                                    GCancellable *cancellable);
> +
>  /* Note the context must be set, and the channel must be brought up
>     (through spice_channel_connect()), before calling this. */
>  void spice_usbredir_channel_connect_device_async(
> @@ -47,7 +51,9 @@ gboolean spice_usbredir_channel_connect_device_finish(
>                                          GAsyncResult         *res,
>                                          GError              **err);
>  
> -void spice_usbredir_channel_disconnect_device(SpiceUsbredirChannel *channel);
> +void spice_usbredir_channel_disconnect_device(GSimpleAsyncResult *simple,
> +                                              GObject *object,
> +                                              GCancellable *cancellable);
>  
>  libusb_device *spice_usbredir_channel_get_device(SpiceUsbredirChannel *channel);
>  
> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> index e7d5949..1e78350 100644
> --- a/src/channel-usbredir.c
> +++ b/src/channel-usbredir.c
> @@ -79,6 +79,7 @@ struct _SpiceUsbredirChannelPrivate {
>      GSimpleAsyncResult *result;
>      SpiceUsbAclHelper *acl_helper;
>  #endif
> +    void* redirect_mutex;

usbredir_alloc_lock returns a void* only because that's what usbredir
expects. Let's just cast it to GMutex * here, and use
g_mutex_lock/unlock.

[snip]

> @@ -440,7 +491,9 @@ void spice_usbredir_channel_disconnect_device(SpiceUsbredirChannel *channel)
>                      spice_usb_device_manager_get(session, NULL));
>          }
>          /* This also closes the libusb handle we passed from open_device */
> +        usbredir_lock_lock(channel->priv->redirect_mutex);
>          usbredirhost_set_device(priv->host, NULL);
> +        usbredir_unlock_lock(channel->priv->redirect_mutex);
>          libusb_unref_device(priv->device);
>          priv->device = NULL;
>          g_boxed_free(spice_usb_device_get_type(), priv->spice_device);
>          priv->spice_device = NULL;
>
> @@ -652,7 +705,9 @@ static void usbredir_handle_msg(SpiceChannel *c, SpiceMsgIn *in)
>      priv->read_buf = buf;
>      priv->read_buf_size = size;
>  
> +    usbredir_lock_lock(priv->redirect_mutex);
>      r = usbredirhost_read_guest_data(priv->host);
> +    usbredir_unlock_lock(priv->redirect_mutex);
>      if (r != 0) {
>          SpiceUsbDevice *spice_device = priv->spice_device;
>          gchar *desc;

>          GError *err;

>          g_return_if_fail(spice_device != NULL);

The 2 hunks I kept are the only 2 places loking/unlocking
redirect_mutex, I assume this means they can race with each other.
If that's true, then we can get in a situation where 2nd hunk code runs
till
      if (r != 0) {
with r != 0, then 1st hunk runs and sets priv->spice_device to NULL,
then 2nd hunk resumes, and we trigger the g_return_if_fail() (which
usually is used to indicate a programming error)

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150707/0af9e509/attachment.sig>