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

[Kirill Moizik]

On Tue, Jul 7, 2015 at 1:47 PM, Christophe Fergeau <cfergeau at redhat.com>
wrote:

> 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...
>
>
The problem is not to prevent call  handle_dev_change from  the
usb-device-widget.c
I agree with you here, in this case it is better solution. The problem is
here

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.


>
> >  /* 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 --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150707/4e519c01/attachment.html>