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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 9 Feb 2016, at 24:56 AM, Jonathon Jongsma <jjongsma@xxxxxxxxxx> wrote:

On Thu, 2015-10-29 at 17:27 +0200, Dmitry Fleytman wrote:
From: Kirill Moizik <kmoizik@xxxxxxxxxx>

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@xxxxxxxxxx>
Signed-off-by: Dmitry Fleytman <dfleytma@xxxxxxxxxx>
---
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@xxxxxxxxxx>

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]