[Spice-devel] [PATCH v2 2/6] WinUsbDev: add redirecting state to GUdevClientPrivate

[Christophe Fergeau]

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.

> 
> Signed-off-by: Kirill Moizik <kirill at daynix.com>
> ---
>  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;
> +}

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...

>  
>  /* 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150707/b02abd66/attachment-0001.sig>