[Spice-devel] [PATCH v7 06/14] usbredir: Spawn a different thread for device redirection flow

[Jonathon Jongsma]

Updated proposal:

--- a/src/channel-usbredir.c
+++ b/src/channel-usbredir.c
@@ -368,16 +368,19 @@ _open_device_async_cb(GTask *task,
     spice_usbredir_channel_lock(channel);
 
     if (!spice_usbredir_channel_open_device(channel, &err)) {
-        g_task_return_error(task, err);
         libusb_unref_device(priv->device);
         priv->device = NULL;
         g_boxed_free(spice_usb_device_get_type(), priv->spice_device);
         priv->spice_device = NULL;
-    } else {
-        g_task_return_boolean(task, TRUE);
     }
 
     spice_usbredir_channel_unlock(channel);
+
+    if (err) {
+        g_task_return_error(task, err);
+    } else {
+        g_task_return_boolean(task, TRUE);
+    }
 }
 #endif
 


On Wed, 2016-03-16 at 16:44 -0500, Jonathon Jongsma wrote:
> On Wed, 2016-03-16 at 10:41 -0500, Jonathon Jongsma wrote:
> > So, after adding the g_task_return_boolean back to _open_device_async_cb(),
> > I
> > noticed that since g_task_return_error() can potentially complete the task
> > immediately (rather than in an idle handler done previously), we may be
> > executing the GAsyncReadyCallback while the channel is locked. Currently, I
> > don't think this causes any problems, but if the callback did anything that
> > required locking the channel's mutex, that could result in a deadlock. So
> > here's
> > an additional proposed patch to unlock before completing the task. If this
> > change is ACKed, I'd probably squash it into this patch.
> > 
> > ------------
> > diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> > index cd2ff4f..aa8e943 100644
> > --- a/src/channel-usbredir.c
> > +++ b/src/channel-usbredir.c
> > @@ -367,17 +367,19 @@ _open_device_async_cb(GTask *task,
> >  
> >      spice_usbredir_channel_lock(channel);
> >  
> > -    if (!spice_usbredir_channel_open_device(channel, &err)) {
> > +    spice_usbredir_channel_open_device(channel, &err);
> > +    libusb_unref_device(priv->device);
> > +    priv->device = NULL;
> > +    g_boxed_free(spice_usb_device_get_type(), priv->spice_device);
> > +    priv->spice_device = NULL;
> > +
> > +    spice_usbredir_channel_unlock(channel);
> > +
> > +    if (err) {
> >          g_task_return_error(task, err);
> > -        libusb_unref_device(priv->device);
> > -        priv->device = NULL;
> > -        g_boxed_free(spice_usb_device_get_type(), priv->spice_device);
> > -        priv->spice_device = NULL;
> >      } else {
> >          g_task_return_boolean(task, TRUE);
> >      }
> > -
> > -    spice_usbredir_channel_unlock(channel);
> >  }
> >  #endif
> >  
> 
> ... And Fabiano pointed out that this patch is incorrect since it frees the
> device even if it was successfully redirected. That's clearly wrong. New patch
> coming.
> 
> 
> > 
> > 
> > On Wed, 2016-03-16 at 10:16 -0500, Jonathon Jongsma wrote:
> > > On Wed, 2016-03-16 at 11:27 +0100, Christophe Fergeau wrote:
> > > > Hey,
> > > > 
> > > > On Tue, Mar 15, 2016 at 02:31:03PM -0500, Jonathon Jongsma wrote:
> > > > > +#ifndef USE_POLKIT
> > > > > +static void
> > > > > +_open_device_async_cb(GTask *task,
> > > > > +                      gpointer object,
> > > > > +                      gpointer task_data,
> > > > > +                      GCancellable *cancellable)
> > > > > +{
> > > > > +    GError *err = NULL;
> > > > > +    SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(object);
> > > > > +    SpiceUsbredirChannelPrivate *priv = channel->priv;
> > > > > +
> > > > > +    spice_usbredir_channel_lock(channel);
> > > > > +
> > > > > +    if (!spice_usbredir_channel_open_device(channel, &err)) {
> > > > > +        g_task_return_error(task, err);
> > > > > +        libusb_unref_device(priv->device);
> > > > > +        priv->device = NULL;
> > > > > +        g_boxed_free(spice_usb_device_get_type(), priv
> > > > > ->spice_device);
> > > > > +        priv->spice_device = NULL;
> > > > > +    }
> > > > > +
> > > > > +    spice_usbredir_channel_unlock(channel);
> > > > > +}
> > > > > +#endif
> > > > > +
> > > > >  G_GNUC_INTERNAL
> > > > >  void spice_usbredir_channel_connect_device_async(
> > > > >                                            SpiceUsbredirChannel
> > > > > *channel,
> > > > > @@ -331,9 +356,6 @@ void spice_usbredir_channel_connect_device_async(
> > > > >  {
> > > > >      SpiceUsbredirChannelPrivate *priv = channel->priv;
> > > > >      GTask *task;
> > > > > -#ifndef USE_POLKIT
> > > > > -    GError *err = NULL;
> > > > > -#endif
> > > > >  
> > > > >      g_return_if_fail(SPICE_IS_USBREDIR_CHANNEL(channel));
> > > > >      g_return_if_fail(device != NULL);
> > > > > @@ -376,15 +398,7 @@ void spice_usbredir_channel_connect_device_async(
> > > > >                                          channel);
> > > > >      return;
> > > > >  #else
> > > > > -    if (!spice_usbredir_channel_open_device(channel, &err)) {
> > > > > -        g_task_return_error(task, err);
> > > > > -        libusb_unref_device(priv->device);
> > > > > -        priv->device = NULL;
> > > > > -        g_boxed_free(spice_usb_device_get_type(), priv
> > > > > ->spice_device);
> > > > > -        priv->spice_device = NULL;
> > > > > -    } else {
> > > > > -        g_task_return_boolean(task, TRUE);
> > > > 
> > > > Only looked at the diff, not at the full code, but this
> > > > g_task_return_boolean(task, TRUE); is gone from the threaded version, is
> > > > this intentional?
> > > > 
> > > 
> > > Good catch. That was unintentional.
> > > 
> > > 
> > > > Christophe
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel