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

[Jonathon Jongsma]

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. 

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