[Spice-devel] [spice-gtk v2 1/8] usb-redir: define interfaces to support emulated devices
Yuri Benditovich
yuri.benditovich at daynix.com
Wed Aug 7 16:20:12 UTC 2019
On Wed, Aug 7, 2019 at 6:21 PM Frediano Ziglio <fziglio at redhat.com> wrote:
>
> >
> > On Mon, Aug 5, 2019 at 3:17 PM Frediano Ziglio <fziglio at redhat.com> wrote:
> > >
> > > >
> > > > On Mon, Aug 5, 2019 at 12:58 PM Frediano Ziglio <fziglio at redhat.com>
> > > > 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 at daynix.com>
> > > > > > ---
> > > > > > 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.
> >
> > This test is probably good for something that I do not see, but it has
> > nothing common with how things work in reality.
> > The main difference with local usb devices (and the reason why teh
> > referencing is done differently) is that for local devices the list of
> > existing devices is always maintained by somebody.
> > Local devices exist without any relation to backend object: in Linux
> > libusb maintains the list, in Windows libusb is able to return the
> > list.
> > I'd prefer to avoid this complication (i.e. maintaining list of all
> > the emulated devices).
> >
>
> I was not referring to an additional list, I was talking about "devices"
> array in SpiceUsbDeviceManagerPrivate. Devices (either from libusb and
> also emulated) are wrapped inside a SpiceUsbBackendDevice which is
> wrapped inside a SpiceUsbDevice and "devices" field keep all SpiceUsbDevice.
>
I just explain that the lifecycle of local devices and emulated
devices is different.
Let's take Linux as an example:
libusb maintains under libusb context the list of local devices (with
minimal refcount of 1).
When the device is just added: libusb calls hotplug indication, if the
usb-device-manager does not increment the refcount, nobody knows that
the device is exists, but it does.
(It will be reported, for example, if one registers hotplug callback -
hotplug callback will be fired with all existing devices from _this_
list).
Usually, of course, usb-device-manager references the backend device
and the backend device holds additional reference to the libusb
device.
When the local device disappears, hot unplug procedure happens, all
the guys (that used the device) dereference it, libusb also
dereferences it -> the device does not exist anymore.
With emulated devices we just use the fact that usb-device-manager
does not have any exceptions, it references each new device and
dereferences it when the device disappears.
The problem of the test is:
it does create_cd_device, then unrefs it.and expect the device will be
freed correctly and completely.
But the unref is not an opposite of create_cd_device (for most of glib
object it seems natural, but this is not a glib object).
opposite of 'create' is 'eject'.
Yes, we can make this test work, but _IMO_ this will not improve the
quality of the life to anybody (because the test uses the sequence of
operations that is not used in any real scenario).
For example, we can add backend * to the backend device and remove the
mask when the refcount of backend device goes to zero.
Is this what you want to be done?
> > Another option is just to change the test:
> > device = create_cd_device(backend, params)
> > spice_usb_backend_device_eject(backend, device)
> >
>
> In a previous mail you proposed to return a boolean and drop a reference so
> you won't have a pointer to device to call spice_usb_backend_device_eject.
>
> The test is supposed to use usb-backend APIs but spice_usb_backend_device_eject
> is not a usb-backend API so this would require that API to be moved/"promoted"
> to usb-backend. Also if this is the API this means that all create_cd_device
> should be followed by a spice_usb_backend_device_eject for consistency but
> this would cause the device to be remove straight away. Or you could add
> an additional list (that as you stated before you don't want) and call
> spice_usb_backend_device_eject at some other time (not clear when) but this
> should not be done automatically on SCSI state change as this would trigger
> a use-after-free as the reference counter get negative.
> Or you would have to add a rule that you need to call
> spice_usb_backend_device_eject only if you don't set delete_on_eject.
> Honestly these rules looks to me weird and complicated.
> Resource management usually has some simple rules like "if you allocate
> with malloc release with free" or "if you need to keep a reference
> call XXX_ref, if you need to remove a reference call XXX_unref",
> quite far from the APIs you are proposing.
> My test did some simple assumption, "if you get a reference to an
> emulated device you can remove it with spice_usb_backend_device_unref",
> as the emulated device is a SpiceUsbBackendDevice it seems logical
> to me.
>
> It seems more consistent to me if either the "devices" list holds the
> "main" (say initial) reference to the emulated device so in case
> spice_usb_backend_device_eject is called by the SCSI layer all it has
> to do is calling the hotplug function to remove from this list.
> For this reason the call to spice_usb_backend_device_unref in
> spice_usb_backend_device_eject seems wrong to me.
> If the device is not released by spice_usb_backend_device_eject will
> be released when the usb device manager release "devices".
> The only problem (currently) is that own_devices_mask bit is not cleared
> (not that is impossible to do it). I also would consider corrent to
> clear own_devices_mask bit when the device is released in order to
> avoid to have two emulated devices with the same address at the same
> time.
>
> > >
> > > > >
> > > > > > +}
> > > > > > +
> > > > > > +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 at redhat.com>
> > > > > > +
> > > > > > + 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
> > > >
> >
More information about the Spice-devel
mailing list