| Hi Jonathon,
Sure, I’ll test. I tried to build your branch but it requires newer spice-protocol:
configure: error: Package requirements (spice-protocol >= 0.12.11) were not met: Requested 'spice-protocol >= 0.12.11' but version of spice-protocol is 0.12.10
Do you have any idea where one can get the corresponding mingw package?
Thanks, Dmitry
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
|