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?
> +}
> +
> +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
> + 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?
> +
> + 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel