On Wed, 2016-03-16 at 23:01 +0100, Fabiano Fidêncio wrote:
> 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?
Absolutely. my rebased branch is available here:
https://cgit.freedesktop.org/~jjongsma/spice-gtk/log/?h=usb-async
>
> >
> >
> >
> > 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