Re: [PATCH v2 6/6] Make usb redirection stop and surprise removal flows asynchronous

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On Tue, Jul 7, 2015 at 3:51 PM, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote:
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:

I understand, I new to this project so i can't give you high level explanation why there is no races. I am aware  there could be races, so i spent time trying to find it. I tried few Os's on different hardware,  with and without spice/gtk debugging flags to change timing of flows to see if it will fail, but l it worked for me. Also i was trying to add another locks in suspicious places, but it led to deadlocks.  Do you have any ideas how to prove quality of those patches ?
 

>
> Signed-off-by: Kirill Moizik <kmoizik@xxxxxxxxxx>
> ---
>  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.

Ok, will fix it 

[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)

we can expand critical section in this case, is it bad idea?

Christophe

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

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]