[Spice-devel] [PATCH v7 01/14] win-usb-dev: Fix device (un)plug notification handler

[Jonathon Jongsma]

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

On Tue, 2016-03-08 at 16:05 +0200, Dmitry Fleytman wrote:
> This patch fixes device list change notification handing
> logic for cases when more than one device being plugged
> or unplugged simultaneously.
> 
> The simplest way to reproduce the problematic scenario
> is (un)plugging of a usb HUB with a few devices inserted.
> 
> Signed-off-by: Dmitry Fleytman <dmitry at daynix.com>
> ---
>  src/win-usb-dev.c | 93 +++++++++++++++++-------------------------------------
> -
>  1 file changed, 28 insertions(+), 65 deletions(-)
> 
> diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
> index 1e4b2d6..2cb3108 100644
> --- a/src/win-usb-dev.c
> +++ b/src/win-usb-dev.c
> @@ -34,7 +34,6 @@
>  
>  struct _GUdevClientPrivate {
>      libusb_context *ctx;
> -    gssize udev_list_size;
>      GList *udev_list;
>      HWND hwnd;
>  };
> @@ -196,9 +195,7 @@ g_udev_client_initable_init(GInitable *initable,
> GCancellable *cancellable,
>      }
>  
>      /* get initial device list */
> -    priv->udev_list_size = g_udev_client_list_devices(self, &priv->udev_list,
> -                                                      err, __FUNCTION__);
> -    if (priv->udev_list_size < 0) {
> +    if (g_udev_client_list_devices(self, &priv->udev_list, err, __FUNCTION__)
> < 0) {
>          goto g_udev_client_init_failed;
>      }
>  
> @@ -319,94 +316,60 @@ static gboolean get_usb_dev_info(libusb_device *dev,
> GUdevDeviceInfo *udevinfo)
>  }
>  
>  /* Only vid:pid are compared */
> -static gboolean gudev_devices_are_equal(GUdevDevice *a, GUdevDevice *b)
> +static gint gudev_devices_differ(gconstpointer a, gconstpointer b)
>  {
>      GUdevDeviceInfo *ai, *bi;
>      gboolean same_vid;
>      gboolean same_pid;
>  
> -    ai = a->priv->udevinfo;
> -    bi = b->priv->udevinfo;
> +    ai = G_UDEV_DEVICE(a)->priv->udevinfo;
> +    bi = G_UDEV_DEVICE(b)->priv->udevinfo;
>  
>      same_vid  = (ai->vid == bi->vid);
>      same_pid  = (ai->pid == bi->pid);
>  
> -    return (same_pid && same_vid);
> +    return (same_pid && same_vid) ? 0 : -1;
>  }
>  
> +static void notify_dev_state_change(GUdevClient *self,
> +                                    GList *old_list,
> +                                    GList *new_list,
> +                                    const gchar *action)
> +{
> +    GList *dev;
> +
> +    for (dev = g_list_first(old_list); dev != NULL; dev = g_list_next(dev)) {
> +        if (g_list_find_custom(new_list, dev->data, gudev_devices_differ) ==
> NULL) {
> +            /* Found a device that changed its state */
> +            g_udev_device_print(dev->data, action);
> +            g_signal_emit(self, signals[UEVENT_SIGNAL], 0, action, dev
> ->data);
> +        }
> +    }
> +}
>  
> -/* Assumes each event stands for a single device change (at most) */
>  static void handle_dev_change(GUdevClient *self)
>  {
>      GUdevClientPrivate *priv = self->priv;
> -    GUdevDevice *changed_dev = NULL;
> -    ssize_t dev_count;
> -    int is_dev_change;
>      GError *err = NULL;
>      GList *now_devs = NULL;
> -    GList *llist, *slist; /* long-list and short-list*/
> -    GList *lit, *sit; /* iterators for long-list and short-list */
> -    GUdevDevice *ldev, *sdev; /* devices on long-list and short-list */
> -
> -    dev_count = g_udev_client_list_devices(self, &now_devs, &err,
> -                                           __FUNCTION__);
> -    g_return_if_fail(dev_count >= 0);
>  
> -    SPICE_DEBUG("number of current devices %"G_GSSIZE_FORMAT
> -                ", I know about %"G_GSSIZE_FORMAT" devices",
> -                dev_count, priv->udev_list_size);
> -
> -    is_dev_change = dev_count - priv->udev_list_size;
> -    if (is_dev_change == 0) {
> -        g_udev_client_free_device_list(&now_devs);
> +    if(g_udev_client_list_devices(self, &now_devs, &err, __FUNCTION__) < 0) {
> +        g_warning("could not retrieve device list");
>          return;
>      }
>  
> -    if (is_dev_change > 0) {
> -        llist  = now_devs;
> -        slist = priv->udev_list;
> -    } else {
> -        llist = priv->udev_list;
> -        slist  = now_devs;
> -    }
> -
> -    g_udev_device_print_list(llist, "handle_dev_change: long list:");
> -    g_udev_device_print_list(slist, "handle_dev_change: short list:");
> -
> -    /* Go over the longer list */
> -    for (lit = g_list_first(llist); lit != NULL; lit=g_list_next(lit)) {
> -        ldev = lit->data;
> -        /* Look for dev in the shorther list */
> -        for (sit = g_list_first(slist); sit != NULL; sit=g_list_next(sit)) {
> -            sdev = sit->data;
> -            if (gudev_devices_are_equal(ldev, sdev))
> -                break;
> -        }
> -        if (sit == NULL) {
> -            /* Found a device which appears only in the longer list */
> -            changed_dev = ldev;
> -            break;
> -        }
> -    }
> +    g_udev_device_print_list(now_devs, "handle_dev_change: current list:");
> +    g_udev_device_print_list(priv->udev_list, "handle_dev_change: previous
> list:");
>  
> -    if (!changed_dev) {
> -        g_warning("couldn't find any device change");
> -        goto leave;
> -    }
> +    /* Unregister devices that are not present anymore */
> +    notify_dev_state_change(self, priv->udev_list, now_devs, "remove");
>  
> -    if (is_dev_change > 0) {
> -        g_udev_device_print(changed_dev, ">>> USB device inserted");
> -        g_signal_emit(self, signals[UEVENT_SIGNAL], 0, "add", changed_dev);
> -    } else {
> -        g_udev_device_print(changed_dev, "<<< USB device removed");
> -        g_signal_emit(self, signals[UEVENT_SIGNAL], 0, "remove",
> changed_dev);
> -    }
> +    /* Register newly inserted devices */
> +    notify_dev_state_change(self, now_devs, priv->udev_list, "add");
>  
> -leave:
>      /* keep most recent info: free previous list, and keep current list */
>      g_udev_client_free_device_list(&priv->udev_list);
>      priv->udev_list = now_devs;
> -    priv->udev_list_size = dev_count;
>  }
>  
>  static LRESULT CALLBACK wnd_proc(HWND hwnd, UINT message, WPARAM wparam,
> LPARAM lparam)