On Wed, Mar 16, 2016 at 10:54 PM, Jonathon Jongsma <jjongsma@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx
>> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>> > _______________________________________________
>> > 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
> _______________________________________________
> 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