[Spice-devel] [PATCH v7 06/14] usbredir: Spawn a different thread for device redirection flow
Dmitry Fleytman
dmitry at daynix.com
Sun Mar 20 11:19:16 UTC 2016
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 17 Mar 2016, at 24:04 AM, Jonathon Jongsma <jjongsma at redhat.com> wrote:
>
> 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 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?
>
>
> Absolutely. my rebased branch is available here:
> https://cgit.freedesktop.org/~jjongsma/spice-gtk/log/?h=usb-async <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 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160320/075eb02e/attachment.html>
More information about the Spice-devel
mailing list