[Spice-devel] [spice-gtk 1/9] usb-redir: define interfaces to support emulated devices
Yuri Benditovich
yuri.benditovich at daynix.com
Mon Jul 29 09:26:11 UTC 2019
On Mon, Jul 29, 2019 at 11:03 AM Frediano Ziglio <fziglio at redhat.com> wrote:
>
> >
> > On Fri, Jul 26, 2019 at 4:54 PM Frediano Ziglio <fziglio at redhat.com> wrote:
> > >
> > > > On Fri, Jul 26, 2019 at 11:43 AM Frediano Ziglio <fziglio at redhat.com>
> > > > wrote:
> > > > >
> > > > > >
> > > > > > On Thu, Jul 25, 2019 at 11:57 AM Frediano Ziglio <fziglio at redhat.com>
> > > > > > wrote:
> > > > > > >
> > > > > > > >
> > > > > > > > SpiceUsbBackendDevice structure is extended to support
> > > > > > > > additional kind of device that is emulated by Spice-GTK
> > > > > > > > and not present locally (and does not have libusb_device),
> > > > > > > > such device has instead pointer to SpiceUsbEmulatedDevice
> > > > > > > > abstract structure. Specific implementation of such device
> > > > > > > > depends on its device type (currently only 'CD device'
> > > > > > > > defined). Implementation module registers constructor for
> > > > > > > > this device type. Device structure is abstract but always
> > > > > > > > starts from table of virtual functions required to redirect
> > > > > > > > such virtual device.
> > > > > > > >
> > > > > > > > Signed-off-by: Yuri Benditovich <yuri.benditovich at daynix.com>
> > > > > > > > ---
> > > > > > > > src/meson.build | 1 +
> > > > > > > > src/usb-backend.c | 128
> > > > > > > > +++++++++++++++++++++++++++++++++++++++++++-
> > > > > > > > src/usb-backend.h | 36 ++++++++++++-
> > > > > > > > src/usb-emulation.h | 88 ++++++++++++++++++++++++++++++
> > > > > > > > 4 files changed, 250 insertions(+), 3 deletions(-)
> > > > > > > > create mode 100644 src/usb-emulation.h
> > > > > > > >
> > > > > > > > diff --git a/src/meson.build b/src/meson.build
> > > > > > > > index adcfaec..49fec52 100644
> > > > > > > > --- a/src/meson.build
> > > > > > > > +++ b/src/meson.build
> > > > > > > > @@ -122,6 +122,7 @@ spice_client_glib_sources = [
> > > > > > > > 'usbutil.c',
> > > > > > > > 'usbutil.h',
> > > > > > > > 'usb-backend.c',
> > > > > > > > + 'usb-emulation.h',
> > > > > > > > 'usb-backend.h',
> > > > > > > > 'vmcstream.c',
> > > > > > > > 'vmcstream.h',
> > > > > > > > diff --git a/src/usb-backend.c b/src/usb-backend.c
> > > > > > > > index 9ac8595..0bf2ecc 100644
> > > > > > > > --- a/src/usb-backend.c
> > > > > > > > +++ b/src/usb-backend.c
> > > > > > > > @@ -39,6 +39,7 @@
> > > > > > > > #include "usbredirparser.h"
> > > > > > > > #include "spice-util.h"
> > > > > > > > #include "usb-backend.h"
> > > > > > > > +#include "usb-emulation.h"
> > > > > > > > #include "channel-usbredir-priv.h"
> > > > > > > > #include "spice-channel-priv.h"
> > > > > > > >
> > > > > > > > @@ -47,6 +48,7 @@
> > > > > > > > struct _SpiceUsbBackendDevice
> > > > > > > > {
> > > > > > > > libusb_device *libusb_device;
> > > > > > > > + SpiceUsbEmulatedDevice *edev;
> > > > > > >
> > > > > > > I suspect only one of these 2 fields are valid, or device
> > > > > > > is emulated or is a real one, right?
> > > > > > > A comment about it like
> > > > > > >
> > > > > > > /* Pointer to device. Either real device (libusb_device)
> > > > > > > * or emulated one (edev) */
> > > > > > > libusb_device *libusb_device;
> > > > > > > SpiceUsbEmulatedDevice *edev;
> > > > > > >
> > > > > > > honestly "edev" looks a bit too shorten, also compared to
> > > > > > > "libusb_device", maybe "emu_dev" ?
> > > > > >
> > > > > > For me it just looks excellent and not similar to any other field.
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > gint ref_count;
> > > > > > > > SpiceUsbBackendChannel *attached_to;
> > > > > > > > UsbDeviceInformation device_info;
> > > > > > > > @@ -66,6 +68,9 @@ struct _SpiceUsbBackend
> > > > > > > > libusb_device **libusb_device_list;
> > > > > > > > gint redirecting;
> > > > > > > > #endif
> > > > > > > > +
> > > > > > > > + SpiceUsbEmulatedDeviceCreate dev_init[USB_DEV_TYPE_MAX];
> > > > > > > > + uint32_t own_devices_mask;
> > > > > > >
> > > > > > > It took a bit of time to understand what is this.
> > > > > > > Looks like it's a sort of bit array of allocated devices.
> > > > > > > Is the "address" limited to 32 by USB specification or is a code
> > > > > > > limitation?
> > > > > > >
> > > > > >
> > > > > > Current code limitation. I do not see we currently need more than 30.
> > > > > >
> > > > > > > I would add a comment like
> > > > > > >
> > > > > > > /* Mask of allocated device, relative bit set to 1 indicate
> > > > > > > allocated
> > > > > > > */
> > > > > > > uint32_t own_devices_mask;
> > > > > > >
> > > > > > > > };
> > > > > > > >
> > > > > > > > struct _SpiceUsbBackendChannel
> > > > > > > > @@ -408,6 +413,7 @@ SpiceUsbBackend *spice_usb_backend_new(GError
> > > > > > > > **error)
> > > > > > > > libusb_set_option(be->libusb_context,
> > > > > > > > LIBUSB_OPTION_USE_USBDK);
> > > > > > > > #endif
> > > > > > > > #endif
> > > > > > > > + be->own_devices_mask = 3; /* exclude addresses 0 and 1
> > > > > > > > */
> > > > > > >
> > > > > > > Why these are reserved? USB specification?
> > > > > >
> > > > > > 0 is reserved, 1 is root hub
> > > > > > Of couse we still can use them for emulated devices but better to
> > > > > > keep
> > > > > > it consistent with real devices.
> > > > > >
> > > > > > >
> > > > > > > > }
> > > > > > > > SPICE_DEBUG("%s <<", __FUNCTION__);
> > > > > > > > return be;
> > > > > > > > @@ -524,8 +530,12 @@ void
> > > > > > > > spice_usb_backend_device_unref(SpiceUsbBackendDevice *dev)
> > > > > > > > {
> > > > > > > > LOUD_DEBUG("%s >> %p(%d)", __FUNCTION__, dev,
> > > > > > > > dev->ref_count);
> > > > > > > > if (g_atomic_int_dec_and_test(&dev->ref_count)) {
> > > > > > > > - libusb_unref_device(dev->libusb_device);
> > > > > > > > - LOUD_DEBUG("%s freeing %p (libusb %p)", __FUNCTION__,
> > > > > > > > dev,
> > > > > > > > dev->libusb_device);
> > > > > > > > + if (dev->libusb_device) {
> > > > > > > > + libusb_unref_device(dev->libusb_device);
> > > > > > > > + LOUD_DEBUG("%s freeing %p (libusb %p)",
> > > > > > > > __FUNCTION__,
> > > > > > > > dev,
> > > > > > > > dev->libusb_device);
> > > > > > > > + } else if (dev->edev) {
> > > > > > > > + device_ops(dev->edev)->delete(dev->edev);
> > > > > > > > + }
> > > > > > >
> > > > > > > Although only one should be NULL I would try to delete both.
> > > > > > > I would also avoid the C++ reserved keyword "delete". Also because
> > > > > > > for the same purpose we we use "free" or "unref" (this function
> > > > > > > is spice_usb_backend_device_unref for instance, above there's
> > > > > > > a call to libusb_unref_device).
> > > > > > >
> > > > > > > > g_free(dev);
> > > > > > > > }
> > > > > > > > }
> > > > > > > > @@ -824,4 +834,118 @@
> > > > > > > > spice_usb_backend_channel_get_guest_filter(SpiceUsbBackendChannel
> > > > > > > > *ch,
> > > > > > > > }
> > > > > > > > }
> > > > > > > >
> > > > > > > > +gchar *
> > > > > > > > spice_usb_backend_device_description(SpiceUsbBackendDevice
> > > > > > > > *dev,
> > > > > > > > + const gchar
> > > > > > > > *format)
> > > > > > > > +{
> > > > > > > > + if (!dev->edev) {
> > > > > > > > + return g_strdup("Not available for libusb devices");
> > > > > > > > + }
> > > > > > > > + gchar *description, *descriptor, *product;
> > > > > > > > + descriptor = g_strdup_printf("[%04x:%04x]",
> > > > > > > > dev->device_info.vid,
> > > > > > > > dev->device_info.pid);
> > > > > > > > + product =
> > > > > > > > device_ops(dev->edev)->get_product_description(dev->edev);
> > > > > > > > + description = g_strdup_printf(format, "", product,
> > > > > > > > descriptor,
> > > > > > > > + dev->device_info.bus,
> > > > > > > > dev->device_info.address);
> > > > > > >
> > > > > > > Not a big fun of having the format not constant. Compiler can do
> > > > > > > magic with a constant while many safety checks are not possible if
> > > > > > > this is not constant.
> > > > > >
> > > > > > Exactly as in
> > > > > > https://gitlab.freedesktop.org/spice/spice-gtk/blob/master/src/usb-device-manager.c#L1460
> > > > > >
> > > > >
> > > > > I think the main problem is that spice_usb_backend_device_description
> > > > > and
> > > > > spice_usb_device_get_description should just be a function,
> > > > > SpiceUsbBackendDevice is containing both emulated and real devices
> > > > > so having spice_usb_device_get_description that handle only real
> > > > > and spice_usb_backend_device_description that handle only emulated
> > > > > does not make sense, there is in fact quite a lot of duplication
> > > > > between the 2 functions at the end of this series
> > > > >
> > > > > > >
> > > > > > > > + g_free(descriptor);
> > > > > > > > + g_free(product);
> > > > > > > > + return description;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +void spice_usb_backend_register_device_type(SpiceUsbBackend *be,
> > > > > > > > +
> > > > > > > > UsbEmulatedDeviceType
> > > > > > > > dev_type,
> > > > > > > > +
> > > > > > > > SpiceUsbEmulatedDeviceCreate
> > > > > > > > init_proc)
> > > > > > > > +{
> > > > > > > > + if (dev_type >= USB_DEV_TYPE_MAX) {
> > > > > > > > + g_return_if_reached();
> > > > > > > > + }
> > > > > > > > + be->dev_init[dev_type] = init_proc;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +void spice_usb_backend_device_report_change(SpiceUsbBackend *be,
> > > > > > > > +
> > > > > > > > SpiceUsbBackendDevice
> > > > > > > > *dev)
> > > > > > > > +{
> > > > > > > > + gchar *desc;
> > > > > > > > + g_return_if_fail(dev && dev->edev);
> > > > > > > > +
> > > > > > > > + desc =
> > > > > > > > device_ops(dev->edev)->get_product_description(dev->edev);
> > > > > > > > + SPICE_DEBUG("%s: %s", __FUNCTION__, desc);
> > > > > > > > + g_free(desc);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +void spice_usb_backend_device_eject(SpiceUsbBackend *be,
> > > > > > > > SpiceUsbBackendDevice *dev)
> > > > > > > > +{
> > > > > > > > + g_return_if_fail(dev && dev->edev);
> > > > > > > > +
> > > > > > > > + if (be->hotplug_callback) {
> > > > > > > > + be->hotplug_callback(be->hotplug_user_data, dev, FALSE);
> > > > > > > > + }
> > > > > > > > + spice_usb_backend_device_unref(dev);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +gboolean spice_usb_backend_create_device(SpiceUsbBackend *be,
> > > > > > > > + UsbEmulatedDeviceType
> > > > > > > > dev_type,
> > > > > > > > +
> > > > > > > > UsbCreateDeviceParameters
> > > > > > > > *param)
> > > > > > > > +{
> > > > > > > > + SpiceUsbEmulatedDevice *edev = NULL;
> > > > > > > > + SpiceUsbBackendDevice *dev;
> > > > > > > > + struct libusb_device_descriptor *desc;
> > > > > > > > + uint16_t device_desc_size;
> > > > > > > > + uint8_t address = 0;
> > > > > > > > +
> > > > > > > > + if (dev_type >= USB_DEV_TYPE_MAX || !be->dev_init[dev_type])
> > > > > > > > {
> > > > > > > > + param->error = g_error_new(SPICE_CLIENT_ERROR,
> > > > > > > > SPICE_CLIENT_ERROR_FAILED,
> > > > > > > > + _("can't create device - not
> > > > > > > > supported"));
> > > > > > >
> > > > > > > Sure you don't want g_error_set instead? This code is always
> > > > > > > assuming that param->error is NULL.
> > > > > >
> > > > > > Even if param->error is not initialized on entry to the function
> > > > > > (although it is), it will be initialized in case of error
> > > > > >
> > > > > > >
> > > > > > > > + return FALSE;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + if (be->own_devices_mask == 0xffffffff) {
> > > > > > > > + param->error = g_error_new(SPICE_CLIENT_ERROR,
> > > > > > > > SPICE_CLIENT_ERROR_FAILED,
> > > > > > > > + _("can't create device -
> > > > > > > > limit
> > > > > > > > reached"));
> > > > > > > > + return FALSE;
> > > > > > > > + }
> > > > > > > > + for (address = 0; address < 32; ++address) {
> > > > > > > > + if (~be->own_devices_mask & (1 << address)) {
> > > > > > > > + break;
> > > > > > > > + }
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + dev = g_new0(SpiceUsbBackendDevice, 1);
> > > > > > > > +
> > > > > > > > + param->address = address;
> > > > > > > > + if (be->dev_init[dev_type](be, dev, param, &edev)) {
> > > > > > > > + g_free(dev);
> > > > > > > > + return FALSE;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + if (!device_ops(edev)->get_descriptor(edev,
> > > > > > > > LIBUSB_DT_DEVICE, 0,
> > > > > > > > + (void **)&desc,
> > > > > > > > &device_desc_size)
> > > > > > > > + || device_desc_size != sizeof(*desc)) {
> > > > > > > > +
> > > > > > > > + device_ops(edev)->delete(edev);
> > > > > > > > + g_free(dev);
> > > > > > > > + param->error = g_error_new(SPICE_CLIENT_ERROR,
> > > > > > > > SPICE_CLIENT_ERROR_FAILED,
> > > > > > > > + _("can't create device -
> > > > > > > > internal
> > > > > > > > error"));
> > > > > > > > + return FALSE;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + be->own_devices_mask |= 1 << address;
> > > > > > > > +
> > > > > > > > + dev->device_info.bus = BUS_NUMBER_FOR_EMULATED_USB;
> > > > > > > > + dev->device_info.address = address;
> > > > > > > > + dev->device_info.vid = desc->idVendor;
> > > > > > > > + dev->device_info.pid = desc->idProduct;
> > > > > > > > + dev->device_info.bcdUSB = desc->bcdUSB;
> > > > > > > > + dev->device_info.class = desc->bDeviceClass;
> > > > > > > > + dev->device_info.subclass = desc->bDeviceSubClass;
> > > > > > > > + dev->device_info.protocol = desc->bDeviceProtocol;
> > > > > > > > + dev->device_info.device_type = dev_type;
> > > > > > > > + dev->ref_count = 1;
> > > > > > > > + dev->edev = edev;
> > > > > > > > +
> > > > > > > > + if (be->hotplug_callback) {
> > > > > > > > + be->hotplug_callback(be->hotplug_user_data, dev, TRUE);
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + return TRUE;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > #endif /* USB_REDIR */
> > > > > > > > diff --git a/src/usb-backend.h b/src/usb-backend.h
> > > > > > > > index 69a490b..8a04ed0 100644
> > > > > > > > --- a/src/usb-backend.h
> > > > > > > > +++ b/src/usb-backend.h
> > > > > > > > @@ -31,15 +31,24 @@ typedef struct _SpiceUsbBackend
> > > > > > > > SpiceUsbBackend;
> > > > > > > > typedef struct _SpiceUsbBackendDevice SpiceUsbBackendDevice;
> > > > > > > > typedef struct _SpiceUsbBackendChannel SpiceUsbBackendChannel;
> > > > > > > >
> > > > > > > > +typedef enum
> > > > > > > > +{
> > > > > > >
> > > > > > > Style: style document state that bracket should be on the same
> > > > > > > line,
> > > > > > > here and in many other places. I know there are other occurrences
> > > > > > > but
> > > > > > > I would prefer we use one style.
> > > > > > >
> > > > > > > > + USB_DEV_TYPE_NONE,
> > > > > > > > + USB_DEV_TYPE_CD,
> > > > > > > > + USB_DEV_TYPE_MAX
> > > > > > > > +} UsbEmulatedDeviceType;
> > > > > > > > +
> > > > > > > > typedef struct UsbDeviceInformation
> > > > > > > > {
> > > > > > > > uint16_t bus;
> > > > > > > > uint16_t address;
> > > > > > > > uint16_t vid;
> > > > > > > > uint16_t pid;
> > > > > > > > + uint16_t bcdUSB;
> > > > > > > > uint8_t class;
> > > > > > > > uint8_t subclass;
> > > > > > > > uint8_t protocol;
> > > > > > > > + uint8_t device_type; /* UsbEmulatedDeviceType */
> > > > > > >
> > > > > > > I suppose you don't use UsbEmulatedDeviceType to reduce structure
> > > > > > > size, right?
> > > > > > > How this field is filled for real devices? USB_DEV_TYPE_NONE?
> > > > > > > Maybe should be named USB_DEV_TYPE_REAL then?
> > > > > > >
> > > > > > > > } UsbDeviceInformation;
> > > > > > > >
> > > > > > > > typedef void(*usb_hot_plug_callback)(void *user_data,
> > > > > > > > SpiceUsbBackendDevice
> > > > > > > > *dev, gboolean added);
> > > > > > > > @@ -71,7 +80,6 @@ gboolean
> > > > > > > > spice_usb_backend_device_isoch(SpiceUsbBackendDevice *dev);
> > > > > > > > /* returns 0 if the device passes the filter */
> > > > > > > > int spice_usb_backend_device_check_filter(SpiceUsbBackendDevice
> > > > > > > > *dev,
> > > > > > > > const struct
> > > > > > > > usbredirfilter_rule
> > > > > > > > *rules, int count);
> > > > > > > > -
> > > > > > >
> > > > > > > I would live this empty line.
> > > > > > >
> > > > > > > > /* Spice USB backend channel API */
> > > > > > > > SpiceUsbBackendChannel
> > > > > > > > *spice_usb_backend_channel_new(SpiceUsbBackend
> > > > > > > > *context,
> > > > > > > > void
> > > > > > > > *user_data);
> > > > > > > > @@ -89,6 +97,32 @@ void
> > > > > > > > spice_usb_backend_channel_get_guest_filter(SpiceUsbBackendChannel
> > > > > > > > *ch,
> > > > > > > > int *count);
> > > > > > > > void spice_usb_backend_return_write_data(SpiceUsbBackendChannel
> > > > > > > > *ch,
> > > > > > > > void
> > > > > > > > *data);
> > > > > > > >
> > > > > > > > +#define BUS_NUMBER_FOR_EMULATED_USB 255
> > > > > > > > +
> > > > > > >
> > > > > > > What is this? Where is it used?
> > > > > >
> > > > > > It can be used anywhere when we need to distinguish between local
> > > > > > device and emulated one.
> > > > > > Yes, we can always get the device info and see the device type. But
> > > > > > if
> > > > > > we already have bus and address we can make it simplier.
> > > > > >
> > > > >
> > > > > I noted that Linux represent the bus using 3 hexadecimal digit and we
> > > > > use a uint16_t to store it. Maybe use a 0xffff value instead?
> > > > >
> > > > > > > Style: do not indent macro definition.
> > > > > > >
> > > > > > > > +typedef struct UsbCreateDeviceParameters
> > > > > > > > +{
> > > > > > > > + GError *error;
> > > > > > > > + uint32_t address;
> > > > > > > > + uint32_t reserved;
> > > > > > >
> > > > > > > "reserved"? Is this a kind of ABI exported structure?
> > > > > > > Alignment to avoid holes on 64 bit?
> > > > > > >
> > > > > > > > + union
> > > > > > > > + {
> > > > > > > > + struct
> > > > > > > > + {
> > > > > > > > + const char *filename;
> > > > > > > > + uint32_t delete_on_eject : 1;
> > > > > > >
> > > > > > > I suppose the ": 1" is to reduce structure size if more flags are
> > > > > > > added.
> > > > > > > I would use uint8_t instead of uint32_t for portability (on Visual
> > > > > > > studio
> > > > > > > this will allocate a uint32_t, GNU use the smaller type).
> > > > > > >
> > > > > > > > + } create_cd;
> > > > > > > > + } device_param;
> > > > > > > > +
> > > > > > > > +} UsbCreateDeviceParameters;
> > > > > > > > +
> > > > > > > > +/* API related to emulated devices */
> > > > > > > > +gboolean spice_usb_backend_create_device(SpiceUsbBackend *be,
> > > > > > > > + UsbEmulatedDeviceType
> > > > > > > > dev_type,
> > > > > > > > +
> > > > > > > > UsbCreateDeviceParameters
> > > > > > > > *param);
> > > > > > >
> > > > > > > Looks like this API is quite type fragile. If required that
> > > > > > > dev_type is
> > > > > > > registered
> > > > > >
> > > > > > I do not understand what is 'fragile' in this context.
> > > > > > Yes, dev_type should be registered to be created.
> > > > > > If due to some reason required type is not registered, this will
> > > > > > raise
> > > > > > the error, nothing bad.
> > > > > > Nobody should know too much about implementation details.
> > > > > >
> > > > >
> > > > > Let say I have to develop a driver for a new harware requiring a
> > > > > driver.
> > > > >
> > > > > Usually
> > > > > - you write a driver implementing a given interface
> > > > > - you install the driver
> > > > > - the driver load
> > > > > - the driver register the implementation for the interface
> > > > >
> > > > > With your interface type
> > > > > - you contact the OS vendor to allocate a number for your device
> > > > > - you write a driver implementing a given interface
> > > > > - you wait the OS vendor to update the system
> > > > > - you install the driver
> > > > > - you reboot the system, the OS need to call your driver
> > > > >
> > > > I am not sure how this sequence is related to the subject.
> > >
> > > usb-backend is like the USB implementation made by kernel OS vendor,
> > > so to edit usb.backend.[ch] for instance you would have to post a
> > > patch and wait the vendor to package and distribute.
> > > When Linux kernel was using fixed major+minor for devices (so no
> > > devfs or udev but manual mknod) you had to post a patch to have
> > > the headers updated to add the numbers.
> > > The same happens with the interface you designed, to add a
> > > constant to UsbEmulatedDeviceType you would need to contact the
> > > vendor. The same to change UsbCreateDeviceParameters.
> > >
> > > > Let's take another example, closer to our situaltion:
> > > > virtual machine (qemu/virtual box/whatever).
> > > > There are many possible kinds of devices, each one registers itself
> > > > as capable of supporting specific type of device.
> > > > When the device should be created (due to system definition or
> > > > special request), the responsible module finds the registered
> > > > supporter for this kind of device and requests it to create device.
> > > >
> > >
> > > Yes, but is not that the usb call all setup for all device, each
> > > device call register but are not called by that code. The difference
> > > is that your code has circular dependencies both in header and
> > > link dependency.
> > >
> > > > > There are multiple circular dependencies.
> > > > > The backend needs to kwnow the list of emulated types and call the
> > > > > relative setup functions that will call your register function.
> > > >
> > > > No problem, let's do the registration by typename, like "cd" without
> > > > any knowledge at all.
> > > >
> > >
> > > That is let's define another shared unique identifier. Can work, also
> > > having an header like
> > >
> > >
> > > // usb-emulation-cd.h
> > > #pragma once
> > >
> > > #include "usb-backend.h"
> > >
> > > typedef struct CdEmulationParams {
> > > const char *filename;
> > > uint32_t delete_on_eject : 1;
> > > } CdEmulationParams;
> > >
> > > SpiceUsbBackendDevice *
> > > create_emulated_cd(SpiceUsbBackend *be,
> > > CdEmulationParams *param,
> > > GError **err);
> > >
> > >
> > > and creating a device with
> > >
> > >
> > > CdEmulationParams params = {
> > > "/dev/test", 1
> > > };
> > > GError *err = NULL;
> > > SpiceUsbBackendDevice *device = create_emulated_cd(backend, ¶ms, &err);
> > >
> > >
> > > will work.
> > > No enumeration to update, no constants to define in a shared header.
> > > usb-backend does not need to know that usb-emulation-cd exists.
> > > The "params" is typed so it's harder to mess it up.
> > > No need to change usb-backend.h to add another emulated device.
> > >
> > I guess in your example each type of emulated device requires
> > dedicated header file.
> > I'd put this into usb-backend.h anyway.
> >
>
> Yes, this create a source circular dependency.
> Most of the code have a .c file and a relative .h file declaring
> the public interface of this source file. I don't see why
> usb-device-cd.c cannot have a usb-device-cd.h file for the same
> reason.
In my vision:
- such header file is usb-emulation.h
- each type of device need just one line in H file for public function
so there is no advantage in heaving special header
> I don't understand much the separation between usb-backend.h and
> usb-emulation.h. There's no usb-emulation.c so surely usb-emulation.h
> is not the public interface of usb-emulation.c. Looks like is
> the pricate interface of usb-backend.h specifically to implement
> the specific emulated devices but the separation is fragile, part
> of this interface is also in usb-backend.h. For instance
> spice_usb_backend_device_eject and spice_usb_backend_device_report_change.
>
> > >
> > > > > The create type needs to have all details off all emulated types.
> > > > >
> > > > > The name spice_usb_backend_create_device suggests that the function
> > > > > is creating any device which is false, it only creates emulated
> > > > > devices.
> > > > >
> > > > > > > with the type required to accept that specific param (param has no
> > > > > > > type
> > > > > > > in
> > > > > > > it
> > > > > > > but the union has different fields).
> > > > > > > Why not passing the initialization function directly?
> > > > > >
> > > > > > Because the caller does not (and should not have any idea) about any
> > > > > > initialization function.
> > > >
> > > > I (always) try to limit the knowledge about internals on 'need-to-know'
> > > > basis.
> > > > In this paradigm the caller (usb-dev-manager) shall know which kinds of
> > > > device it may try to create/use and which parameter it needs to
> > > > provide, nothing more.
> > > >
> > >
> > > usb-dev-manager will know all the possible emulated devices and also
> > > usb-backend will know all the devices and both will depends on all
> > > emulated devices implementation.
> > >
> >
> > In my variant the same thing happens (but without any dedicated header).
>
>
> Well, this IS the description of your variant.
> Mine don't have
> - usb-backend depending on emulated devices;
> - usb-dev-manger depending on all emulated devices (only emulated cd).
>
> > Actually this is not so important, any code is suitable, advantage of
> > existing one is that it exists.
> >
>
> What does it means? The code cannot be improved before being merged?
> It does not seem so crazy change to make to me.
> As I said multiple time I can do that change and send as follow up.
If this help us moving forward, please do that.
My interest is just have it merged faster.
>
> > > > >
> > > > > The caller is passing the type and must initialize the proper part of
> > > > > the parameter structure so it must know quite some detail.
> > > > >
> > > > > Passing the init function would remove the necessity of having to
> > > > > register the type, remove the circular dependency, the backend layer
> > > > > does not need to know the details of the various emulations (like the
> > > > > CD).
> > > >
> > > > Seems very strange for me: the init function required to create the
> > > > device shall be exported
> > > > but the backend can't use it. Other module that want to create
> > > > emulated device should
> > > > know what is 'init function' and pass it to backend. Sorry, but for me
> > > > this does not make sense...
> > > >
> > >
> > > See example I proposed above.
> > >
> > > > >
> > > > > > It may want to create some specific device with specific initial
> > > > > > parameters.
> > > > > >
> > > > >
> > > > > But it can't on its own, you need to change the backend adding
> > > > > constant,
> > > > > changing types and calling a new setup function.
> > > > >
> > > > > > >
> > > > > > > > +gchar
> > > > > > > > *spice_usb_backend_device_description(SpiceUsbBackendDevice
> > > > > > > > *dev,
> > > > > > > > const gchar *format);
> > > > > > > > +void spice_usb_backend_device_eject(SpiceUsbBackend *be,
> > > > > > > > SpiceUsbBackendDevice *device);
> > > > > > > > +void spice_usb_backend_device_report_change(SpiceUsbBackend *be,
> > > > > > > > SpiceUsbBackendDevice *device);
> > > > > > > > +
> > > > > > > > G_END_DECLS
> > > > > > > >
> > > > > > > > #endif
> > > > > > > > diff --git a/src/usb-emulation.h b/src/usb-emulation.h
> > > > > > > > new file mode 100644
> > > > > > > > index 0000000..4473361
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/src/usb-emulation.h
> > > > > > > > @@ -0,0 +1,88 @@
> > > > > > > > +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> > > > > > > > +/*
> > > > > > > > + Copyright (C) 2019 Red Hat, Inc.
> > > > > > > > +
> > > > > > > > + Red Hat Authors:
> > > > > > > > + Yuri Benditovich<ybendito at redhat.com>
> > > > > > > > +
> > > > > > > > + This library is free software; you can redistribute it
> > > > > > > > and/or
> > > > > > > > + modify it under the terms of the GNU Lesser General Public
> > > > > > > > + License as published by the Free Software Foundation; either
> > > > > > > > + version 2.1 of the License, or (at your option) any later
> > > > > > > > version.
> > > > > > > > +
> > > > > > > > + This library is distributed in the hope that it will be
> > > > > > > > useful,
> > > > > > > > + but WITHOUT ANY WARRANTY; without even the implied warranty
> > > > > > > > of
> > > > > > > > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
> > > > > > > > the
> > > > > > > > GNU
> > > > > > > > + Lesser General Public License for more details.
> > > > > > > > +
> > > > > > > > + You should have received a copy of the GNU Lesser General
> > > > > > > > Public
> > > > > > > > + License along with this library; if not, see
> > > > > > > > <http://www.gnu.org/licenses/>.
> > > > > > > > +*/
> > > > > > > > +
> > > > > > > > +#ifndef __SPICE_USB_EMULATION_H__
> > > > > > > > +#define __SPICE_USB_EMULATION_H__
> > > > > > > > +
> > > > > > >
> > > > > > > We should stop using reserved identifier, C99 is about 20 years
> > > > > > > old.
> > > > > > >
> > > > > > > Maybe won't be also bad to use "#pragma once" instead but this is
> > > > > > > OT.
> > > > > > >
> > > > > > > > +#include "usbredirparser.h"
> > > > > > > > +#include "usb-backend.h"
> > > > > > > > +
> > > > > > > > +typedef struct _SpiceUsbEmulatedDevice SpiceUsbEmulatedDevice;
> > > > > > >
> > > > > > > C99 reserved identifier, use SpiceUsbEmulatedDevice not
> > > > > > > _SpiceUsbEmulatedDevice.
> > > > > > >
> > > > > > > > +typedef int (*SpiceUsbEmulatedDeviceCreate)(SpiceUsbBackend *be,
> > > > > > > > +
> > > > > > > > SpiceUsbBackendDevice
> > > > > > > > *parent,
> > > > > > > > +
> > > > > > > > UsbCreateDeviceParameters
> > > > > > > > *param,
> > > > > > > > +
> > > > > > > > SpiceUsbEmulatedDevice
> > > > > > > > **device);
> > > > > > > > +
> > > > > > > > +/*
> > > > > > > > + function table for emulated USB device
> > > > > > > > + must be first member of device structure
> > > > > > > > + all functions are mandatory for implementation
> > > > > > > > +*/
> > > > > > >
> > > > > > > Style: we use style like
> > > > > > >
> > > > > > > /* function table for emulated USB device
> > > > > > > * must be first member of device structure
> > > > > > > * all functions are mandatory for implementation
> > > > > > > */
> > > > > > >
> > > > > > > Main exception is file header but beside that in all other comment
> > > > > > > there's an initial star ("*") and comment start at first line.
> > > > > > > Here and in other occurences.
> > > > > > >
> > > > > > > > +typedef struct UsbDeviceOps
> > > > > > > > +{
> > > > > > > > + gboolean (*get_descriptor)(SpiceUsbEmulatedDevice *device,
> > > > > > > > + uint8_t type, uint8_t index,
> > > > > > > > + void **buffer, uint16_t *size);
> > > > > > > > + gchar * (*get_product_description)(SpiceUsbEmulatedDevice
> > > > > > > > *device);
> > > > > > > > + void (*attach)(SpiceUsbEmulatedDevice *device, struct
> > > > > > > > usbredirparser
> > > > > > > > *parser);
> > > > > > > > + void (*reset)(SpiceUsbEmulatedDevice *device);
> > > > > > > > + /*
> > > > > > > > + processing is synchronous, default = stall:
> > > > > > > > + - return success without data: set status to 0
> > > > > > > > + - return error - set status to error
> > > > > > > > + - return success with data - set status to 0,
> > > > > > > > + set buffer to some buffer
> > > > > > > > + set length to out len
> > > > > > > > + truncation is automatic
> > > > > > > > + */
> > > > > > > > + void (*control_request)(SpiceUsbEmulatedDevice *device,
> > > > > > > > + uint8_t *data, int data_len,
> > > > > > > > + struct
> > > > > > > > usb_redir_control_packet_header
> > > > > > > > *h,
> > > > > > > > + void **buffer);
> > > > > > > > + /*
> > > > > > > > + processing is synchronous:
> > > > > > > > + - set h->status to resulting status, default = stall
> > > > > > > > + */
> > > > > > > > + void (*bulk_out_request)(SpiceUsbEmulatedDevice *device,
> > > > > > > > + uint8_t ep, uint8_t *data, int
> > > > > > > > data_len,
> > > > > > > > + uint8_t *status);
> > > > > > > > + /*
> > > > > > > > + if returns true, processing is asynchronous
> > > > > > > > + otherwise header contains error status
> > > > > > > > + */
> > > > > > > > + gboolean (*bulk_in_request)(SpiceUsbEmulatedDevice *device,
> > > > > > > > uint64_t
> > > > > > > > id,
> > > > > > > > + struct usb_redir_bulk_packet_header
> > > > > > > > *bulk_header);
> > > > > > > > + void (*cancel_request)(SpiceUsbEmulatedDevice *device,
> > > > > > > > uint64_t
> > > > > > > > id);
> > > > > > > > + void (*detach)(SpiceUsbEmulatedDevice *device);
> > > > > > > > + void (*delete)(SpiceUsbEmulatedDevice *device);
> > > > > > >
> > > > > > > As stated above, I would prefer "free" or "unref".
> > > > > >
> > > > > > There is no "ref", so should not be also unref.
> > > > > > If you do not like 'delete' so much (although it exactly reflects
> > > > > > what
> > > > > > we do), we can call it 'unrealize'
> > > > > >
> > > > > > >
> > > > > > > > +} UsbDeviceOps;
> > > > > > > > +
> > > > > > > > +static inline const UsbDeviceOps
> > > > > > > > *device_ops(SpiceUsbEmulatedDevice
> > > > > > > > *dev) {
> > > > > > >
> > > > > > > Style: bracket on next line for functions.
> > > > > > >
> > > > > > > > + return (const UsbDeviceOps *)dev;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +void spice_usb_backend_register_device_type(SpiceUsbBackend *be,
> > > > > > > > +
> > > > > > > > UsbEmulatedDeviceType
> > > > > > > > dev_type,
> > > > > > > > +
> > > > > > > > SpiceUsbEmulatedDeviceCreate
> > > > > > > > init_proc);
> > > > > > > > +/* supported device types */
> > > > > > > > +void spice_usb_device_register_cd(SpiceUsbBackend *be);
> > > > > > > > +
> > > > > > > > +#endif
> > > > > > >
> > > > > > > I can write a follow up patch for style and some other changes
> > > > > > > (like
> > > > > > > some
> > > > > > > comments in code) if you like.
> > > > > > >
> > > > > >
> > > > > > This and further patches of this series will cause a lot of notes
> > > > > > regarding 'style'.
> > > > > > Surprisingly, most of these 'style rules' are not reflected in
> > > > > > https://www.spice-space.org/spice-project-coding-style-and-coding-conventions.html
> > > > > > For most such notes I can see files in the project which use
> > > > > > different
> > > > > > style.
> > > > > > I suggest to focus first on other than style aspects, after we finish
> > > > > > with
> > > > > > them,
> > > > > > probably this is the best idea to make such 'style' patches to drop
> > > > > > all
> > > > > > the
> > > > > > questions regarding style.
> > > > > >
> > > > >
> > > > > Code is surely 100% coherent, this does not mean that when we write new
> > > > > code we are free to ignore it totally.
> > > > > Obviously if patches are quite out of style won't be acked.
> > > > >
> > > > > If is fine for you we could fix the style on commit.
> > > > >
> > > > > Frediano
> > > >
> >
More information about the Spice-devel
mailing list