>
> 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.
>
It sounds fine. I found the name not much meaningful. Looking at the code
this is a state change (stop or start), the name indicate a generic
"change". Maybe "spice_usb_backend_device_state_changed" would be more
meaningful?
> >
> > > +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.
>
That make sense and be more coherent. In this case the call to
spice_usb_backend_device_unref is surely wrong and should be moved
to spice_usb_backend_create_emulated_device (as suggested below)
to avoid leaks or use-after-free.
I still think that a alloc/free test should work without calling
a spice_usb_backend_device_eject function.
> >
> > > +}
> > > +
> > > +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