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@xxxxxxxxxx> 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
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel