> On 9 Feb 2016, at 24:56 AM, Jonathon Jongsma <[email protected]> wrote: > > On Thu, 2015-10-29 at 17:27 +0200, Dmitry Fleytman wrote: >> From: Kirill Moizik <[email protected]> >> >> 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 <[email protected]> >> Signed-off-by: Dmitry Fleytman <[email protected]> >> --- >> 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 <[email protected] > <mailto:[email protected]>>
_______________________________________________ Spice-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/spice-devel
