Re: [Spice-devel] [spice-gtk] usb-redir: use persistent libusb device on Windows
On Mon, Feb 25, 2019 at 04:07:03PM +0200, Yuri Benditovich wrote: > On Fri, Feb 22, 2019 at 2:06 PM Christophe Fergeau > wrote: > > I don't think we should have a hard limit on the number of lines in a > > patch, however there should be a maximum of 1 logical change per commit, > > see > > https://www.berrange.com/posts/2012/06/27/thoughts-on-improving-openstack-git-commit-practicehistory/ > > for example for a rationale about this. > > https://gitlab.freedesktop.org/teuf/spice-gtk/tree/gudev would be a > > rough attempt at such a split (but authorship needs to be set back to > > you, commit logs needs to be more verbose, ...). > > > > IMO, this is excellent example of why this approach is wrong. > For example, there is definitely wrong commit > https://gitlab.freedesktop.org/teuf/spice-gtk/commit/0135d831bc8929e45bac065f47daa1b4470ab7b0 > But nobody notice it as the problem in it fixed toward end of chain. When I said "rough attempt at such a split", I meant it to be read as "this is an untested proof of concept". So yes, there are bugs ;) > Which tests are expected after _each_ commit? Really depends on what the commit is doing. Each commit should build, and basic functionality should still be working. I'd usually keep extensive testing for the final patch, if some things are broken, I'd move the fix to the right place, and possibly do more testing for the commit which was broken. In an ideal world, >95% of the testing would be automated, which would make it trivial to test each commit.. Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk] usb-redir: use persistent libusb device on Windows
> On Fri, Feb 22, 2019 at 2:06 PM Christophe Fergeau
> wrote:
> >
> > Hi,
> >
> > On Thu, Feb 21, 2019 at 09:37:06AM +0200, Yuri Benditovich wrote:
> > > On Wed, Feb 20, 2019 at 6:03 PM Christophe Fergeau
> > > wrote:
> > > > > static SpiceUsbDevice*
> > > > > spice_usb_device_manager_find_device(SpiceUsbDeviceManager *self,
> > > > > const int bus, const int
> > > > > address)
> > > > > @@ -970,6 +914,16 @@ static void
> > > > > spice_usb_device_manager_add_dev(SpiceUsbDeviceManager *self,
> > > > > if (desc.bDeviceClass == LIBUSB_CLASS_HUB)
> > > > > return;
> > > > >
> > > > > +if (spice_usb_device_manager_find_device(self,
> > > > > +libusb_get_bus_number(libdev),
> > > > > +
> > > > > libusb_get_device_address(libdev)))
> > > > > {
> > > > > +SPICE_DEBUG("device not added %04x:%04x (%p)",
> > > > > +desc.idVendor,
> > > > > +desc.idProduct,
> > > > > +libdev);
> > > > > +return;
> > > > > +}
> > > > > +
> > > >
> > > > I did not understand why we need this?
> > >
> > > There are several reasons:
> > > 1. It is possible that the hot plug procedure for Linux might be
> > > called more than once for the same device (this is mentioned in libusb
> > > API).
> >
> > It's
> > http://libusb.sourceforge.net/api-1.0/group__hotplug.html#gae6c5f1add6cc754005549c7259dc35ea
> > and this can happen at coldplug time (ie when using
> > LIBUSB_HOTPLUG_ENUMERATE), so exactly the race I was worried about
> > before.
> >
> > > 2. If second instance of usb-device-manager created (as it happens at
> > > time of remote-viewer termination),
> >
> > It sounds weird that we create a new one when exiting, probably
> > something we can/should get rid of if this starts causing problems.
> >
> > > existing one receives each device that already present.
> > > 3. It is much simpler to exclude device duplication than later look
> > > why they present.
> >
> >
> > All of these explanations would be very useful to have in the log of a
> > separate commit.
> >
> > > > > +#ifdef G_OS_WIN32
> > > > > static void spice_usb_device_manager_uevent_cb(GUdevClient
> > > > > *client,
> > > > > - const gchar
> > > > > *action,
> > > > > - GUdevDevice
> > > > > *udevice,
> > > > > + libusb_device
> > > > > *libdev,
> > > > > + gboolean add,
> > > > > gpointer
> > > > > user_data)
> > > > > {
> > > > > SpiceUsbDeviceManager *self =
> > > > > SPICE_USB_DEVICE_MANAGER(user_data);
> > > > >
> > > > > -if (g_str_equal(action, "add"))
> > > > > -spice_usb_device_manager_add_udev(self, udevice);
> > > > > -else if (g_str_equal (action, "remove"))
> > > > > -spice_usb_device_manager_remove_udev(self, udevice);
> > > > > +if (add)
> > > > > +spice_usb_device_manager_add_dev(self, libdev);
> > > > > +else
> > > > > +spice_usb_device_manager_remove_dev(self,
> > > > > +libusb_get_bus_number(libdev),
> > > > > +
> > > > > libusb_get_device_address(libdev));
> > > >
> > > > This could almost reuse spice_usb_device_manager_hotplug_cb somehow,
> > > > but
> > > > I'm not sure this is worth it.
> > >
> > > What I'm expected to do to address this note?
> >
> > Just random thoughts, maybe you'll think this could be a useful
> > improvement, maybe you already considered it and rejected it, maybe it's
> > not a good idea, I don't know. I don't have any expectations, just
> > something that could be useful to mention.
> >
> >
> > > > > @@ -1889,9 +1751,7 @@ static SpiceUsbDeviceInfo
> > > > > *spice_usb_device_new(libusb_device *libdev)
> > > > > info->pid = pid;
> > > > > info->ref = 1;
> > > > > info->isochronous = probe_isochronous_endpoint(libdev);
> > > > > -#ifndef G_OS_WIN32
> > > > > info->libdev = libusb_ref_device(libdev);
> > > > > -#endif
> > > > >
> > > > > return info;
> > > > > }
> > > > > @@ -2027,14 +1887,11 @@ static void
> > > > > spice_usb_device_unref(SpiceUsbDevice *device)
> > > > >
> > > > > ref_count_is_0 = g_atomic_int_dec_and_test(>ref);
> > > > > if (ref_count_is_0) {
> > > > > -#ifndef G_OS_WIN32
> > > > > libusb_unref_device(info->libdev);
> > > > > -#endif
> > > >
> > > > Most of the 'persistent' libusb_device changes for Windows are in the
> > > > hunks before this, but they also depend on some rework of the hotplug
> > > > detection in GUdevClient.
> > >
> > > What I'm expected to do to address this note?
> >
> > Just a comment that could be useful to me in the future, or to other
> > people looking at
Re: [Spice-devel] [spice-gtk] usb-redir: use persistent libusb device on Windows
On Fri, Feb 22, 2019 at 2:06 PM Christophe Fergeau wrote:
>
> Hi,
>
> On Thu, Feb 21, 2019 at 09:37:06AM +0200, Yuri Benditovich wrote:
> > On Wed, Feb 20, 2019 at 6:03 PM Christophe Fergeau
> > wrote:
> > > > static SpiceUsbDevice*
> > > > spice_usb_device_manager_find_device(SpiceUsbDeviceManager *self,
> > > > const int bus, const int address)
> > > > @@ -970,6 +914,16 @@ static void
> > > > spice_usb_device_manager_add_dev(SpiceUsbDeviceManager *self,
> > > > if (desc.bDeviceClass == LIBUSB_CLASS_HUB)
> > > > return;
> > > >
> > > > +if (spice_usb_device_manager_find_device(self,
> > > > +libusb_get_bus_number(libdev),
> > > > +
> > > > libusb_get_device_address(libdev))) {
> > > > +SPICE_DEBUG("device not added %04x:%04x (%p)",
> > > > +desc.idVendor,
> > > > +desc.idProduct,
> > > > +libdev);
> > > > +return;
> > > > +}
> > > > +
> > >
> > > I did not understand why we need this?
> >
> > There are several reasons:
> > 1. It is possible that the hot plug procedure for Linux might be
> > called more than once for the same device (this is mentioned in libusb
> > API).
>
> It's
> http://libusb.sourceforge.net/api-1.0/group__hotplug.html#gae6c5f1add6cc754005549c7259dc35ea
> and this can happen at coldplug time (ie when using
> LIBUSB_HOTPLUG_ENUMERATE), so exactly the race I was worried about
> before.
>
> > 2. If second instance of usb-device-manager created (as it happens at
> > time of remote-viewer termination),
>
> It sounds weird that we create a new one when exiting, probably
> something we can/should get rid of if this starts causing problems.
>
> > existing one receives each device that already present.
> > 3. It is much simpler to exclude device duplication than later look
> > why they present.
>
>
> All of these explanations would be very useful to have in the log of a
> separate commit.
>
> > > > +#ifdef G_OS_WIN32
> > > > static void spice_usb_device_manager_uevent_cb(GUdevClient *client,
> > > > - const gchar *action,
> > > > - GUdevDevice
> > > > *udevice,
> > > > + libusb_device *libdev,
> > > > + gboolean add,
> > > > gpointer
> > > > user_data)
> > > > {
> > > > SpiceUsbDeviceManager *self = SPICE_USB_DEVICE_MANAGER(user_data);
> > > >
> > > > -if (g_str_equal(action, "add"))
> > > > -spice_usb_device_manager_add_udev(self, udevice);
> > > > -else if (g_str_equal (action, "remove"))
> > > > -spice_usb_device_manager_remove_udev(self, udevice);
> > > > +if (add)
> > > > +spice_usb_device_manager_add_dev(self, libdev);
> > > > +else
> > > > +spice_usb_device_manager_remove_dev(self,
> > > > +libusb_get_bus_number(libdev),
> > > > +libusb_get_device_address(libdev));
> > >
> > > This could almost reuse spice_usb_device_manager_hotplug_cb somehow, but
> > > I'm not sure this is worth it.
> >
> > What I'm expected to do to address this note?
>
> Just random thoughts, maybe you'll think this could be a useful
> improvement, maybe you already considered it and rejected it, maybe it's
> not a good idea, I don't know. I don't have any expectations, just
> something that could be useful to mention.
>
>
> > > > @@ -1889,9 +1751,7 @@ static SpiceUsbDeviceInfo
> > > > *spice_usb_device_new(libusb_device *libdev)
> > > > info->pid = pid;
> > > > info->ref = 1;
> > > > info->isochronous = probe_isochronous_endpoint(libdev);
> > > > -#ifndef G_OS_WIN32
> > > > info->libdev = libusb_ref_device(libdev);
> > > > -#endif
> > > >
> > > > return info;
> > > > }
> > > > @@ -2027,14 +1887,11 @@ static void
> > > > spice_usb_device_unref(SpiceUsbDevice *device)
> > > >
> > > > ref_count_is_0 = g_atomic_int_dec_and_test(>ref);
> > > > if (ref_count_is_0) {
> > > > -#ifndef G_OS_WIN32
> > > > libusb_unref_device(info->libdev);
> > > > -#endif
> > >
> > > Most of the 'persistent' libusb_device changes for Windows are in the
> > > hunks before this, but they also depend on some rework of the hotplug
> > > detection in GUdevClient.
> >
> > What I'm expected to do to address this note?
>
> Just a comment that could be useful to me in the future, or to other
> people looking at that mail.
>
>
> > > > diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
> > > > index 327976d..ef6804f 100644
> > > > --- a/src/win-usb-dev.c
> > > > +++ b/src/win-usb-dev.c
> > > > @@ -49,31 +49,6 @@ G_DEFINE_TYPE_WITH_CODE(GUdevClient, g_udev_client,
> > > > G_TYPE_OBJECT,
> > > >
Re: [Spice-devel] [spice-gtk] usb-redir: use persistent libusb device on Windows
Hi,
On Thu, Feb 21, 2019 at 09:37:06AM +0200, Yuri Benditovich wrote:
> On Wed, Feb 20, 2019 at 6:03 PM Christophe Fergeau
> wrote:
> > > static SpiceUsbDevice*
> > > spice_usb_device_manager_find_device(SpiceUsbDeviceManager *self,
> > > const int bus, const int address)
> > > @@ -970,6 +914,16 @@ static void
> > > spice_usb_device_manager_add_dev(SpiceUsbDeviceManager *self,
> > > if (desc.bDeviceClass == LIBUSB_CLASS_HUB)
> > > return;
> > >
> > > +if (spice_usb_device_manager_find_device(self,
> > > +libusb_get_bus_number(libdev),
> > > +libusb_get_device_address(libdev))) {
> > > +SPICE_DEBUG("device not added %04x:%04x (%p)",
> > > +desc.idVendor,
> > > +desc.idProduct,
> > > +libdev);
> > > +return;
> > > +}
> > > +
> >
> > I did not understand why we need this?
>
> There are several reasons:
> 1. It is possible that the hot plug procedure for Linux might be
> called more than once for the same device (this is mentioned in libusb
> API).
It's
http://libusb.sourceforge.net/api-1.0/group__hotplug.html#gae6c5f1add6cc754005549c7259dc35ea
and this can happen at coldplug time (ie when using
LIBUSB_HOTPLUG_ENUMERATE), so exactly the race I was worried about
before.
> 2. If second instance of usb-device-manager created (as it happens at
> time of remote-viewer termination),
It sounds weird that we create a new one when exiting, probably
something we can/should get rid of if this starts causing problems.
> existing one receives each device that already present.
> 3. It is much simpler to exclude device duplication than later look
> why they present.
All of these explanations would be very useful to have in the log of a
separate commit.
> > > +#ifdef G_OS_WIN32
> > > static void spice_usb_device_manager_uevent_cb(GUdevClient *client,
> > > - const gchar *action,
> > > - GUdevDevice *udevice,
> > > + libusb_device *libdev,
> > > + gboolean add,
> > > gpointer
> > > user_data)
> > > {
> > > SpiceUsbDeviceManager *self = SPICE_USB_DEVICE_MANAGER(user_data);
> > >
> > > -if (g_str_equal(action, "add"))
> > > -spice_usb_device_manager_add_udev(self, udevice);
> > > -else if (g_str_equal (action, "remove"))
> > > -spice_usb_device_manager_remove_udev(self, udevice);
> > > +if (add)
> > > +spice_usb_device_manager_add_dev(self, libdev);
> > > +else
> > > +spice_usb_device_manager_remove_dev(self,
> > > +libusb_get_bus_number(libdev),
> > > +libusb_get_device_address(libdev));
> >
> > This could almost reuse spice_usb_device_manager_hotplug_cb somehow, but
> > I'm not sure this is worth it.
>
> What I'm expected to do to address this note?
Just random thoughts, maybe you'll think this could be a useful
improvement, maybe you already considered it and rejected it, maybe it's
not a good idea, I don't know. I don't have any expectations, just
something that could be useful to mention.
> > > @@ -1889,9 +1751,7 @@ static SpiceUsbDeviceInfo
> > > *spice_usb_device_new(libusb_device *libdev)
> > > info->pid = pid;
> > > info->ref = 1;
> > > info->isochronous = probe_isochronous_endpoint(libdev);
> > > -#ifndef G_OS_WIN32
> > > info->libdev = libusb_ref_device(libdev);
> > > -#endif
> > >
> > > return info;
> > > }
> > > @@ -2027,14 +1887,11 @@ static void spice_usb_device_unref(SpiceUsbDevice
> > > *device)
> > >
> > > ref_count_is_0 = g_atomic_int_dec_and_test(>ref);
> > > if (ref_count_is_0) {
> > > -#ifndef G_OS_WIN32
> > > libusb_unref_device(info->libdev);
> > > -#endif
> >
> > Most of the 'persistent' libusb_device changes for Windows are in the
> > hunks before this, but they also depend on some rework of the hotplug
> > detection in GUdevClient.
>
> What I'm expected to do to address this note?
Just a comment that could be useful to me in the future, or to other
people looking at that mail.
> > > diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
> > > index 327976d..ef6804f 100644
> > > --- a/src/win-usb-dev.c
> > > +++ b/src/win-usb-dev.c
> > > @@ -49,31 +49,6 @@ G_DEFINE_TYPE_WITH_CODE(GUdevClient, g_udev_client,
> > > G_TYPE_OBJECT,
> > > G_ADD_PRIVATE(GUdevClient)
> > > G_IMPLEMENT_INTERFACE(G_TYPE_INITABLE,
> > > g_udev_client_initable_iface_init))
> > >
> > > -
> > > -typedef struct _GUdevDeviceInfo GUdevDeviceInfo;
> > > -
> > > -struct _GUdevDeviceInfo {
> > > -guint16 bus;
> > > -guint16
Re: [Spice-devel] [spice-gtk] usb-redir: use persistent libusb device on Windows
On Wed, Feb 20, 2019 at 6:03 PM Christophe Fergeau wrote:
>
> Hey,
>
> Sorry, it took a bit of time to review, but this patch is sticking a lot
> of changes together, splitting such patches in multiple smaller ones
> really help to get speedier reviews (and actually, probably improves
> these reviews, there are some things I would have missed if I did not
> split this locally myself).
>
> On Mon, Feb 11, 2019 at 11:42:03AM +0200, Yuri Benditovich wrote:
> > Discard unneeded GUDev simulation and use persistent libusb
> > device pointer in SpiceUsbDevice on Windows as we do on Linux.
> > This removes significant part of differences between Linux and
> > Windows code. GUDevClient in win-usb-dev.c now maintains list
> > of libusb_device pointers and passes new and removed libusb devices
> > to usb-device-manager via signal.
>
> Yes, I would even emphazise in the commit log that this is the *only*
> thing GUdevClient does now, it's just a wrapper on top of libusb which
> gives us hotplug events on Windows.
> What this commit does in addition to using a persistent libusb device
> pointer is to kill GUdevInfo and GUdevDevice, which means some
> s/GUdevDevice/libusb_device in the codebase. It also changes some
> USE_GUDEV to G_OS_WIN32, remove an unused field in SpiceUsbDeviceInfo,
> changes callback parameters from "add"/"remove" to a boolean,
> ... (and that really is a lot to do in a single patch:-/ )
>
> > Important part of the change is that usb-device-manager on Windows
> > does not create its own libusb context and uses one created by
> > win-usb-dev (as libusb device must be enumerated and redirected
> > with the same libusb context).
> >
> > Signed-off-by: Yuri Benditovich
> > ---
> > src/usb-device-manager.c | 290 +++
> > src/win-usb-dev.c| 254 ++
> > src/win-usb-dev.h| 34 +
> > 3 files changed, 126 insertions(+), 452 deletions(-)
> >
> > diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> > index 55b672b..009b15b 100644
> > --- a/src/usb-device-manager.c
> > +++ b/src/usb-device-manager.c
> > @@ -33,7 +33,6 @@
> >
> > #if defined(G_OS_WIN32)
> > #include "win-usb-dev.h"
> > -#define USE_GUDEV /* win-usb-dev.h provides a fake gudev interface */
> > #endif
> >
> > #include "channel-usbredir-priv.h"
> > @@ -106,11 +105,10 @@ struct _SpiceUsbDeviceManagerPrivate {
> > struct usbredirfilter_rule *redirect_on_connect_rules;
> > int auto_conn_filter_rules_count;
> > int redirect_on_connect_rules_count;
> > -#ifdef USE_GUDEV
> > +#ifdef G_OS_WIN32
> > GUdevClient *udev;
> > -libusb_device **coldplug_list; /* Avoid needless reprobing during init
> > */
> > #else
> > -gboolean redirecting; /* Handled by GUdevClient in the gudev case */
> > +gboolean redirecting; /* Handled by GUdevClient in Windows */
> > libusb_hotplug_callback_handle hp_handle;
> > #endif
> > #ifdef G_OS_WIN32
> > @@ -141,11 +139,7 @@ typedef struct _SpiceUsbDeviceInfo {
> > guint16 vid;
> > guint16 pid;
> > gboolean isochronous;
> > -#ifdef G_OS_WIN32
> > -guint8 state;
> > -#else
> > libusb_device *libdev;
> > -#endif
> > gintref;
> > } SpiceUsbDeviceInfo;
> >
> > @@ -156,13 +150,11 @@ static void channel_destroy(SpiceSession *session,
> > SpiceChannel *channel,
> > gpointer user_data);
> > static void channel_event(SpiceChannel *channel, SpiceChannelEvent event,
> >gpointer user_data);
> > -#ifdef USE_GUDEV
> > +#ifdef G_OS_WIN32
> > static void spice_usb_device_manager_uevent_cb(GUdevClient *client,
> > - const gchar *action,
> > - GUdevDevice *udevice,
> > + libusb_device *libdev,
> > + gboolean add,
> > gpointer user_data);
> > -static void spice_usb_device_manager_add_udev(SpiceUsbDeviceManager *self,
> > - GUdevDevice
> > *udev);
> > #else
> > static int spice_usb_device_manager_hotplug_cb(libusb_context *ctx,
> > libusb_device
> > *device,
> > @@ -210,7 +202,7 @@ G_DEFINE_BOXED_TYPE(SpiceUsbDevice, spice_usb_device,
> > static void
> > _set_redirecting(SpiceUsbDeviceManager *self, gboolean is_redirecting)
> > {
> > -#ifdef USE_GUDEV
> > +#ifdef G_OS_WIN32
> > g_object_set(self->priv->udev, "redirecting", is_redirecting, NULL);
> > #else
> > self->priv->redirecting = is_redirecting;
> > @@ -235,7 +227,7 @@ gboolean
> > spice_usb_device_manager_is_redirecting(SpiceUsbDeviceManager *self)
> > {
> > #ifdef USE_USBREDIR
> >
> > -#ifdef USE_GUDEV
> > +#ifdef G_OS_WIN32
> > gboolean redirecting;
>
Re: [Spice-devel] [spice-gtk] usb-redir: use persistent libusb device on Windows
Hey,
Sorry, it took a bit of time to review, but this patch is sticking a lot
of changes together, splitting such patches in multiple smaller ones
really help to get speedier reviews (and actually, probably improves
these reviews, there are some things I would have missed if I did not
split this locally myself).
On Mon, Feb 11, 2019 at 11:42:03AM +0200, Yuri Benditovich wrote:
> Discard unneeded GUDev simulation and use persistent libusb
> device pointer in SpiceUsbDevice on Windows as we do on Linux.
> This removes significant part of differences between Linux and
> Windows code. GUDevClient in win-usb-dev.c now maintains list
> of libusb_device pointers and passes new and removed libusb devices
> to usb-device-manager via signal.
Yes, I would even emphazise in the commit log that this is the *only*
thing GUdevClient does now, it's just a wrapper on top of libusb which
gives us hotplug events on Windows.
What this commit does in addition to using a persistent libusb device
pointer is to kill GUdevInfo and GUdevDevice, which means some
s/GUdevDevice/libusb_device in the codebase. It also changes some
USE_GUDEV to G_OS_WIN32, remove an unused field in SpiceUsbDeviceInfo,
changes callback parameters from "add"/"remove" to a boolean,
... (and that really is a lot to do in a single patch:-/ )
> Important part of the change is that usb-device-manager on Windows
> does not create its own libusb context and uses one created by
> win-usb-dev (as libusb device must be enumerated and redirected
> with the same libusb context).
>
> Signed-off-by: Yuri Benditovich
> ---
> src/usb-device-manager.c | 290 +++
> src/win-usb-dev.c| 254 ++
> src/win-usb-dev.h| 34 +
> 3 files changed, 126 insertions(+), 452 deletions(-)
>
> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> index 55b672b..009b15b 100644
> --- a/src/usb-device-manager.c
> +++ b/src/usb-device-manager.c
> @@ -33,7 +33,6 @@
>
> #if defined(G_OS_WIN32)
> #include "win-usb-dev.h"
> -#define USE_GUDEV /* win-usb-dev.h provides a fake gudev interface */
> #endif
>
> #include "channel-usbredir-priv.h"
> @@ -106,11 +105,10 @@ struct _SpiceUsbDeviceManagerPrivate {
> struct usbredirfilter_rule *redirect_on_connect_rules;
> int auto_conn_filter_rules_count;
> int redirect_on_connect_rules_count;
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
> GUdevClient *udev;
> -libusb_device **coldplug_list; /* Avoid needless reprobing during init */
> #else
> -gboolean redirecting; /* Handled by GUdevClient in the gudev case */
> +gboolean redirecting; /* Handled by GUdevClient in Windows */
> libusb_hotplug_callback_handle hp_handle;
> #endif
> #ifdef G_OS_WIN32
> @@ -141,11 +139,7 @@ typedef struct _SpiceUsbDeviceInfo {
> guint16 vid;
> guint16 pid;
> gboolean isochronous;
> -#ifdef G_OS_WIN32
> -guint8 state;
> -#else
> libusb_device *libdev;
> -#endif
> gintref;
> } SpiceUsbDeviceInfo;
>
> @@ -156,13 +150,11 @@ static void channel_destroy(SpiceSession *session,
> SpiceChannel *channel,
> gpointer user_data);
> static void channel_event(SpiceChannel *channel, SpiceChannelEvent event,
>gpointer user_data);
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
> static void spice_usb_device_manager_uevent_cb(GUdevClient *client,
> - const gchar *action,
> - GUdevDevice *udevice,
> + libusb_device *libdev,
> + gboolean add,
> gpointer user_data);
> -static void spice_usb_device_manager_add_udev(SpiceUsbDeviceManager *self,
> - GUdevDevice*udev);
> #else
> static int spice_usb_device_manager_hotplug_cb(libusb_context *ctx,
> libusb_device*device,
> @@ -210,7 +202,7 @@ G_DEFINE_BOXED_TYPE(SpiceUsbDevice, spice_usb_device,
> static void
> _set_redirecting(SpiceUsbDeviceManager *self, gboolean is_redirecting)
> {
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
> g_object_set(self->priv->udev, "redirecting", is_redirecting, NULL);
> #else
> self->priv->redirecting = is_redirecting;
> @@ -235,7 +227,7 @@ gboolean
> spice_usb_device_manager_is_redirecting(SpiceUsbDeviceManager *self)
> {
> #ifdef USE_USBREDIR
>
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
> gboolean redirecting;
> g_object_get(self->priv->udev, "redirecting", , NULL);
> return redirecting;
> @@ -286,11 +278,20 @@ static gboolean
> spice_usb_device_manager_initable_init(GInitable *initable,
> SpiceUsbDeviceManagerPrivate *priv = self->priv;
> GList *list;
> GList
[Spice-devel] [spice-gtk] usb-redir: use persistent libusb device on Windows
Discard unneeded GUDev simulation and use persistent libusb
device pointer in SpiceUsbDevice on Windows as we do on Linux.
This removes significant part of differences between Linux and
Windows code. GUDevClient in win-usb-dev.c now maintains list
of libusb_device pointers and passes new and removed libusb devices
to usb-device-manager via signal.
Important part of the change is that usb-device-manager on Windows
does not create its own libusb context and uses one created by
win-usb-dev (as libusb device must be enumerated and redirected
with the same libusb context).
Signed-off-by: Yuri Benditovich
---
src/usb-device-manager.c | 290 +++
src/win-usb-dev.c| 254 ++
src/win-usb-dev.h| 34 +
3 files changed, 126 insertions(+), 452 deletions(-)
diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
index 55b672b..009b15b 100644
--- a/src/usb-device-manager.c
+++ b/src/usb-device-manager.c
@@ -33,7 +33,6 @@
#if defined(G_OS_WIN32)
#include "win-usb-dev.h"
-#define USE_GUDEV /* win-usb-dev.h provides a fake gudev interface */
#endif
#include "channel-usbredir-priv.h"
@@ -106,11 +105,10 @@ struct _SpiceUsbDeviceManagerPrivate {
struct usbredirfilter_rule *redirect_on_connect_rules;
int auto_conn_filter_rules_count;
int redirect_on_connect_rules_count;
-#ifdef USE_GUDEV
+#ifdef G_OS_WIN32
GUdevClient *udev;
-libusb_device **coldplug_list; /* Avoid needless reprobing during init */
#else
-gboolean redirecting; /* Handled by GUdevClient in the gudev case */
+gboolean redirecting; /* Handled by GUdevClient in Windows */
libusb_hotplug_callback_handle hp_handle;
#endif
#ifdef G_OS_WIN32
@@ -141,11 +139,7 @@ typedef struct _SpiceUsbDeviceInfo {
guint16 vid;
guint16 pid;
gboolean isochronous;
-#ifdef G_OS_WIN32
-guint8 state;
-#else
libusb_device *libdev;
-#endif
gintref;
} SpiceUsbDeviceInfo;
@@ -156,13 +150,11 @@ static void channel_destroy(SpiceSession *session,
SpiceChannel *channel,
gpointer user_data);
static void channel_event(SpiceChannel *channel, SpiceChannelEvent event,
gpointer user_data);
-#ifdef USE_GUDEV
+#ifdef G_OS_WIN32
static void spice_usb_device_manager_uevent_cb(GUdevClient *client,
- const gchar *action,
- GUdevDevice *udevice,
+ libusb_device *libdev,
+ gboolean add,
gpointer user_data);
-static void spice_usb_device_manager_add_udev(SpiceUsbDeviceManager *self,
- GUdevDevice*udev);
#else
static int spice_usb_device_manager_hotplug_cb(libusb_context *ctx,
libusb_device*device,
@@ -210,7 +202,7 @@ G_DEFINE_BOXED_TYPE(SpiceUsbDevice, spice_usb_device,
static void
_set_redirecting(SpiceUsbDeviceManager *self, gboolean is_redirecting)
{
-#ifdef USE_GUDEV
+#ifdef G_OS_WIN32
g_object_set(self->priv->udev, "redirecting", is_redirecting, NULL);
#else
self->priv->redirecting = is_redirecting;
@@ -235,7 +227,7 @@ gboolean
spice_usb_device_manager_is_redirecting(SpiceUsbDeviceManager *self)
{
#ifdef USE_USBREDIR
-#ifdef USE_GUDEV
+#ifdef G_OS_WIN32
gboolean redirecting;
g_object_get(self->priv->udev, "redirecting", , NULL);
return redirecting;
@@ -286,11 +278,20 @@ static gboolean
spice_usb_device_manager_initable_init(GInitable *initable,
SpiceUsbDeviceManagerPrivate *priv = self->priv;
GList *list;
GList *it;
-int rc;
-#ifdef USE_GUDEV
-const gchar *const subsystems[] = {"usb", NULL};
-#endif
+/* Start listening for usb devices plug / unplug */
+#ifdef G_OS_WIN32
+/* libusb_init called by win-usb-dev */
+priv->udev = g_udev_client_new(>context);
+if (priv->udev == NULL) {
+g_warning("Error initializing GUdevClient");
+return FALSE;
+}
+g_signal_connect(G_OBJECT(priv->udev), "uevent",
+ G_CALLBACK(spice_usb_device_manager_uevent_cb), self);
+g_udev_client_report_devices(priv->udev);
+#else
+int rc;
/* Initialize libusb */
rc = libusb_init(>context);
if (rc < 0) {
@@ -300,33 +301,6 @@ static gboolean
spice_usb_device_manager_initable_init(GInitable *initable,
"Error initializing USB support: %s [%i]", desc, rc);
return FALSE;
}
-
-#ifdef G_OS_WIN32
-#if LIBUSB_API_VERSION >= 0x01000106
-libusb_set_option(priv->context, LIBUSB_OPTION_USE_USBDK);
-#endif
-#endif
-
-/* Start listening for usb devices plug / unplug */
-#ifdef USE_GUDEV
-priv->udev = g_udev_client_new(subsystems);
-if (priv->udev
