[Spice-devel,spice-gtk] win-usb-dev: fix device arrival event logic

Submitted by Yuri Benditovich on July 3, 2017, 4:48 a.m.

Details

Message ID 1499057311-684-1-git-send-email-yuri.benditovich@daynix.com
State New
Headers show
Series "win-usb-dev: fix device arrival event logic" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Yuri Benditovich July 3, 2017, 4:48 a.m.
https://bugzilla.redhat.com/show_bug.cgi?id=1425961
If attached new device when one device with the same vid
and pid already present, the notification is ignored and
attached device is not redirected (if auto share set) and
not displayed in USB devices widget

Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
---
 src/win-usb-dev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
index ec3dd91..e5cd7c6 100644
--- a/src/win-usb-dev.c
+++ b/src/win-usb-dev.c
@@ -380,20 +380,20 @@  static gboolean get_usb_dev_info(libusb_device *dev, GUdevDeviceInfo *udevinfo)
     return TRUE;
 }
 
-/* Only vid:pid are compared */
 static gint gudev_devices_differ(gconstpointer a, gconstpointer b)
 {
     GUdevDeviceInfo *ai, *bi;
-    gboolean same_vid;
-    gboolean same_pid;
+    gboolean same_vid, same_pid, same_bus, same_addr;
 
     ai = G_UDEV_DEVICE(a)->priv->udevinfo;
     bi = G_UDEV_DEVICE(b)->priv->udevinfo;
 
     same_vid  = (ai->vid == bi->vid);
     same_pid  = (ai->pid == bi->pid);
+    same_bus = (ai->bus == bi->bus);
+    same_addr = (ai->addr == bi->addr);
 
-    return (same_pid && same_vid) ? 0 : -1;
+    return (same_pid && same_vid && same_bus && same_addr) ? 0 : -1;
 }
 
 static void notify_dev_state_change(GUdevClient *self,

Comments

On Mon, Jul 03, 2017 at 07:48:31AM +0300, Yuri Benditovich wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1425961
> If attached new device when one device with the same vid
> and pid already present, the notification is ignored and
> attached device is not redirected (if auto share set) and
> not displayed in USB devices widget

There apparently were some issues in the past with bus/addr changing
when it should not
https://cgit.freedesktop.org/spice/spice-gtk/commit/?id=f9631cd6f8

Any idea whether this is no longer needed?

Christophe

> 
> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> ---
>  src/win-usb-dev.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
> index ec3dd91..e5cd7c6 100644
> --- a/src/win-usb-dev.c
> +++ b/src/win-usb-dev.c
> @@ -380,20 +380,20 @@ static gboolean get_usb_dev_info(libusb_device *dev, GUdevDeviceInfo *udevinfo)
>      return TRUE;
>  }
>  
> -/* Only vid:pid are compared */
>  static gint gudev_devices_differ(gconstpointer a, gconstpointer b)
>  {
>      GUdevDeviceInfo *ai, *bi;
> -    gboolean same_vid;
> -    gboolean same_pid;
> +    gboolean same_vid, same_pid, same_bus, same_addr;
>  
>      ai = G_UDEV_DEVICE(a)->priv->udevinfo;
>      bi = G_UDEV_DEVICE(b)->priv->udevinfo;
>  
>      same_vid  = (ai->vid == bi->vid);
>      same_pid  = (ai->pid == bi->pid);
> +    same_bus = (ai->bus == bi->bus);
> +    same_addr = (ai->addr == bi->addr);
>  
> -    return (same_pid && same_vid) ? 0 : -1;
> +    return (same_pid && same_vid && same_bus && same_addr) ? 0 : -1;
>  }
>  
>  static void notify_dev_state_change(GUdevClient *self,
> -- 
> 2.9.4
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Mon, Jul 3, 2017 at 10:16 AM, Christophe Fergeau <cfergeau@redhat.com>
wrote:

> On Mon, Jul 03, 2017 at 07:48:31AM +0300, Yuri Benditovich wrote:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1425961
> > If attached new device when one device with the same vid
> > and pid already present, the notification is ignored and
> > attached device is not redirected (if auto share set) and
> > not displayed in USB devices widget
>
> There apparently were some issues in the past with bus/addr changing
> when it should not
> https://cgit.freedesktop.org/spice/spice-gtk/commit/?id=f9631cd6f8
>
> Any idea whether this is no longer needed?
>

There is no additional information about case when the same device comes
with different bus.addr
1. From my point of view this should not be a problem - if new device with
different bus.addr comes in, the previous one with
old bus.addr should disappear and be removed anywhere ; new device shall be
redirected automatically if required.
If such flow will be identified/reported with UsbDk, we will be able to
investigate it and solve.
2. Whether the configuration with WinUSB is still used by spice-gtk on
Windows? According to instructions
on https://www.spice-space.org/spice-user-manual.html , UsbDk should be
used and WinUSB is not mentioned.

If we still need to support WinUSB (?) in backward compatible manner, we
can add the condition and with
WinUSB just one drive with vid/pid combination will be recognized on
plug-in.


>
> Christophe
>
> >
> > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> > ---
> >  src/win-usb-dev.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
> > index ec3dd91..e5cd7c6 100644
> > --- a/src/win-usb-dev.c
> > +++ b/src/win-usb-dev.c
> > @@ -380,20 +380,20 @@ static gboolean get_usb_dev_info(libusb_device
> *dev, GUdevDeviceInfo *udevinfo)
> >      return TRUE;
> >  }
> >
> > -/* Only vid:pid are compared */
> >  static gint gudev_devices_differ(gconstpointer a, gconstpointer b)
> >  {
> >      GUdevDeviceInfo *ai, *bi;
> > -    gboolean same_vid;
> > -    gboolean same_pid;
> > +    gboolean same_vid, same_pid, same_bus, same_addr;
> >
> >      ai = G_UDEV_DEVICE(a)->priv->udevinfo;
> >      bi = G_UDEV_DEVICE(b)->priv->udevinfo;
> >
> >      same_vid  = (ai->vid == bi->vid);
> >      same_pid  = (ai->pid == bi->pid);
> > +    same_bus = (ai->bus == bi->bus);
> > +    same_addr = (ai->addr == bi->addr);
> >
> > -    return (same_pid && same_vid) ? 0 : -1;
> > +    return (same_pid && same_vid && same_bus && same_addr) ? 0 : -1;
> >  }
> >
> >  static void notify_dev_state_change(GUdevClient *self,
> > --
> > 2.9.4
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
On Mon, Jul 03, 2017 at 02:27:29PM +0300, Yuri Benditovich wrote:
> On Mon, Jul 3, 2017 at 10:16 AM, Christophe Fergeau <cfergeau@redhat.com>
> wrote:
> 
> > On Mon, Jul 03, 2017 at 07:48:31AM +0300, Yuri Benditovich wrote:
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1425961
> > > If attached new device when one device with the same vid
> > > and pid already present, the notification is ignored and
> > > attached device is not redirected (if auto share set) and
> > > not displayed in USB devices widget
> >
> > There apparently were some issues in the past with bus/addr changing
> > when it should not
> > https://cgit.freedesktop.org/spice/spice-gtk/commit/?id=f9631cd6f8
> >
> > Any idea whether this is no longer needed?
> >
> 
> There is no additional information about case when the same device comes
> with different bus.addr
> 1. From my point of view this should not be a problem - if new device with
> different bus.addr comes in, the previous one with
> old bus.addr should disappear and be removed anywhere ; new device shall be
> redirected automatically if required.
> If such flow will be identified/reported with UsbDk, we will be able to
> investigate it and solve.
> 2. Whether the configuration with WinUSB is still used by spice-gtk on
> Windows? According to instructions
> on https://www.spice-space.org/spice-user-manual.html , UsbDk should be
> used and WinUSB is not mentioned.

To be honest, I don't know what WinUSB is.. ;) But since this patch is
removing an (old) bugfix, I'd explain in the commit log why it's no
longer necessary to have it.

Christophe
It does not exactly removes old bugfix.
First version of the code did check for bus.addr only and for example,
ignored change of vid/pid without change of bus.addr (AFAIK this is what
programmable devices like Cypress do).
Previous commit removed check for bus.addr and replaced it with check for
vid and pid, effectively causing the bug we are fixing now.
Now we check all of them and ignore the update event only if there is no
change in USB tree.

I'll mention the old commit in new commit message.
I'll also ask the author of previous commit for comment.

On Mon, Jul 3, 2017 at 6:31 PM, Christophe Fergeau <cfergeau@redhat.com>
wrote:

> On Mon, Jul 03, 2017 at 02:27:29PM +0300, Yuri Benditovich wrote:
> > On Mon, Jul 3, 2017 at 10:16 AM, Christophe Fergeau <cfergeau@redhat.com
> >
> > wrote:
> >
> > > On Mon, Jul 03, 2017 at 07:48:31AM +0300, Yuri Benditovich wrote:
> > > > https://bugzilla.redhat.com/show_bug.cgi?id=1425961
> > > > If attached new device when one device with the same vid
> > > > and pid already present, the notification is ignored and
> > > > attached device is not redirected (if auto share set) and
> > > > not displayed in USB devices widget
> > >
> > > There apparently were some issues in the past with bus/addr changing
> > > when it should not
> > > https://cgit.freedesktop.org/spice/spice-gtk/commit/?id=f9631cd6f8
> > >
> > > Any idea whether this is no longer needed?
> > >
> >
> > There is no additional information about case when the same device comes
> > with different bus.addr
> > 1. From my point of view this should not be a problem - if new device
> with
> > different bus.addr comes in, the previous one with
> > old bus.addr should disappear and be removed anywhere ; new device shall
> be
> > redirected automatically if required.
> > If such flow will be identified/reported with UsbDk, we will be able to
> > investigate it and solve.
> > 2. Whether the configuration with WinUSB is still used by spice-gtk on
> > Windows? According to instructions
> > on https://www.spice-space.org/spice-user-manual.html , UsbDk should be
> > used and WinUSB is not mentioned.
>
> To be honest, I don't know what WinUSB is.. ;) But since this patch is
> removing an (old) bugfix, I'd explain in the commit log why it's no
> longer necessary to have it.
>
> Christophe
>
Hi,

On 07/03/2017 06:53 PM, Yuri Benditovich wrote:
> It does not exactly removes old bugfix.

Possibly it does, but only if WinUsb (+ usbclerk) are used.

> First version of the code did check for bus.addr only and for example, 
> ignored change of vid/pid without change of bus.addr (AFAIK this is what 
> programmable devices like Cypress do).

Yes, this is the preferred method to identify a specific device in the
system, when not using WinUSB.

> Previous commit removed check for bus.addr and replaced it with check 
> for vid and pid, effectively causing the bug we are fixing now.

Said commit fixed a bug (feature?) that only occurs with WinUSb
(at least at the time it was committed). The bus.addr id
before/after installing the WinUSB driver may be different.
With WinUSB, the driver needs to be installed separately
for every device that is redirected. This sometimes caused
the bus.addr to change.
That means one can not rely on bus.addr to identify a specific device.
The WinUSB driver would only be installed once the device
is to be usbredir'ed with the guest (that was done via usbclerk).
Using vid:pid is a good identifier in such a scenario, but
of course can only support a single such vid:pid device
per client.

Since commit 66c705caca74f61ffce8637bee38b1e5e4909062,
WinUSB is not automatically installed via usbclerk.
We recommend using UsbDk.

> Now we check all of them and ignore the update event only if there is no 
> change in USB tree.
Alternatively you can simply revert the aforementioned patch.
You do not need to check vid/pid (should not harm either)

> 
> I'll mention the old commit in new commit message.
> I'll also ask the author of previous commit for comment.

I agree with Christophe that a comment mentioning
old commit and the behavior difference between WinUSB
and UsbDk would be helpful.

Regards,
     Uri.


> 
> On Mon, Jul 3, 2017 at 6:31 PM, Christophe Fergeau <cfergeau@redhat.com 
> <mailto:cfergeau@redhat.com>> wrote:
> 
>     On Mon, Jul 03, 2017 at 02:27:29PM +0300, Yuri Benditovich wrote:
>      > On Mon, Jul 3, 2017 at 10:16 AM, Christophe Fergeau
>     <cfergeau@redhat.com <mailto:cfergeau@redhat.com>>
>      > wrote:
>      >
>      > > On Mon, Jul 03, 2017 at 07:48:31AM +0300, Yuri Benditovich wrote:
>      > > > https://bugzilla.redhat.com/show_bug.cgi?id=1425961
>     <https://bugzilla.redhat.com/show_bug.cgi?id=1425961>
>      > > > If attached new device when one device with the same vid
>      > > > and pid already present, the notification is ignored and
>      > > > attached device is not redirected (if auto share set) and
>      > > > not displayed in USB devices widget
>      > >
>      > > There apparently were some issues in the past with bus/addr
>     changing
>      > > when it should not
>      > >
>     https://cgit.freedesktop.org/spice/spice-gtk/commit/?id=f9631cd6f8
>     <https://cgit.freedesktop.org/spice/spice-gtk/commit/?id=f9631cd6f8>
>      > >
>      > > Any idea whether this is no longer needed?
>      > >
>      >
>      > There is no additional information about case when the same
>     device comes
>      > with different bus.addr
>      > 1. From my point of view this should not be a problem - if new
>     device with
>      > different bus.addr comes in, the previous one with
>      > old bus.addr should disappear and be removed anywhere ; new
>     device shall be
>      > redirected automatically if required.
>      > If such flow will be identified/reported with UsbDk, we will be
>     able to
>      > investigate it and solve.
>      > 2. Whether the configuration with WinUSB is still used by
>     spice-gtk on
>      > Windows? According to instructions
>      > on https://www.spice-space.org/spice-user-manual.html
>     <https://www.spice-space.org/spice-user-manual.html> , UsbDk should be
>      > used and WinUSB is not mentioned.
> 
>     To be honest, I don't know what WinUSB is.. ;) But since this patch is
>     removing an (old) bugfix, I'd explain in the commit log why it's no
>     longer necessary to have it.
> 
>     Christophe
> 
> 
> 
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>