[Spice-devel] [spice-gtk v2 1/1] usb-redirection: implementation of usb backend layer
This layer communicates with libusb and libusbredir and
provides the API for USB redirection procedures.
All the modules of spice-gtk communicate only with usb
backend instead of calling libusb and usbredirhost directly.
This is prerequisite of further implementation of
cd-sharing via USB redirection.
Signed-off-by: Yuri Benditovich
---
src/Makefile.am | 2 +
src/channel-usbredir-priv.h | 9 +-
src/channel-usbredir.c| 232
src/meson.build | 1 +
src/usb-backend-common.c | 688 ++
src/usb-backend.h | 115 ++
src/usb-device-manager-priv.h | 3 +-
src/usb-device-manager.c | 398 +++-
src/usb-device-manager.h | 1 +
src/usbutil.c | 36 --
src/usbutil.h | 2 -
src/win-usb-dev.c | 59 +--
12 files changed, 1044 insertions(+), 502 deletions(-)
create mode 100644 src/usb-backend-common.c
create mode 100644 src/usb-backend.h
diff --git a/src/Makefile.am b/src/Makefile.am
index 1bb6f9b..3431459 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -253,6 +253,8 @@ libspice_client_glib_2_0_la_SOURCES =
\
spice-uri-priv.h\
usb-device-manager.c\
usb-device-manager-priv.h \
+ usb-backend.h \
+ usb-backend-common.c\
usbutil.c \
usbutil.h \
$(USB_ACL_HELPER_SRCS) \
diff --git a/src/channel-usbredir-priv.h b/src/channel-usbredir-priv.h
index 17e9716..4064f36 100644
--- a/src/channel-usbredir-priv.h
+++ b/src/channel-usbredir-priv.h
@@ -21,9 +21,8 @@
#ifndef __SPICE_CLIENT_USBREDIR_CHANNEL_PRIV_H__
#define __SPICE_CLIENT_USBREDIR_CHANNEL_PRIV_H__
-#include
-#include
#include "spice-client.h"
+#include "usb-backend.h"
G_BEGIN_DECLS
@@ -31,7 +30,7 @@ G_BEGIN_DECLS
context should not be destroyed before the last device has been
disconnected */
void spice_usbredir_channel_set_context(SpiceUsbredirChannel *channel,
-libusb_context *context);
+SpiceUsbBackend *context);
void spice_usbredir_channel_disconnect_device_async(SpiceUsbredirChannel
*channel,
GCancellable *cancellable,
@@ -46,7 +45,7 @@ gboolean
spice_usbredir_channel_disconnect_device_finish(SpiceUsbredirChannel *c
(through spice_channel_connect()), before calling this. */
void spice_usbredir_channel_connect_device_async(
SpiceUsbredirChannel *channel,
-libusb_device*device,
+SpiceUsbBackendDevice *device,
SpiceUsbDevice *spice_device,
GCancellable *cancellable,
GAsyncReadyCallback callback,
@@ -58,7 +57,7 @@ gboolean spice_usbredir_channel_connect_device_finish(
void spice_usbredir_channel_disconnect_device(SpiceUsbredirChannel *channel);
-libusb_device *spice_usbredir_channel_get_device(SpiceUsbredirChannel
*channel);
+SpiceUsbBackendDevice *spice_usbredir_channel_get_device(SpiceUsbredirChannel
*channel);
void spice_usbredir_channel_lock(SpiceUsbredirChannel *channel);
diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
index 182edc4..0eba954 100644
--- a/src/channel-usbredir.c
+++ b/src/channel-usbredir.c
@@ -23,7 +23,6 @@
#ifdef USE_USBREDIR
#include
-#include
#ifdef USE_LZ4
#include
#endif
@@ -66,15 +65,12 @@ enum SpiceUsbredirChannelState {
};
struct _SpiceUsbredirChannelPrivate {
-libusb_device *device;
+SpiceUsbBackendDevice *device;
SpiceUsbDevice *spice_device;
-libusb_context *context;
-struct usbredirhost *host;
+SpiceUsbBackend *context;
+SpiceUsbBackendChannel *host;
/* To catch usbredirhost error messages and report them as a GError */
GError **catch_error;
-/* Data passed from channel handle msg to the usbredirhost read cb */
-const uint8_t *read_buf;
-int read_buf_size;
enum SpiceUsbredirChannelState state;
#ifdef USE_POLKIT
GTask *task;
@@ -90,18 +86,10 @@ static void spice_usbredir_channel_dispose(GObject *obj);
static void spice_usbredir_channel_finalize(GObject *obj);
static void usbredir_handle_msg(SpiceChannel *channel, SpiceMsgIn *in);
-static void usbredir_log(void *user_data, int level, const char *msg);
-static int usbredir_read_callback(void *user_data, uint8_t *data, int count);
+static void usbredir_error(void *user_data, const char *msg);
static int usbredir_write_callback(void *user_data, uint8_t *data, in
Re: [Spice-devel] [spice-gtk v2 1/1] usb-redirection: implementation of usb backend layer
On Sun, Sep 30, 2018 at 06:13:04PM +0300, Yuri Benditovich wrote:
> This layer communicates with libusb and libusbredir and
> provides the API for USB redirection procedures.
> All the modules of spice-gtk communicate only with usb
> backend instead of calling libusb and usbredirhost directly.
> This is prerequisite of further implementation of
> cd-sharing via USB redirection.
>
> Signed-off-by: Yuri Benditovich
> ---
> src/Makefile.am | 2 +
> src/channel-usbredir-priv.h | 9 +-
> src/channel-usbredir.c| 232
> src/meson.build | 1 +
> src/usb-backend-common.c | 688 ++
> src/usb-backend.h | 115 ++
> src/usb-device-manager-priv.h | 3 +-
> src/usb-device-manager.c | 398 +++-
> src/usb-device-manager.h | 1 +
> src/usbutil.c | 36 --
> src/usbutil.h | 2 -
> src/win-usb-dev.c | 59 +--
> 12 files changed, 1044 insertions(+), 502 deletions(-)
> create mode 100644 src/usb-backend-common.c
> create mode 100644 src/usb-backend.h
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 1bb6f9b..3431459 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -253,6 +253,8 @@ libspice_client_glib_2_0_la_SOURCES =
> \
> spice-uri-priv.h\
> usb-device-manager.c\
> usb-device-manager-priv.h \
> + usb-backend.h \
> + usb-backend-common.c\
> usbutil.c \
> usbutil.h \
> $(USB_ACL_HELPER_SRCS) \
> diff --git a/src/channel-usbredir-priv.h b/src/channel-usbredir-priv.h
> index 17e9716..4064f36 100644
> --- a/src/channel-usbredir-priv.h
> +++ b/src/channel-usbredir-priv.h
> @@ -21,9 +21,8 @@
> #ifndef __SPICE_CLIENT_USBREDIR_CHANNEL_PRIV_H__
> #define __SPICE_CLIENT_USBREDIR_CHANNEL_PRIV_H__
>
> -#include
> -#include
> #include "spice-client.h"
> +#include "usb-backend.h"
>
> G_BEGIN_DECLS
>
> @@ -31,7 +30,7 @@ G_BEGIN_DECLS
> context should not be destroyed before the last device has been
> disconnected */
> void spice_usbredir_channel_set_context(SpiceUsbredirChannel *channel,
> -libusb_context *context);
> +SpiceUsbBackend *context);
>
> void spice_usbredir_channel_disconnect_device_async(SpiceUsbredirChannel
> *channel,
> GCancellable
> *cancellable,
> @@ -46,7 +45,7 @@ gboolean
> spice_usbredir_channel_disconnect_device_finish(SpiceUsbredirChannel *c
> (through spice_channel_connect()), before calling this. */
> void spice_usbredir_channel_connect_device_async(
> SpiceUsbredirChannel *channel,
> -libusb_device*device,
> +SpiceUsbBackendDevice *device,
> SpiceUsbDevice *spice_device,
> GCancellable *cancellable,
> GAsyncReadyCallback callback,
> @@ -58,7 +57,7 @@ gboolean spice_usbredir_channel_connect_device_finish(
>
> void spice_usbredir_channel_disconnect_device(SpiceUsbredirChannel *channel);
>
> -libusb_device *spice_usbredir_channel_get_device(SpiceUsbredirChannel
> *channel);
> +SpiceUsbBackendDevice
> *spice_usbredir_channel_get_device(SpiceUsbredirChannel *channel);
>
> void spice_usbredir_channel_lock(SpiceUsbredirChannel *channel);
>
> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> index 182edc4..0eba954 100644
> --- a/src/channel-usbredir.c
> +++ b/src/channel-usbredir.c
> @@ -23,7 +23,6 @@
>
> #ifdef USE_USBREDIR
> #include
> -#include
> #ifdef USE_LZ4
> #include
> #endif
> @@ -66,15 +65,12 @@ enum SpiceUsbredirChannelState {
> };
>
> struct _SpiceUsbredirChannelPrivate {
> -libusb_device *device;
> +SpiceUsbBackendDevice *device;
> SpiceUsbDevice *spice_device;
> -libusb_context *context;
> -struct usbredirhost *host;
> +SpiceUsbBackend *context;
> +SpiceUsbBackendChannel *host;
> /* To catch usbredirhost error messages and report them as a GError */
> GError **catch_error;
> -/* Data passed from channel handle msg to the usbredirhost read cb */
> -const uint8_t *read_buf;
> -int read_buf_size;
> enum SpiceUsbredirChannelState state;
> #ifdef USE_POLKIT
> GTask *task;
> @@ -90,18 +86,10 @@ static void spice_usbredir_channel_dispose(GObject *obj);
> static void spice_usbredir_channel_finalize(GObject *obj);
> static void usbredir_handle_msg(SpiceChannel *channel, SpiceMsgIn *in
Re: [Spice-devel] [spice-gtk v2 1/1] usb-redirection: implementation of usb backend layer
Hey,
On Mon, Jan 07, 2019 at 03:50:12PM +0200, Yuri Benditovich wrote:
> >
> > > if (!priv->host)
> > > -g_error("Out of memory allocating usbredirhost");
> > > +g_error("Out of memory initializing redirection support");
> > >
> > > -#if USBREDIR_VERSION >= 0x000701
> > > -usbredirhost_set_buffered_output_size_cb(priv->host,
> > usbredir_buffered_output_size_callback);
> > > -#endif
> > > #ifdef USE_LZ4
> > > spice_channel_set_capability(channel,
> > SPICE_SPICEVMC_CAP_DATA_COMPRESS_LZ4);
> > > #endif
> > > @@ -299,8 +279,6 @@ static gboolean spice_usbredir_channel_open_device(
> > > {
> > > SpiceUsbredirChannelPrivate *priv = channel->priv;
> > > SpiceSession *session;
> > > -libusb_device_handle *handle = NULL;
> > > -int rc, status;
> > > SpiceUsbDeviceManager *manager;
> > >
> > > g_return_val_if_fail(priv->state == STATE_DISCONNECTED
> > > @@ -309,21 +287,16 @@ static gboolean spice_usbredir_channel_open_device(
> > > #endif
> > > , FALSE);
> > >
> > > -rc = libusb_open(priv->device, &handle);
> > > -if (rc != 0) {
> > > -g_set_error(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> > > -"Could not open usb device: %s [%i]",
> > > -spice_usbutil_libusb_strerror(rc), rc);
> > > -return FALSE;
> > > -}
> > > -
> > > priv->catch_error = err;
> > > -status = usbredirhost_set_device(priv->host, handle);
> > > -priv->catch_error = NULL;
> > > -if (status != usb_redir_success) {
> > > -g_return_val_if_fail(err == NULL || *err != NULL, FALSE);
> > > +if (!spice_usb_backend_channel_attach(priv->host, priv->device,
> > err)) {
> > > +priv->catch_error = NULL;
> > > +if (*err == NULL) {
> > > +g_set_error(err, SPICE_CLIENT_ERROR,
> > SPICE_CLIENT_ERROR_FAILED,
> > > +"Error attaching device: (no error information)");
> >
> > You could line up the "Error .." with the opening parenthesis.
> >
> >
> This is not described exactly in Spice coding style document and existing
> code of spice-gtk
> uses different solution for indentation, so this is point of personal
> preference.
From a quick look, aligning multiple line arguments lists seem much more
common that then opposite. See the g_set_error call that this hunk is
removing for example.
> > > @@ -362,8 +335,7 @@ static void spice_usbredir_channel_open_acl_cb(
> > > spice_usbredir_channel_open_device(channel, &err);
> > > }
> > > if (err) {
> > > -libusb_unref_device(priv->device);
> > > -priv->device = NULL;
> > > +g_clear_pointer(&priv->device,
> > spice_usb_backend_device_release);
> >
> > _unref rather than _release?
> >
>
> This can be changed in V3, although I would prefer not to change it.
> Semantics of ref/unref and acquire/release are not different.
> Please let me know whether this change in mandatory.
I assume you mean that the semantics of ref/unref and acquire/release
*are* different? If this is what you mean, how are they different?
If the semantics are the same, then let's use the same name.
> > > @@ -446,7 +420,8 @@ void spice_usbredir_channel_connect_device_async(
> > > goto done;
> > > }
> > >
> > > -priv->device = libusb_ref_device(device);
> > > +spice_usb_backend_device_acquire(device);
> > > +priv->device = device;
> >
> > You could mimic libusb API actually,
> > priv->device = spice_usb_backend_device_ref(device);
> >
>
> I do not think I need to mimic libusb or something other.
> Please let me know whether this change is mandatory.
libusb_ref_device, g_object_ref all return a pointer to the new ref, and
this makes that code slightly simpler, so just a suggestion in case you
think that's a good change.
> >
> > > +void *msc;
> > > +} d;
> > > +gboolean is_libusb;
> > > +gint ref_count;
> > > +SpiceUsbBackendChannel *attached_to;
> >
> > You don't need 'msc' just yet, nor 'is_libusb', and I'm not sure about
> > 'attached_to'
> >
>
> msc can be changed to reserved if you do not like it so much.
> removal of is_libusb makes further merges harder (and current commit is
> only prerequisite for further merge).
> attached_to is useful for debugging/support
> Please let me know whether this is a requirement.
This patch is refactoring the existing code so that we can add more
stuff on top of it. It should not be adding new things, especially if
they are not used in the current commit. attached_to can be added in a
separate commit with the debugging checks that it lets us do, msc can be
added when it's needed.
>
> >
> > > +void *hotplug_user_data;
> > > +libusb_hotplug_callback_handle hotplug_handle;
> > > +};
> > > +
> > > +struct _SpiceUsbBackendChannel
> > > +{
> > > +struct usbredirhost *usbredirhost;
> > > +uint8_t *read_buf;
> > > +int read_buf_size;
> > > +struct usbredirfilter_rule *rules
Re: [Spice-devel] [spice-gtk v2 1/1] usb-redirection: implementation of usb backend layer
On Mon, Jan 07, 2019 at 03:50:12PM +0200, Yuri Benditovich wrote: > On Mon, Nov 26, 2018 at 7:41 PM Christophe Fergeau > wrote: > > In _SpiceUsbDeviceManagerPrivate, you replaced > > #ifndef G_OS_WIN32 > > libusb_device *libdev; > > #endif > > > > with > > > > #ifndef G_OS_WIN32 > > SpiceUsbBackendDevice *bdev; > > #endif > > > > The #ifdef is there because of this comment in > > spice_usb_device_manager_device_to_bdev: > > > > /* > >* On win32 we need to do this the hard and slow way, by asking libusb to > >* re-enumerate all devices and then finding a matching device. > >* We cannot cache the libusb_device like we do under Linux since the > >* driver swap we do under windows invalidates the cached libdev. > >*/ > > > > After your patch, spice_usb_device_manager_device_to_bdev is no longer > > at the right level of indirection imo, it looks up the 'bdev' when > > needed on Windows in usb-device-manager.c, but my understanding of that > > comment is that any libusb call within SpiceUsbBackendDevice should not > > use a cached libusb_device? > > > > > > > As fas as I understand, there is no change in the level of indirection. > Previously was: > On Linux: usb-device-manager receives libusb device on hotplug indication > and uses it until the device disappears. > On Windows: each time the usb-device-manager needs the libusb device it > takes fresh list of libusb devices and finds one according to known device > properties. > > Now: > On Linux: usb-device-manager receives backend device wrapping libusb device > on hotplug indication and uses it until the device disappears > On Windows: each time the usb-device-manager needs the backend device it > takes fresh list of new backend devices and finds one according to known > device properties. > So, the backend device is always fresh one and it always have fresh libusb > device. > > This can be simplified with removal of all these lookups in the > usb-device-manager (and I already illustrated how) but after this commit is > done. > If from your point of view this is the requirement to existing commit, > please let me know (then we will need to reevaluate our plans and our > ability to deliver the patches in reasonable time). I'll quote again the comment: /* * On win32 we need to do this the hard and slow way, by asking libusb to * re-enumerate all devices and then finding a matching device. * We cannot cache the libusb_device like we do under Linux since the * driver swap we do under windows invalidates the cached libdev. */ It says we cannot cache a libusb_device because it can change behind our back, so we need to reenumerate all devices and lookup the right libusb_device before every call to libusb. Before your changes, we did not hold any long-lasting references to a libusb_device on Windows, so we were guaranteed to do that lookup before every call. After your changes, SpiceUsbBackendDevice is holding a long-lasting reference to a libusb_device, but does not care about the win32 issue. Only usb-device-manager is making sure the libusb_device it's going to use is valid. This does not make sense to me to leak this implementation detail to usb-device-manager. SpiceUsbBackend should hide it, otherwise its API is going to be error-prone. When you say this can be simplified by removing this lookup, are you confirming that this comment I quoted above is obsolete? 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 v2 1/1] usb-redirection: implementation of usb backend layer
On Wed, Jan 16, 2019 at 6:11 PM Christophe Fergeau wrote: > On Mon, Jan 07, 2019 at 03:50:12PM +0200, Yuri Benditovich wrote: > > On Mon, Nov 26, 2018 at 7:41 PM Christophe Fergeau > > wrote: > > > In _SpiceUsbDeviceManagerPrivate, you replaced > > > #ifndef G_OS_WIN32 > > > libusb_device *libdev; > > > #endif > > > > > > with > > > > > > #ifndef G_OS_WIN32 > > > SpiceUsbBackendDevice *bdev; > > > #endif > > > > > > The #ifdef is there because of this comment in > > > spice_usb_device_manager_device_to_bdev: > > > > > > /* > > >* On win32 we need to do this the hard and slow way, by asking > libusb to > > >* re-enumerate all devices and then finding a matching device. > > >* We cannot cache the libusb_device like we do under Linux since the > > >* driver swap we do under windows invalidates the cached libdev. > > >*/ > > > > > > After your patch, spice_usb_device_manager_device_to_bdev is no longer > > > at the right level of indirection imo, it looks up the 'bdev' when > > > needed on Windows in usb-device-manager.c, but my understanding of that > > > comment is that any libusb call within SpiceUsbBackendDevice should not > > > use a cached libusb_device? > > > > > > > > > > > As fas as I understand, there is no change in the level of indirection. > > Previously was: > > On Linux: usb-device-manager receives libusb device on hotplug indication > > and uses it until the device disappears. > > On Windows: each time the usb-device-manager needs the libusb device it > > takes fresh list of libusb devices and finds one according to known > device > > properties. > > > > Now: > > On Linux: usb-device-manager receives backend device wrapping libusb > device > > on hotplug indication and uses it until the device disappears > > On Windows: each time the usb-device-manager needs the backend device it > > takes fresh list of new backend devices and finds one according to known > > device properties. > > So, the backend device is always fresh one and it always have fresh > libusb > > device. > > > > This can be simplified with removal of all these lookups in the > > usb-device-manager (and I already illustrated how) but after this commit > is > > done. > > If from your point of view this is the requirement to existing commit, > > please let me know (then we will need to reevaluate our plans and our > > ability to deliver the patches in reasonable time). > > I'll quote again the comment: > >/* > * On win32 we need to do this the hard and slow way, by asking libusb > to > * re-enumerate all devices and then finding a matching device. > * We cannot cache the libusb_device like we do under Linux since the > * driver swap we do under windows invalidates the cached libdev. > */ > > It says we cannot cache a libusb_device because it can change behind our > back, so we need to reenumerate all devices and lookup the right > libusb_device before every call to libusb. > Before your changes, we did not hold any long-lasting references to a > libusb_device on Windows, so we were guaranteed to do that lookup before > every call. After your changes, SpiceUsbBackendDevice is holding a > long-lasting reference to a libusb_device, but does not care about the > win32 issue. Only usb-device-manager is making sure the libusb_device > it's going to use is valid. This does not make sense to me to leak this > implementation detail to usb-device-manager. SpiceUsbBackend should hide > it, otherwise its API is going to be error-prone. > > When you say this can be simplified by removing this lookup, are you > confirming that this comment I quoted above is obsolete? > This comment, IMO, is wrong and permanent enumeration is not mandatory. None of existing objects can disappear if it is is referenced. But the object must be used with proper libusb context. Changes in existing patch are intentionally minimal to allow comparison between previous and existing code. After the current patch is merged I will be able to solve the problem of persistency as I should be, I believe (together with unification of the structure holding device properties). But mixing all these things together discards the current patch and I can't predict the time frame of next round. > Christophe > ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk v2 1/1] usb-redirection: implementation of usb backend layer
On Fri, Jan 18, 2019 at 10:23:04AM +0200, Yuri Benditovich wrote: > On Wed, Jan 16, 2019 at 6:11 PM Christophe Fergeau > wrote: > > > On Mon, Jan 07, 2019 at 03:50:12PM +0200, Yuri Benditovich wrote: > > > On Mon, Nov 26, 2018 at 7:41 PM Christophe Fergeau > > > wrote: > > > > In _SpiceUsbDeviceManagerPrivate, you replaced > > > > #ifndef G_OS_WIN32 > > > > libusb_device *libdev; > > > > #endif > > > > > > > > with > > > > > > > > #ifndef G_OS_WIN32 > > > > SpiceUsbBackendDevice *bdev; > > > > #endif > > > > > > > > The #ifdef is there because of this comment in > > > > spice_usb_device_manager_device_to_bdev: > > > > > > > > /* > > > >* On win32 we need to do this the hard and slow way, by asking > > libusb to > > > >* re-enumerate all devices and then finding a matching device. > > > >* We cannot cache the libusb_device like we do under Linux since the > > > >* driver swap we do under windows invalidates the cached libdev. > > > >*/ > > > > > > > > After your patch, spice_usb_device_manager_device_to_bdev is no longer > > > > at the right level of indirection imo, it looks up the 'bdev' when > > > > needed on Windows in usb-device-manager.c, but my understanding of that > > > > comment is that any libusb call within SpiceUsbBackendDevice should not > > > > use a cached libusb_device? > > > > > > > > > > > > > > > As fas as I understand, there is no change in the level of indirection. > > > Previously was: > > > On Linux: usb-device-manager receives libusb device on hotplug indication > > > and uses it until the device disappears. > > > On Windows: each time the usb-device-manager needs the libusb device it > > > takes fresh list of libusb devices and finds one according to known > > device > > > properties. > > > > > > Now: > > > On Linux: usb-device-manager receives backend device wrapping libusb > > device > > > on hotplug indication and uses it until the device disappears > > > On Windows: each time the usb-device-manager needs the backend device it > > > takes fresh list of new backend devices and finds one according to known > > > device properties. > > > So, the backend device is always fresh one and it always have fresh > > libusb > > > device. > > > > > > This can be simplified with removal of all these lookups in the > > > usb-device-manager (and I already illustrated how) but after this commit > > is > > > done. > > > If from your point of view this is the requirement to existing commit, > > > please let me know (then we will need to reevaluate our plans and our > > > ability to deliver the patches in reasonable time). > > > > I'll quote again the comment: > > > >/* > > * On win32 we need to do this the hard and slow way, by asking libusb > > to > > * re-enumerate all devices and then finding a matching device. > > * We cannot cache the libusb_device like we do under Linux since the > > * driver swap we do under windows invalidates the cached libdev. > > */ > > > > It says we cannot cache a libusb_device because it can change behind our > > back, so we need to reenumerate all devices and lookup the right > > libusb_device before every call to libusb. > > Before your changes, we did not hold any long-lasting references to a > > libusb_device on Windows, so we were guaranteed to do that lookup before > > every call. After your changes, SpiceUsbBackendDevice is holding a > > long-lasting reference to a libusb_device, but does not care about the > > win32 issue. Only usb-device-manager is making sure the libusb_device > > it's going to use is valid. This does not make sense to me to leak this > > implementation detail to usb-device-manager. SpiceUsbBackend should hide > > it, otherwise its API is going to be error-prone. > > > > When you say this can be simplified by removing this lookup, are you > > confirming that this comment I quoted above is obsolete? > > > > This comment, IMO, is wrong and permanent enumeration is not mandatory. > None of existing objects can disappear if it is is referenced. The libusb object does not disappear, but apparently there were some situations where the device 'moved', and the libusb object pointed to a no-longer existing device, thus the need to enumerate again. I believe this is related to https://gitlab.freedesktop.org/spice/spice-gtk/commit/76e94509cf29a78aa39740c81dcdd2eee355c7b9 https://bugzilla.redhat.com/show_bug.cgi?id=842816 > Changes in existing patch are intentionally minimal to allow > comparison between previous and existing code. After the current patch > is merged I will be able to solve the problem of persistency as I > should be, I believe (together with unification of the structure > holding device properties). But mixing all these things together > discards the current patch and I can't predict the time frame of next > round. I'm not suggesting to squash everything in the current patch, but we are not limited to one single patch. If you intend to remove that persistency code right aft
Re: [Spice-devel] [spice-gtk v2 1/1] usb-redirection: implementation of usb backend layer
On Tue, Jan 15, 2019 at 4:52 PM Christophe Fergeau
wrote:
> Hey,
>
> On Mon, Jan 07, 2019 at 03:50:12PM +0200, Yuri Benditovich wrote:
> > >
> > > > if (!priv->host)
> > > > -g_error("Out of memory allocating usbredirhost");
> > > > +g_error("Out of memory initializing redirection support");
> > > >
> > > > -#if USBREDIR_VERSION >= 0x000701
> > > > -usbredirhost_set_buffered_output_size_cb(priv->host,
> > > usbredir_buffered_output_size_callback);
> > > > -#endif
> > > > #ifdef USE_LZ4
> > > > spice_channel_set_capability(channel,
> > > SPICE_SPICEVMC_CAP_DATA_COMPRESS_LZ4);
> > > > #endif
> > > > @@ -299,8 +279,6 @@ static gboolean
> spice_usbredir_channel_open_device(
> > > > {
> > > > SpiceUsbredirChannelPrivate *priv = channel->priv;
> > > > SpiceSession *session;
> > > > -libusb_device_handle *handle = NULL;
> > > > -int rc, status;
> > > > SpiceUsbDeviceManager *manager;
> > > >
> > > > g_return_val_if_fail(priv->state == STATE_DISCONNECTED
> > > > @@ -309,21 +287,16 @@ static gboolean
> spice_usbredir_channel_open_device(
> > > > #endif
> > > > , FALSE);
> > > >
> > > > -rc = libusb_open(priv->device, &handle);
> > > > -if (rc != 0) {
> > > > -g_set_error(err, SPICE_CLIENT_ERROR,
> SPICE_CLIENT_ERROR_FAILED,
> > > > -"Could not open usb device: %s [%i]",
> > > > -spice_usbutil_libusb_strerror(rc), rc);
> > > > -return FALSE;
> > > > -}
> > > > -
> > > > priv->catch_error = err;
> > > > -status = usbredirhost_set_device(priv->host, handle);
> > > > -priv->catch_error = NULL;
> > > > -if (status != usb_redir_success) {
> > > > -g_return_val_if_fail(err == NULL || *err != NULL, FALSE);
> > > > +if (!spice_usb_backend_channel_attach(priv->host, priv->device,
> > > err)) {
> > > > +priv->catch_error = NULL;
> > > > +if (*err == NULL) {
> > > > +g_set_error(err, SPICE_CLIENT_ERROR,
> > > SPICE_CLIENT_ERROR_FAILED,
> > > > +"Error attaching device: (no error information)");
> > >
> > > You could line up the "Error .." with the opening parenthesis.
> > >
> > >
> > This is not described exactly in Spice coding style document and existing
> > code of spice-gtk
> > uses different solution for indentation, so this is point of personal
> > preference.
>
> From a quick look, aligning multiple line arguments lists seem much more
> common that then opposite. See the g_set_error call that this hunk is
> removing for example.
>
> > > > @@ -362,8 +335,7 @@ static void spice_usbredir_channel_open_acl_cb(
> > > > spice_usbredir_channel_open_device(channel, &err);
> > > > }
> > > > if (err) {
> > > > -libusb_unref_device(priv->device);
> > > > -priv->device = NULL;
> > > > +g_clear_pointer(&priv->device,
> > > spice_usb_backend_device_release);
> > >
> > > _unref rather than _release?
> > >
> >
> > This can be changed in V3, although I would prefer not to change it.
> > Semantics of ref/unref and acquire/release are not different.
> > Please let me know whether this change in mandatory.
>
> I assume you mean that the semantics of ref/unref and acquire/release
> *are* different? If this is what you mean, how are they different?
> If the semantics are the same, then let's use the same name.
>
No, I mean that this is the same and there is no difference between
ref/unref
and acquire/release; ref/unref is used in glib, so I prefer to use
acquire/release.
I do not have a goal to mimic libusb, I rather prefer to remove any
knowledge
about libusb from all the parts of spice-gtk except of backend code.
I've asked whether this change is mandatory, i.e. if this leads the
rejection of the patch.
Please respond, preferrably yes or no.
>
> > > > @@ -446,7 +420,8 @@ void spice_usbredir_channel_connect_device_async(
> > > > goto done;
> > > > }
> > > >
> > > > -priv->device = libusb_ref_device(device);
> > > > +spice_usb_backend_device_acquire(device);
> > > > +priv->device = device;
> > >
> > > You could mimic libusb API actually,
> > > priv->device = spice_usb_backend_device_ref(device);
> > >
> >
> > I do not think I need to mimic libusb or something other.
> > Please let me know whether this change is mandatory.
>
> libusb_ref_device, g_object_ref all return a pointer to the new ref, and
> this makes that code slightly simpler, so just a suggestion in case you
> think that's a good change.
>
> > >
> > > > +void *msc;
> > > > +} d;
> > > > +gboolean is_libusb;
> > > > +gint ref_count;
> > > > +SpiceUsbBackendChannel *attached_to;
> > >
> > > You don't need 'msc' just yet, nor 'is_libusb', and I'm not sure about
> > > 'attached_to'
> > >
> >
> > msc can be changed to reserved if you do not like it so much.
> > removal of is_libusb makes further merges harder (and current commit is
> > only prerequisite for further
Re: [Spice-devel] [spice-gtk v2 1/1] usb-redirection: implementation of usb backend layer
On Fri, Jan 18, 2019 at 3:53 PM Christophe Fergeau wrote: > On Fri, Jan 18, 2019 at 10:23:04AM +0200, Yuri Benditovich wrote: > > On Wed, Jan 16, 2019 at 6:11 PM Christophe Fergeau > > wrote: > > > > > On Mon, Jan 07, 2019 at 03:50:12PM +0200, Yuri Benditovich wrote: > > > > On Mon, Nov 26, 2018 at 7:41 PM Christophe Fergeau < > cferg...@redhat.com> > > > > wrote: > > > > > In _SpiceUsbDeviceManagerPrivate, you replaced > > > > > #ifndef G_OS_WIN32 > > > > > libusb_device *libdev; > > > > > #endif > > > > > > > > > > with > > > > > > > > > > #ifndef G_OS_WIN32 > > > > > SpiceUsbBackendDevice *bdev; > > > > > #endif > > > > > > > > > > The #ifdef is there because of this comment in > > > > > spice_usb_device_manager_device_to_bdev: > > > > > > > > > > /* > > > > >* On win32 we need to do this the hard and slow way, by asking > > > libusb to > > > > >* re-enumerate all devices and then finding a matching device. > > > > >* We cannot cache the libusb_device like we do under Linux > since the > > > > >* driver swap we do under windows invalidates the cached libdev. > > > > >*/ > > > > > > > > > > After your patch, spice_usb_device_manager_device_to_bdev is no > longer > > > > > at the right level of indirection imo, it looks up the 'bdev' when > > > > > needed on Windows in usb-device-manager.c, but my understanding of > that > > > > > comment is that any libusb call within SpiceUsbBackendDevice > should not > > > > > use a cached libusb_device? > > > > > > > > > > > > > > > > > > > As fas as I understand, there is no change in the level of > indirection. > > > > Previously was: > > > > On Linux: usb-device-manager receives libusb device on hotplug > indication > > > > and uses it until the device disappears. > > > > On Windows: each time the usb-device-manager needs the libusb device > it > > > > takes fresh list of libusb devices and finds one according to known > > > device > > > > properties. > > > > > > > > Now: > > > > On Linux: usb-device-manager receives backend device wrapping libusb > > > device > > > > on hotplug indication and uses it until the device disappears > > > > On Windows: each time the usb-device-manager needs the backend > device it > > > > takes fresh list of new backend devices and finds one according to > known > > > > device properties. > > > > So, the backend device is always fresh one and it always have fresh > > > libusb > > > > device. > > > > > > > > This can be simplified with removal of all these lookups in the > > > > usb-device-manager (and I already illustrated how) but after this > commit > > > is > > > > done. > > > > If from your point of view this is the requirement to existing > commit, > > > > please let me know (then we will need to reevaluate our plans and our > > > > ability to deliver the patches in reasonable time). > > > > > > I'll quote again the comment: > > > > > >/* > > > * On win32 we need to do this the hard and slow way, by asking > libusb > > > to > > > * re-enumerate all devices and then finding a matching device. > > > * We cannot cache the libusb_device like we do under Linux since > the > > > * driver swap we do under windows invalidates the cached libdev. > > > */ > > > > > > It says we cannot cache a libusb_device because it can change behind > our > > > back, so we need to reenumerate all devices and lookup the right > > > libusb_device before every call to libusb. > > > Before your changes, we did not hold any long-lasting references to a > > > libusb_device on Windows, so we were guaranteed to do that lookup > before > > > every call. After your changes, SpiceUsbBackendDevice is holding a > > > long-lasting reference to a libusb_device, but does not care about the > > > win32 issue. Only usb-device-manager is making sure the libusb_device > > > it's going to use is valid. This does not make sense to me to leak this > > > implementation detail to usb-device-manager. SpiceUsbBackend should > hide > > > it, otherwise its API is going to be error-prone. > > > > > > When you say this can be simplified by removing this lookup, are you > > > confirming that this comment I quoted above is obsolete? > > > > > > > This comment, IMO, is wrong and permanent enumeration is not mandatory. > > None of existing objects can disappear if it is is referenced. > > The libusb object does not disappear, but apparently there were some > situations where the device 'moved', and the libusb object pointed to a > no-longer existing device, thus the need to enumerate again. > I believe this is related to > > https://gitlab.freedesktop.org/spice/spice-gtk/commit/76e94509cf29a78aa39740c81dcdd2eee355c7b9 > https://bugzilla.redhat.com/show_bug.cgi?id=842816 This commit was discarder by further changes and related to some problem with WinUSB and there is no other information about it. If the device is moved from one bus to another one, this is regular PnP event and it should be processed as such. > > Changes in exist
Re: [Spice-devel] [spice-gtk v2 1/1] usb-redirection: implementation of usb backend layer
On Mon, Jan 21, 2019 at 04:10:20PM +0200, Yuri Benditovich wrote: > On Fri, Jan 18, 2019 at 3:53 PM Christophe Fergeau > wrote: > > > If you point me at a branch with all your patches applied including this > > one, I can try to help with some of the preparatory commits/some of the > > splitting. > > > > > I've already sent you the illustration how this can be done. > See last 3 commits at > https://gitlab.freedesktop.org/yuri_benditovich/spice-gtk/commits/pers-dev > But this is NOT what I'm submitting right now. What I'm looking for is a branch with the patch being discussed applied, and then the other patches which were sent to this mailing list a few months ago (the cd sharing UI for example). Is there such a branch? Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
