<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Tue, Jul 7, 2015 at 1:47 PM, Christophe Fergeau <<a href="mailto:cfergeau@redhat.com" target="_blank">cfergeau@redhat.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">On Mon, Jul 06, 2015 at 08:59:02PM +0300, Kirill Moizik wrote: > wnd_proc callback should not query usb devices in the middle of redirecting flow. Caller may get inconsistent enumeration results due to device resets performed by UsbDk and Windows mechanisms handling those resets. Please wrap all your logs to 72 chars or so. <div><div class="h5"> > > Signed-off-by: Kirill Moizik <<a href="mailto:kirill@daynix.com">kirill@daynix.com</a>> > --- > src/map-file | 2 ++ > src/win-usb-dev.c | 15 +++++++++++++++ > src/win-usb-dev.h | 2 ++ > 3 files changed, 19 insertions(+) > > diff --git a/src/map-file b/src/map-file > index d5a073f..d0c24a4 100644 > --- a/src/map-file > +++ b/src/map-file > @@ -117,6 +117,8 @@ spice_uri_to_string; > spice_usb_device_get_description; > spice_usb_device_get_libusb_device; > spice_usb_device_get_type; > +spice_g_udev_set_redirecting; > +spice_g_udev_handle_device_change; > spice_usb_device_manager_can_redirect_device; > spice_usb_device_manager_connect_device_async; > spice_usb_device_manager_connect_device_finish; > diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c > index 1e4b2d6..23bea42 100644 > --- a/src/win-usb-dev.c > +++ b/src/win-usb-dev.c > @@ -37,6 +37,7 @@ struct _GUdevClientPrivate { > gssize udev_list_size; > GList *udev_list; > HWND hwnd; > + gboolean redirecting; > }; > > #define G_UDEV_CLIENT_WINCLASS_NAME TEXT("G_UDEV_CLIENT") > @@ -186,6 +187,7 @@ g_udev_client_initable_init(GInitable *initable, GCancellable *cancellable, > self = G_UDEV_CLIENT(initable); > priv = self->priv; > > + priv->redirecting = FALSE; > rc = libusb_init(&priv->ctx); > if (rc < 0) { > const char *errstr = spice_usbutil_libusb_strerror(rc); > @@ -334,6 +336,11 @@ static gboolean gudev_devices_are_equal(GUdevDevice *a, GUdevDevice *b) > return (same_pid && same_vid); > } > > +void spice_g_udev_set_redirecting (gboolean val) > +{ > + GUdevClientPrivate *priv = singleton->priv; > + priv->redirecting = val; > +} </div></div>I still think we should have something like if (priv->redirecting && !val) { handle_dev_change(singleton); } and not export spice_g_udev_handle_device_change() as updating the device list is required anyway when redirecting changes from TRUE to FALSE. Doing it automatically makes using the API less error-prone... </blockquote><div> </div><div>The problem is not to prevent call handle_dev_change from the usb-device-widget.c</div><div>I agree with you here, in this case it is better solution. The problem is here</div><div> </div><div>static LRESULT CALLBACK wnd_proc(HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam) { /* Only DBT_DEVNODES_CHANGED recieved */ if (message == WM_DEVICECHANGE) { handle_dev_change(singleton); } return DefWindowProc(hwnd, message, wparam, lparam); } It is a registered system callback. The only context available when it is called is GUdevClient singleton object. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> > > /* Assumes each event stands for a single device change (at most) */ > static void handle_dev_change(GUdevClient *self) > @@ -347,6 +354,9 @@ static void handle_dev_change(GUdevClient *self) > 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 */ > + if (priv->redirecting == TRUE) { Just if (priv->redirecting) { }, especially as g_udev_set_redirecting() does not do priv->redirecting = !!val; to ensure this only gets set to 0 or 1 Christophe </blockquote></div> </div></div>