[Spice-devel] [spice-gtk v1 0/2] Add usb backend module

[Yuri Benditovich]

Christophe,

Our primary goal, if you remember, was to deliver cd-sharing feature.
For that it was needed to isolate usb redirection modules from libusb and
usbredir to create some abstraction layer for USB devices whether they are
local or emulated in software.
SpiceUsbDevice is type of object that Usb Device Manager uses internally,
opaque for other modules and external API, with its own life cycle and with
system dependent structure. It is referenced by external components (as
widget or external application).
Introduced by us SpiceUsbBackendDevice is used by abstraction layer
internally, with its own life cycle and opaque for all other modules with
system-independent structure.
Current commit makes minimal changes in existing modules and serves the
initial goal.

Your suggestion breaks the initial goal and moves us to additional
refactoring of already prepared 'engineering stable' code and additional
complicated (IMO) reintegration with USB device manager.
This also breaks the original idea of isolation - such a way all the
internals of 'backend device' will be visible to usb device manager,
including libusb.
I do not see what is advantage of doing this and especially of doing this
right now.

>From my point of view, the order should be following:
1. Finalize step 1 patches (isolation, opaque usb backend)
2. Add cd-sharing  feature above it
3. Finalize UI solution that serves cd-sharing
4. Reduce differences between Linux and Windows in Usb device manager
(persistent backend device object)
5. Reevalute the plans of unifying internal data structures (if this makes
sense).

Thanks,
Yuri


On Thu, Sep 27, 2018 at 3:00 PM Christophe Fergeau <cfergeau at redhat.com>
wrote:

> Hey,
>
> One additional high-level comment, after these patches, we have
> in the public API an opaque SpiceUsbDevice, which is
> // this is the structure behind SpiceUsbDevice
> typedef struct _SpiceUsbDeviceInfo {
>     guint8  busnum;
>     guint8  devaddr;
>     guint16 vid;
>     guint16 pid;
>     gboolean isochronous;
> #ifdef G_OS_WIN32
>     guint8  state;
> #else
>     SpiceUsbBackendDevice *bdev;
> #endif
>     gint    ref;
> } SpiceUsbDeviceInfo;
>
> SpiceUsbBackendDevice is
> struct _SpiceUsbBackendDevice
> {
>     union
>     {
>         void *libusb_device;
>         void *msc;
>     } d;
>     uint32_t isLibUsb   : 1;
>     uint32_t configured : 1;
>
>     int refCount;
>     void *mutex;
>     SpiceUsbBackendChannel *attached_to;
>     UsbDeviceInformation device_info;
> };
> And UsbDeviceInformation is
> typedef struct UsbDeviceInformation
> {
>     uint16_t bus;
>     uint16_t address;
>     uint16_t vid;
>     uint16_t pid;
>     uint8_t class;
>     uint8_t subclass;
>     uint8_t protocol;
>     uint8_t isochronous;
>     uint8_t max_luns;
> } UsbDeviceInformation;
>
> This is really redundant, from experimenting a bit, it seems the
> SpiceUsbBackendDevice + UsbDeviceInformation you add in this patch could
> instead be SpiceUsbDevice. In short, I'd move SpiceUsbDeviceInfo
> definition from usb-device-manager.c to a new usb-device.c (aka
> usb-device-backend.c), and then add the new API introduced in your patch
> on top of that, using SpiceUsbDevice everywhere rather than adding
> SpiceUsbBackend.
>
> Christophe
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180927/267b64f2/attachment.html>