Re: [spice-gtk v2 1/8] usb-redir: define interfaces to support emulated devices

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Virtualization]     [Linux Virtualization]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]