>
> On Thu, Jul 25, 2019 at 11:57 AM Frediano Ziglio <fziglio@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > > 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
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.
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.
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).
> 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@xxxxxxxxxx>
> > > +
> > > + 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel