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

[Jonathon Jongsma]

On Sun, 2016-02-28 at 11:54 +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 | 82 +++++++++++++++++++-----------------------------------
> -
>  1 file changed, 28 insertions(+), 54 deletions(-)
> 
> diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
> index 1e4b2d6..1b4d353 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;
>      }
>  
> @@ -339,74 +336,51 @@ static gboolean gudev_devices_are_equal(GUdevDevice *a,
> GUdevDevice *b)

Immediately above this line is the following comment:

/* Assumes each event stands for a single device change (at most) */

Since you're changing the function to handle multiple insertions or removals at
once, we should probably remove this comment.


>  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 */
> +    GList *prev, *curr; /* iterators for previous and current device lists */
>  
> -    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:");
> +    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:");
>  
> -    /* 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))
> +    /* Unregister devices that are not present anymore */
> +    for (prev = g_list_first(priv->udev_list); prev != NULL; prev =
> g_list_next(prev)) {
> +        /* Look for dev in the current list */
> +        for (curr = g_list_first(now_devs); curr != NULL; curr = 
> g_list_next(curr)) {
> +            if (gudev_devices_are_equal(prev->data, curr->data))

g_list_find_custom() is a possibility here


>                  break;
>          }
> -        if (sit == NULL) {
> -            /* Found a device which appears only in the longer list */
> -            changed_dev = ldev;
> -            break;
> +
> +        if (curr == NULL) {
> +            /* Found a device that was removed */
> +            g_udev_device_print(prev->data, "<<< USB device removed");
> +            g_signal_emit(self, signals[UEVENT_SIGNAL], 0, "remove", prev
> ->data);
>          }
>      }
>  
> -    if (!changed_dev) {
> -        g_warning("couldn't find any device change");
> -        goto leave;
> -    }
> +    /* Register newly inserted devices */
> +    for (curr = g_list_first(now_devs); curr != NULL; curr =
> g_list_next(curr)) {
> +        /* Look for dev in the previous list */
> +        for (prev = g_list_first(priv->udev_list); prev != NULL; prev =
> g_list_next(prev)) {
> +            if (gudev_devices_are_equal(prev->data, curr->data))
> +                break;

and here

> +        }
>  
> -    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);
> +        if (prev == NULL) {
> +            /* Found a device that was inserted */
> +            g_udev_device_print(curr->data, ">>> USB device inserted");
> +            g_signal_emit(self, signals[UEVENT_SIGNAL], 0, "add", curr
> ->data);
> +        }
>      }
>  
> -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)


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