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

[Fabiano Fidêncio]

On Wed, Mar 16, 2016 at 10:54 PM, Jonathon Jongsma <jjongsma at redhat.com> wrote:
> 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

ACK from my side. But I really would like to have Dmitry doing a last
round of tests on his series, this time with this GTask changes
applied.
Does it sound reasonable?

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