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

[Kirill Moizik]

On Tue, Jul 7, 2015 at 3:51 PM, Christophe Fergeau <cfergeau at redhat.com>
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 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.
>

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150707/515bc252/attachment-0001.html>