On Thu, Jul 25, 2019 at 11:08 AM Victor Toso <victortoso@xxxxxxxxxx> wrote:
>
> Hi,
>
> On Wed, Jul 24, 2019 at 01:53:43PM +0300, Yuri Benditovich 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;
> > 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;
> > };
> >
> > 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 */
> > }
> > 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);
> > + }
> > 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)
>
> Style: extra space after asterisk
>
> > +{
> > + 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);
> > + 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();
> > + }
>
> g_return_if_fail(dev_type < USB_DEV_TYPE_MAX); ?
>
> > + 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);
>
> Curious about this function, looking at the whole set this is
> called on cd_usb_bulk_msd_lun_changed() which has the comment
> "called when a change happens on SCSI layer"
>
> What kind of change can it happen on the emulated device?
Anything caused by interaction with VM and needed reflection in client UI.
For case of CD, for example this is unload.
>
> > +}
> > +
> > +void spice_usb_backend_device_eject(SpiceUsbBackend *be, SpiceUsbBackendDevice *dev)
> > +{
> > + g_return_if_fail(dev && dev->edev);
> > +
> > + if (be->hotplug_callback) {
>
> Wondering if this should not be a
> g_return_if_fail(be->hotplug_callback != NULL);
>
> This callback is unset only on
> spice_usb_backend_deregister_hotplug() so it should be somewhat
> programming error having spice_usb_backend_device_eject() being
> called after that, no? That could also make not wanted unref to
> dev
ok. but under in any case, the device was created with ref = 1
>
> > + 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"));
> > + 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;
>
> I see the bit is set to 1 but shouldn't it be there a place where
> the bit is set to 0, let's say, when device is disconnected?
MIssed, thanks. Was easy to miss as there is no dynamic creation yet.
>
> > +
> > + 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
> > +{
> > + 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 */
> > } 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);
> > -
> > /* 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
>
> Is there a specific reason for it to be 255 (like standard)? If
> yes, I'd add a small comment above it.
>
> > +
> > +typedef struct UsbCreateDeviceParameters
> > +{
> > + GError *error;
> > + uint32_t address;
> > + uint32_t reserved;
> > + union
> > + {
> > + struct
> > + {
> > + const char *filename;
> > + uint32_t delete_on_eject : 1;
> > + } create_cd;
> > + } device_param;
> > +
> > +} UsbCreateDeviceParameters;
> > +
> > +/* API related to emulated devices */
> > +gboolean spice_usb_backend_create_device(SpiceUsbBackend *be,
> > + UsbEmulatedDeviceType dev_type,
> > + UsbCreateDeviceParameters *param);
> > +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__
> > +
> > +#include "usbredirparser.h"
> > +#include "usb-backend.h"
> > +
> > +typedef struct _SpiceUsbEmulatedDevice 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
> > +*/
> > +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);
> > +} UsbDeviceOps;
>
> Ok,
>
> > +
> > +static inline const UsbDeviceOps *device_ops(SpiceUsbEmulatedDevice *dev) {
> > + 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);
>
> Should be added in later patches.
>
> Cheers,
> Victor
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel