On Mon, Aug 5, 2019 at 12:58 PM 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. Implementation module will define
> > constructor for specific 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 | 102 +++++++++++++++++++++++++++++++++++++++++++-
> > src/usb-backend.h | 3 ++
> > src/usb-emulation.h | 91 +++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 195 insertions(+), 2 deletions(-)
> > create mode 100644 src/usb-emulation.h
> >
> > diff --git a/src/meson.build b/src/meson.build
> > index b4a6870..fe19c16 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 3334f56..be60cf3 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"
> >
> > @@ -46,7 +47,10 @@
> >
> > struct _SpiceUsbBackendDevice
> > {
> > + /* Pointer to device. Either real device (libusb_device)
> > + * or emulated one (edev) */
> > libusb_device *libusb_device;
> > + SpiceUsbEmulatedDevice *edev;
> > gint ref_count;
> > SpiceUsbBackendChannel *attached_to;
> > UsbDeviceInformation device_info;
> > @@ -66,6 +70,10 @@ struct _SpiceUsbBackend
> > libusb_device **libusb_device_list;
> > gint redirecting;
> > #endif
> > +
> > + /* Mask of allocated device, a specific bit set to 1 to indicate that
> > the device at
> > + * that address is allocated */
> > + uint32_t own_devices_mask;
> > };
> >
> > struct _SpiceUsbBackendChannel
> > @@ -413,6 +421,8 @@ SpiceUsbBackend *spice_usb_backend_new(GError **error)
> > libusb_set_option(be->libusb_context, LIBUSB_OPTION_USE_USBDK);
> > #endif
> > #endif
> > + /* exclude addresses 0 (reserved) and 1 (root hub) */
> > + be->own_devices_mask = 3;
> > }
> > SPICE_DEBUG("%s <<", __FUNCTION__);
> > return be;
> > @@ -529,8 +539,13 @@ 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);
> > + }
> > + if (dev->edev) {
> > + device_ops(dev->edev)->unrealize(dev->edev);
> > + }
> > g_free(dev);
> > }
> > }
> > @@ -829,4 +844,87 @@
> > spice_usb_backend_channel_get_guest_filter(SpiceUsbBackendChannel *ch,
> > }
> > }
> >
> > +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);
> > +}
> > +
>
> Looks like this function is just for debugging.
> Why instead use spice_usb_backend_device_get_description in usb-device-cd.c
> and call SPICE_DEBUG directly?
usb-device-cd.c has nothing to do with this change.
It issues the update to whom it might be important.
Currently as we do not have UI, also outside of usb-device-cd.c there
is no action.
>
> > +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);
> > + }
> > + be->own_devices_mask &= ~(1 << dev->device_info.address);
> > +
> > + spice_usb_backend_device_unref(dev);
>
> From my experiments looks like that reference counting for these
> emulated devices are different from normal ones.
> In normal ones the device list in usb manager is the main owner,
> for these devices you have an additional reference that is removed
> here. So if this function is not called you have a leak. Also
> is weird to have a reference without having an actual pointer.
> The same apply to own_devices_mask bit clearance (added in this
> version of the patch). If I set the bit creating an object I expect
> the bit to be clear during the object destruction, not another
> not related function. I wrote a test that call in a loop 128 times
> create_emulated_cd and spice_usb_backend_device_unref and fails
> (https://gitlab.freedesktop.org/fziglio/spice-gtk/commit/0bbc0d85b43b5dbcb92c2a3915b4b1c9d9228a7a,
> will probably disappear in a while). I would expect this test to
> pass and to delete completely the object without leaks.
This is wrong approach, I think.
The interface should be changed and shall not return the object.
(initial interface returned boolean)
usb-device-manager shall receive the device only via hotplug
indication, reference it and deref it when it receives unplug.
>
> > +}
> > +
> > +SpiceUsbBackendDevice*
> > +spice_usb_backend_create_emulated_device(SpiceUsbBackend *be,
> > + SpiceUsbEmulatedDeviceCreate
> > create_proc,
> > + void *create_params,
> > + GError **err)
> > +{
> > + SpiceUsbEmulatedDevice *edev;
> > + SpiceUsbBackendDevice *dev;
> > + struct libusb_device_descriptor *desc;
> > + uint16_t device_desc_size;
> > + uint8_t address = 0;
> > +
> > + if (be->own_devices_mask == 0xffffffff) {
> > + g_set_error(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> > + _("can't create device - limit reached"));
> > + return NULL;
> > + }
> > + for (address = 0; address < 32; ++address) {
> > + if (~be->own_devices_mask & (1 << address)) {
> > + break;
> > + }
> > + }
> > +
> > + dev = g_new0(SpiceUsbBackendDevice, 1);
> > + dev->device_info.bus = BUS_NUMBER_FOR_EMULATED_USB;
> > + dev->device_info.address = address;
> > + dev->ref_count = 1;
> > +
> > + dev->edev = edev = create_proc(be, dev, create_params, err);
> > + if (edev == NULL) {
> > + spice_usb_backend_device_unref(dev);
> > + return NULL;
> > + }
> > +
> > + if (!device_ops(edev)->get_descriptor(edev, LIBUSB_DT_DEVICE, 0,
> > + (void **)&desc, &device_desc_size)
> > + || device_desc_size != sizeof(*desc)) {
> > +
> > + spice_usb_backend_device_unref(dev);
> > + g_set_error(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> > + _("can't create device - internal error"));
> > + return NULL;
> > + }
> > +
> > + be->own_devices_mask |= 1 << 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;
> > +
> > + if (be->hotplug_callback) {
> > + be->hotplug_callback(be->hotplug_user_data, dev, TRUE);
> > + }
>
> Here the difference from normal devices. In normal devices
> hotplug_callback callback is called from hotplug_callback function then
> device is released with spice_usb_backend_device_unref. Here the
> function returns the object. This pointer is returned by create_emulated_cd
> then the caller (spice_usb_device_manager_set_property) just discard
> the pointer.
>
> > +
> > + return dev;
> > +}
> > +
> > #endif /* USB_REDIR */
> > diff --git a/src/usb-backend.h b/src/usb-backend.h
> > index 69a490b..63b9202 100644
> > --- a/src/usb-backend.h
> > +++ b/src/usb-backend.h
> > @@ -31,12 +31,15 @@ typedef struct _SpiceUsbBackend SpiceUsbBackend;
> > typedef struct _SpiceUsbBackendDevice SpiceUsbBackendDevice;
> > typedef struct _SpiceUsbBackendChannel SpiceUsbBackendChannel;
> >
> > +#define BUS_NUMBER_FOR_EMULATED_USB G_MAXUINT16
> > +
> > 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;
> > diff --git a/src/usb-emulation.h b/src/usb-emulation.h
> > new file mode 100644
> > index 0000000..9e626a2
> > --- /dev/null
> > +++ b/src/usb-emulation.h
> > @@ -0,0 +1,91 @@
> > +/* -*- 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 SpiceUsbEmulatedDevice*
> > +(*SpiceUsbEmulatedDeviceCreate)(SpiceUsbBackend *be,
> > + SpiceUsbBackendDevice *parent,
> > + void *create_params,
> > + GError **err);
> > +
> > +/*
> > + 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 (*unrealize)(SpiceUsbEmulatedDevice *device);
> > +} UsbDeviceOps;
> > +
> > +static inline const UsbDeviceOps *device_ops(SpiceUsbEmulatedDevice *dev)
> > +{
> > + return (const UsbDeviceOps *)dev;
> > +}
> > +
> > +SpiceUsbBackendDevice*
> > +spice_usb_backend_create_emulated_device(SpiceUsbBackend *be,
> > + SpiceUsbEmulatedDeviceCreate
> > create_proc,
> > + void *create_params,
> > + GError **err);
> > +void spice_usb_backend_device_eject(SpiceUsbBackend *be,
> > SpiceUsbBackendDevice *device);
> > +void spice_usb_backend_device_report_change(SpiceUsbBackend *be,
> > SpiceUsbBackendDevice *device);
> > +
> > +#endif
>
> Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel