<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Hi Jonathon,<div class=""> </div><div class="">Sure, I’ll test.</div><div class="">I tried to build your branch but it requires newer spice-protocol:</div><div class=""> </div><div class=""><div class="">configure: error: Package requirements (spice-protocol >= 0.12.11) were not met:</div><div class="">Requested 'spice-protocol >= 0.12.11' but version of spice-protocol is 0.12.10</div><div class=""> </div><div class="">Do you have any idea where one can get the corresponding mingw package?</div><div class=""> </div><div class="">Thanks,</div><div class="">Dmitry</div><div class=""> </div><div><blockquote type="cite" class=""><div class="">On 17 Mar 2016, at 24:04 AM, Jonathon Jongsma <<a href="mailto:jjongsma@redhat.com" class="">jjongsma@redhat.com</a>> wrote:</div> <div class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">On Wed, 2016-03-16 at 23:01 +0100, Fabiano Fidêncio wrote:<br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">On Wed, Mar 16, 2016 at 10:54 PM, Jonathon Jongsma <<a href="mailto:jjongsma@redhat.com" class="">jjongsma@redhat.com</a>> wrote: <blockquote type="cite" class="">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 </blockquote> 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? </blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Absolutely. my rebased branch is available here: <br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><a href="https://cgit.freedesktop.org/~jjongsma/spice-gtk/log/?h=usb-async" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">https://cgit.freedesktop.org/~jjongsma/spice-gtk/log/?h=usb-async</a><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""> <blockquote type="cite" class=""> On Wed, 2016-03-16 at 16:44 -0500, Jonathon Jongsma wrote: <blockquote type="cite" class="">On Wed, 2016-03-16 at 10:41 -0500, Jonathon Jongsma wrote: <blockquote type="cite" class="">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 </blockquote> ... 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. <blockquote type="cite" class=""> On Wed, 2016-03-16 at 10:16 -0500, Jonathon Jongsma wrote: <blockquote type="cite" class="">On Wed, 2016-03-16 at 11:27 +0100, Christophe Fergeau wrote: <blockquote type="cite" class="">Hey, On Tue, Mar 15, 2016 at 02:31:03PM -0500, Jonathon Jongsma wrote: <blockquote type="cite" class="">+#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); </blockquote> 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? </blockquote> Good catch. That was unintentional. <blockquote type="cite" class="">Christophe </blockquote>_______________________________________________ Spice-devel mailing list <a href="mailto:Spice-devel@lists.freedesktop.org" class="">Spice-devel@lists.freedesktop.org</a> https://lists.freedesktop.org/mailman/listinfo/spice-devel </blockquote>_______________________________________________ Spice-devel mailing list <a href="mailto:Spice-devel@lists.freedesktop.org" class="">Spice-devel@lists.freedesktop.org</a> https://lists.freedesktop.org/mailman/listinfo/spice-devel </blockquote>_______________________________________________ Spice-devel mailing list <a href="mailto:Spice-devel@lists.freedesktop.org" class="">Spice-devel@lists.freedesktop.org</a> https://lists.freedesktop.org/mailman/listinfo/spice-devel </blockquote>_______________________________________________ Spice-devel mailing list <a href="mailto:Spice-devel@lists.freedesktop.org" class="">Spice-devel@lists.freedesktop.org</a> https://lists.freedesktop.org/mailman/listinfo/spice-devel</blockquote></blockquote></div></blockquote></div> </div></body></html>