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
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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel