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 hotplugevents. 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 ifthe 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, Ibelieve there's a possibility for a race condition:- Start a new redirection- a new device is plugged in. Since priv->redirecting == TRUE, we return earlyand don't handle that change- a different device is unplugged. Since priv->redirecting is still TRUE, wereturn early again and don't handle the change- redirection completes and priv->redirecting is set to FALSE. We also triggera new call to handle_dev_change(). However, since there was an added device anda removed device, the total number of devices is the same, and it matches thelength of the cached device list. Therefore, no signal is emitted. But we shouldemit 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.
|