[Spice-devel] [PATCH v5 03/13] GUdevClient: Do not process USB hotplug events while redirection is in progress

[Dmitry Fleytman]

> On 9 Feb 2016, at 24:56 AM, Jonathon Jongsma <jjongsma at redhat.com> wrote:
> 
> On Thu, 2015-10-29 at 17:27 +0200, Dmitry Fleytman wrote:
>> From: Kirill Moizik <kmoizik at redhat.com>
>> 
>> USB redirection flow on Windows includes a number of reset requests issued
>> to the port that hosts the device deing redirected.
>> 
>> Each port reset emulates device removal and reinsertion and produces
>> corresponding hotplug events and a number of device list updates on
>> different levels of USB stack and USB redirection engine.
>> 
>> As a result, queriyng USB device list performed by spice-gtk's hotplug
>> event handler may return inconsistent results if performed in parallel
>> to redirection flow.
>> 
>> This patch suppresses handling of USB hotplug events during redirection
>> and injects a simulated hotplug event after redirection completion
>> in order to properly process real device list changes in case they
>> happened during the redirection flow.
>> 
>> Signed-off-by: Kirill Moizik <kmoizik at redhat.com>
>> Signed-off-by: Dmitry Fleytman <dfleytma at redhat.com>
>> ---
>> src/win-usb-dev.c | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>> 
>> diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
>> index 60bc434..7364ee5 100644
>> --- a/src/win-usb-dev.c
>> +++ b/src/win-usb-dev.c
>> @@ -305,10 +305,18 @@ static void g_udev_client_set_property(GObject      
>> *gobject,
>> {
>>     GUdevClient *self = G_UDEV_CLIENT(gobject);
>>     GUdevClientPrivate *priv = self->priv;
>> +    gboolean old_val;
>> 
>>     switch (prop_id) {
>>     case PROP_REDIRECTING:
>> +        old_val = priv->redirecting;
>>         priv->redirecting = g_value_get_boolean(value);
>> +        if (old_val && !priv->redirecting) {
>> +            /* This is a redirection completion case.
>> +               Inject hotplug event in case we missed device changes
>> +               during redirection processing. */
>> +            handle_dev_change(self);
>> +        }
>>         break;
>>     default:
>>         G_OBJECT_WARN_INVALID_PROPERTY_ID(gobject, prop_id, pspec);
>> @@ -409,6 +417,15 @@ static void handle_dev_change(GUdevClient *self)
>>     GList *lit, *sit; /* iterators for long-list and short-list */
>>     GUdevDevice *ldev, *sdev; /* devices on long-list and short-list */
>> 
>> +    if (priv->redirecting == TRUE) {
>> +        /* On Windows, querying USB device list may return inconsistent
>> results
>> +           if performed in parallel to redirection flow.
>> +           A simulated hotplug event will be injected after redirection
>> +           completion in order to process real device list changes that may
>> +           had taken place during redirection process. */
>> +        return;
>> +    }
>> +
>>     dev_count = g_udev_client_list_devices(self, &now_devs, &err,
>>                                            __FUNCTION__);
>>     g_return_if_fail(dev_count >= 0);
> 
> 
> It seems to me that this change might potentially cause us to miss hotplug
> events. Notice that the comment at the beginning of handle_dev_change() says:
> 
> /* Assumes each event stands for a single device change (at most) */
> 
> This becomes relevant because the implementation of this function asumes that if
> the length of the device list is unchanged from the last time it was executed,
> no change occurred.  However, now that we've added an early return here, I
> believe there's a possibility for a race condition:
> 
> - Start a new redirection
> - a new device is plugged in. Since priv->redirecting == TRUE, we return early
> and don't handle that change
> - a different device is unplugged. Since priv->redirecting is still TRUE, we
> return early again and don't handle the change
> - redirection completes and priv->redirecting is set to FALSE. We also trigger
> a new call to handle_dev_change(). However, since there was an added device and
> a removed device, the total number of devices is the same, and it matches the
> length of the cached device list. Therefore, no signal is emitted. But we should
> emit one remove signal and one add signal.
> 
> Maybe this scenario is not a realistic possibility, I don't know. 


Yes, the scenario is real.

This problem existed even before this patch. For example, when a USB hub with a few devices inserted is plugged or unplugged, only one device state change is processed.
I’m adding additional patch in front of series to address this problem.


> 
> Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com <mailto:jjongsma at redhat.com>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160228/a7f4aa74/attachment-0001.html>