[Spice-devel] [PATCH v6 01/14] win-usb-dev: Fix device (un)plug notification handler
Dmitry Fleytman
dmitry at daynix.com
Tue Mar 8 13:37:34 UTC 2016
> On 1 Mar 2016, at 24:06 AM, Jonathon Jongsma <jjongsma at redhat.com> wrote:
>
> 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.
Right, I forgot to drop it. Removed.
>
>
>> 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
Ok.
>
>
>> 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
Actually these nested loops are similar in both cases so in the next version I moved those to a separate function and used g_list_find_custom() as you suggested.
>
>> + }
>>
>> - 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 <mailto:jjongsma at redhat.com>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160308/c047d56d/attachment-0001.html>
More information about the Spice-devel
mailing list