[Spice-devel] [spice-gtk v1 1/2] usb-redirection: introduce usb backend layer
Yuri Benditovich
yuri.benditovich at daynix.com
Thu Sep 27 16:18:25 UTC 2018
On Tue, Sep 25, 2018 at 4:39 PM Christophe Fergeau <cfergeau at redhat.com>
wrote:
> Hey,
>
> Thanks for doing some splitting work!
>
> I would personally squash these 2 patches, as there is some code
> movement between the new file introduced in this patch, and the files
> which are modified in the other one.
> You seem to have left out some of the comments I made previously (the
> __FUNCTION__ comment for example), replying to these suggestions would
> be nice.. ;)
>
In spice-gtk logs the function name is never printed if not explicitly
requested.
As I can see in gmacros.h
/* Provide a string identifying the current code position */
#if defined(__GNUC__) && (__GNUC__ < 3) && !defined(__cplusplus)
#define G_STRLOC __FILE__ ":" G_STRINGIFY (__LINE__) ":"
__PRETTY_FUNCTION__ "()"
#else
#define G_STRLOC __FILE__ ":" G_STRINGIFY (__LINE__)
#endif
>
> On Mon, Sep 24, 2018 at 11:43:54AM +0300, Yuri Benditovich wrote:
> > This layer communicates with libusb and libusbredir and
> > provides the API for USB redirection procedures.
> > In future all the modules of spice-gtk will communicate
> > only with usb backend instead of calling libusb and
> > usbredirhost directly. This is prerequisite of further
> > implementation of cd-sharing via USB redirection.
> >
> > Signed-off-by: Yuri Benditovich <yuri.benditovich at daynix.com>
> > ---
> > src/usb-backend-common.c | 809
> +++++++++++++++++++++++++++++++++++++++++++++++
> > src/usb-backend.h | 97 ++++++
> > 2 files changed, 906 insertions(+)
> > create mode 100644 src/usb-backend-common.c
> > create mode 100644 src/usb-backend.h
> >
> > diff --git a/src/usb-backend-common.c b/src/usb-backend-common.c
> > new file mode 100644
> > index 0000000..b3963ad
> > --- /dev/null
> > +++ b/src/usb-backend-common.c
> > @@ -0,0 +1,809 @@
> > +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> > +/*
> > + Copyright (C) 2018 Red Hat, Inc.
> > +
> > + Red Hat Authors:
> > + Yuri Benditovich<ybendito at redhat.com>
>
> You probably want to copy some of the authors from the files you are
> copying code from, and reuse the (C) date from there too
> > +
> > +struct _SpiceUsbBackendDevice
> > +{
> > + union
> > + {
> > + void *libusb_device;
>
> Why void * rather than the proper libusb_device * type?
>
> > + void *msc;
> > + } d;
> > + uint32_t isLibUsb : 1;
>
> Should be name "is_libusb", but it's really used in this patch.
>
> > + uint32_t configured : 1;
>
> This is not used at all. I think using 'bool' rather than a bitfield
> should be good enough.
>
> > + int refCount;
>
> 'ref_count'. Have you considered making this a GObject which would give
> you the refcounting?
>
>
IMO, less code is better and making it GObject does not provide any added
value.
> > + void *mutex;
>
> A proper type would be good to have here, but see my comments in the
> acquire/release methods.
>
> > + SpiceUsbBackendChannel *attached_to;
>
> Or just 'backend_channel'?
>
>
Please let me know whether this change is mandatory.
I'm fine with existing name.
> > + UsbDeviceInformation device_info;
> > +};
> > +
> > +struct _SpiceUsbBackend
>
> I think I would call this SpiceUsbContext
>
> Please let me know whether this change is mandatory.
I'm fine with existing name.
> > +{
> > + libusb_context *libusbContext;
>
> No CamelCase.
>
>
> > + usb_hot_plug_callback hp_callback;
>
> usb_hotplug_callback
>
> > + void *hp_user_data;
> > + libusb_hotplug_callback_handle hp_handle;
>
> s/hp_/hotplug_
>
> > + void *dev_change_user_data;
> > + uint32_t suppressed : 1;
>
> This is not used in this patch save for:
> #ifndef USE_CD_SHARING
> be->suppressed = TRUE;
> #endif
>
> Later it will only be used for
> if (!be->suppressed) {
> ...
> }
> Not clear why this code block could not be surrounded by #ifndef
> USE_CD_SHARING, which would get rid of the code altogether when
> cd sharing was disabled at compile time.
>
>
For now I remove till cd sharing commit.
In general, I would like to avoid making code like a zebra.
All the cd sharing functionality is disabled this is undefined.
If this is mandatory requirement for future commits, please let me know.
> > +};
> > +
> > +/* backend object for device change notification */
> > +static SpiceUsbBackend *notify_backend;
>
> This is not used at the moment. Looking at the final state of the series
> you sent previously, you need that global variable at one point because
> otherwise you have no way of getting the SpiceUsbBackend associated with
> the device you are currently processing. If SpiceUsbBackendDevice is a
> GObject,
> it seems this stuff could be replaced by a
> SpiceUsbBackendDevice::lun-changed signal (?)
>
This is because SpiceUsbBackendDevice is not associated with
SpiceUsbBackend, not because it is not a GObject. This notify routine
required only for updating the widget upon event in CD device.
I'll remove it for now till cd sharing.
>
> > +
> > +struct _SpiceUsbBackendChannel
> > +{
> > + struct usbredirhost *usbredirhost;
> > + uint8_t *read_buf;
> > + int read_buf_size;
> > + struct usbredirfilter_rule *rules;
> > + int rules_count;
>
> I could not find where rules/rules_count are initialized in the current
> patch, is there something missing?
>
The SpiceUsbBackendChannel is allocated with new0.
>
> > + uint32_t rejected : 1;
>
> Could be bool, but not used.
>
> > + SpiceUsbBackendDevice *attached;
>
> I think I'd name this backend_device.
>
> > + SpiceUsbBackendChannelInitData data;
>
> Better name than 'data' needed.
>
> > +};
> > +
> > +static const char *spice_usbutil_libusb_strerror(enum libusb_error
> error_code)
> > +{
> > + switch (error_code) {
> > + case LIBUSB_SUCCESS:
> > + return "Success";
> > + case LIBUSB_ERROR_IO:
> > + return "Input/output error";
> > + case LIBUSB_ERROR_INVALID_PARAM:
> > + return "Invalid parameter";
> > + case LIBUSB_ERROR_ACCESS:
> > + return "Access denied (insufficient permissions)";
> > + case LIBUSB_ERROR_NO_DEVICE:
> > + return "No such device (it may have been disconnected)";
> > + case LIBUSB_ERROR_NOT_FOUND:
> > + return "Entity not found";
> > + case LIBUSB_ERROR_BUSY:
> > + return "Resource busy";
> > + case LIBUSB_ERROR_TIMEOUT:
> > + return "Operation timed out";
> > + case LIBUSB_ERROR_OVERFLOW:
> > + return "Overflow";
> > + case LIBUSB_ERROR_PIPE:
> > + return "Pipe error";
> > + case LIBUSB_ERROR_INTERRUPTED:
> > + return "System call interrupted (perhaps due to signal)";
> > + case LIBUSB_ERROR_NO_MEM:
> > + return "Insufficient memory";
> > + case LIBUSB_ERROR_NOT_SUPPORTED:
> > + return "Operation not supported or unimplemented on this
> platform";
> > + case LIBUSB_ERROR_OTHER:
> > + return "Other error";
> > + }
> > + return "Unknown error";
> > +}
> > +
> > +// lock functions for usbredirhost and usbredirparser
> > +static void *usbredir_alloc_lock(void) {
> > + GMutex *mutex;
> > +
> > + mutex = g_new0(GMutex, 1);
> > + g_mutex_init(mutex);
> > +
> > + return mutex;
> > +}
> > +
> > +static void usbredir_free_lock(void *user_data) {
> > + GMutex *mutex = user_data;
> > +
> > + g_mutex_clear(mutex);
> > + g_free(mutex);
> > +}
> > +
> > +static void usbredir_lock_lock(void *user_data) {
> > + GMutex *mutex = user_data;
> > +
> > + g_mutex_lock(mutex);
> > +}
> > +
> > +static void usbredir_unlock_lock(void *user_data) {
> > + GMutex *mutex = user_data;
> > +
> > + g_mutex_unlock(mutex);
> > +}
> > +
> > +static gboolean fill_usb_info(SpiceUsbBackendDevice *bdev)
> > +{
> > + UsbDeviceInformation *pi = &bdev->device_info;
>
> pi? what about info?
>
> > +
> > + if (bdev->isLibUsb)
> > + {
> > + struct libusb_device_descriptor desc;
> > + libusb_device *libdev = bdev->d.libusb_device;
> > + int res = libusb_get_device_descriptor(libdev, &desc);
> > + pi->bus = libusb_get_bus_number(libdev);
> > + pi->address = libusb_get_device_address(libdev);
> > + if (res < 0) {
> > + g_warning("cannot get device descriptor for (%p) %d.%d",
> > + libdev, pi->bus, pi->address);
> > + return FALSE;
> > + }
> > + pi->vid = desc.idVendor;
> > + pi->pid = desc.idProduct;
> > + pi->class = desc.bDeviceClass;
> > + pi->subclass = desc.bDeviceSubClass;
> > + pi->protocol = desc.bDeviceProtocol;
> > + pi->isochronous = 0;
> > + pi->max_luns = 0;
> > + }
> > + return TRUE;
> > +}
> > +
> > +/* Note that this function must be re-entrant safe, as it can get called
> > +from both the main thread as well as from the usb event handling thread
> */
> > +static void usbredir_write_flush_callback(void *user_data)
> > +{
> > + SpiceUsbBackendChannel *ch = user_data;
> > + gboolean b = ch->data.is_channel_ready(ch->data.user_data);
>
> backend_channel and ready would be far better names...
>
I understand that you'd like to change many names.
The 'ch' in this file is consistently designates backend channel and
nothing other.
Please let me know whether this change is mandatory.
Note also many one-letter and two letters identifiers in existing code.
>
> > + if (b) {
> > + if (ch->usbredirhost) {
> > + SPICE_DEBUG("%s ch %p -> usbredirhost", __FUNCTION__, ch);
> > + usbredirhost_write_guest_data(ch->usbredirhost);
> > + }
> > + else {
> > + b = FALSE;
> > + }
> > + }
> > +
> > + if (!b) {
>
> Could go in an } else { block
>
> > + SPICE_DEBUG("%s ch %p (not ready)", __FUNCTION__, ch);
> > + }
> > +}
> > +
> > +#ifdef INTERCEPT_LOG
> > +static void log_handler(
> > + const gchar *log_domain,
> > + GLogLevelFlags log_level,
> > + const gchar *message,
> > + gpointer user_data)
>
> Indentation is wrong
>
> > +{
> > + GString *log_msg;
> > + log_msg = g_string_new(NULL);
> > + if (log_msg)
> > + {
> > + gchar *timestamp;
> > + GThread *th = g_thread_self();
> > + GDateTime *current_time = g_date_time_new_now_local();
> > + gint micros = g_date_time_get_microsecond(current_time);
> > + timestamp = g_date_time_format(current_time, "%H:%M:%S");
> > + g_string_append_printf(log_msg, "[%p][%s.%03d]", th, timestamp,
> micros / 1000);
> > + g_date_time_unref(current_time);
> > + g_free(timestamp);
> > + g_string_append(log_msg, message);
> > +#ifdef INTERCEPT_LOG2FILE
> > + g_string_append(log_msg, "\n");
> > + fwrite(log_msg->str, 1, strlen(log_msg->str), fLog);
> > +#else
> > + g_log_default_handler(log_domain, log_level, log_msg->str,
> NULL);
> > +#endif
> > + g_string_free(log_msg, TRUE);
> > + }
> > +}
> > +#endif
> > +
> > +static void configure_log(void)
> > +{
> > +#ifdef INTERCEPT_LOG2FILE
> > + fLog = fopen("remote-viewer.log", "w+t");
> > +#endif
> > +#ifdef INTERCEPT_LOG
> > + g_log_set_default_handler(log_handler, NULL);
> > +#endif
> > +}
>
> I'd drop that logging stuff, or move it to a separate commit. Imo this
> is better addressed with log categories, or grep+redirection, and I
> don't think this is something that should be specific to the usb code.
>
> > +
> > +SpiceUsbBackend *spice_usb_backend_initialize(void)
> > +{
> > + SpiceUsbBackend *be;
> > + SPICE_DEBUG("%s >>", __FUNCTION__);
> > + if (!g_mutex) {
> > + g_mutex = usbredir_alloc_lock();
> > + configure_log();
> > + }
> > + be = (SpiceUsbBackend *)g_new0(SpiceUsbBackend, 1);
> > +#ifndef USE_CD_SHARING
> > + be->suppressed = TRUE;
> > +#endif
> > + int rc;
> > + rc = libusb_init(&be->libusbContext);
> > + if (rc < 0) {
> > + const char *desc = spice_usbutil_libusb_strerror(rc);
> > + g_warning("Error initializing LIBUSB support: %s [%i]", desc,
> rc);
>
> Maybe have a GError **error argument so that we can properly report
> failures? This is what the initial code was doing.
>
> > + } else {
> > +#ifdef G_OS_WIN32
> > +#if LIBUSB_API_VERSION >= 0x01000106
> > + libusb_set_option(be->libusbContext, LIBUSB_OPTION_USE_USBDK);
> > +#endif
> > +#endif
> > + }
> > + SPICE_DEBUG("%s <<", __FUNCTION__);
> > + return be;
> > +}
> > +
> > +gboolean spice_usb_backend_handle_events(SpiceUsbBackend *be)
>
> s/be/backend?
>
>
Please let me know whether this change is mandatory.
> > +{
> > + SPICE_DEBUG("%s >>", __FUNCTION__);
> > + gboolean b = TRUE;
>
> Something more expressive than 'b' would be nice.
>
>
> > + if (be->libusbContext) {
> > + SPICE_DEBUG("%s >> libusb", __FUNCTION__);
> > + int res = libusb_handle_events(be->libusbContext);
> > + if (res && res != LIBUSB_ERROR_INTERRUPTED) {
>
> ((res != 0) && (res != LIBUSB_ERROR_INTERRUPTED)) maybe?
>
> > + const char *desc = spice_usbutil_libusb_strerror(res);
> > + g_warning("Error handling USB events: %s [%i]", desc, res);
> > + b = FALSE;
> > + }
> > + SPICE_DEBUG("%s << libusb %d", __FUNCTION__, res);
> > + }
> > + else {
> > + b = TRUE;
> > + g_usleep(1000000);
>
> I don't think that belongs to this commit.
>
> > + }
> > + SPICE_DEBUG("%s <<", __FUNCTION__);
> > + return b;
> > +}
> > +
> > +static int LIBUSB_CALL hotplug_callback(libusb_context *ctx,
> > + libusb_device *device,
> > + libusb_hotplug_event event,
> > + void *user_data)
>
> indentation
>
> > +{
> > + SpiceUsbBackend *be = (SpiceUsbBackend *)user_data;
> > + if (be->hp_callback) {
> > + SpiceUsbBackendDevice *d;
>
> Once again, 'd' and 'be' naming could be improved
>
> > + gboolean val = event == LIBUSB_HOTPLUG_EVENT_DEVICE_ARRIVED;
>
> 'val' naming. I usually prefer to add () around the condition in such
> assignments.
>
> > + d = g_new0(SpiceUsbBackendDevice, 1);
> > + d->isLibUsb = 1;
> > + d->refCount = 1;
> > + d->mutex = g_mutex;
> > + d->d.libusb_device = device;
> > + if (fill_usb_info(d)) {
> > + SPICE_DEBUG("created dev %p, usblib dev %p", d, device);
> > + be->hp_callback(be->hp_user_data, d, val);
> > + } else {
> > + g_free(d);
> > + }
> > + }
> > + return 0;
> > +}
> > +
> > +gboolean spice_usb_backend_handle_hotplug(
> > + SpiceUsbBackend *be,
> > + void *user_data,
> > + usb_hot_plug_callback proc)
>
> Indentation
>
> > +{
> > + int rc;
> > + if (!proc) {
> > + if (be->hp_handle) {
> > + libusb_hotplug_deregister_callback(be->libusbContext,
> be->hp_handle);
> > + be->hp_handle = 0;
> > + }
> > + be->hp_callback = proc;
> > + return TRUE;
> > + }
> > +
> > + be->hp_callback = proc;
> > + be->hp_user_data = user_data;
> > + if (!be->libusbContext) {
> > + // it is acceptable if libusb is not available at all
>
> Not in this commit I think?
>
> > + return TRUE;
> > + }
> > + rc = libusb_hotplug_register_callback(be->libusbContext,
> > + LIBUSB_HOTPLUG_EVENT_DEVICE_ARRIVED |
> LIBUSB_HOTPLUG_EVENT_DEVICE_LEFT,
> > + LIBUSB_HOTPLUG_ENUMERATE, LIBUSB_HOTPLUG_MATCH_ANY,
> > + LIBUSB_HOTPLUG_MATCH_ANY, LIBUSB_HOTPLUG_MATCH_ANY,
> > + hotplug_callback, be, &be->hp_handle);
> > + if (rc != LIBUSB_SUCCESS) {
> > + const char *desc = spice_usbutil_libusb_strerror(rc);
> > + g_warning("Error initializing USB hotplug support: %s [%i]",
> desc, rc);
>
> This could be reported through a GError
>
> > + be->hp_callback = NULL;
> > + return FALSE;
> > + }
> > + return TRUE;
> > +}
> > +
> > +void spice_usb_backend_finalize(SpiceUsbBackend *be)
> > +{
> > + SPICE_DEBUG("%s >>", __FUNCTION__);
> > + if (be->libusbContext) {
> > + libusb_exit(be->libusbContext);
> > + }
> > + if (be == notify_backend) {
> > + notify_backend = NULL;
> > + }
> > + g_free(be);
> > + SPICE_DEBUG("%s <<", __FUNCTION__);
> > +}
> > +
> > +SpiceUsbBackendDevice
> **spice_usb_backend_get_device_list(SpiceUsbBackend *be)
> > +{
> > + LOUD_DEBUG("%s >>", __FUNCTION__);
> > + libusb_device **devlist = NULL, **dev;
> > + SpiceUsbBackendDevice *d, **list;
> > +
> > + int n = 0, index;
> > +
> > + if (be->libusbContext) {
> > + libusb_get_device_list(be->libusbContext, &devlist);
> > + }
> > +
> > + // add all the libusb device that not present in our list
> > + for (dev = devlist; dev && *dev; dev++) {
> > + n++;
> > + }
> > +
> > + list = g_new0(SpiceUsbBackendDevice*, n + 1);
> > +
> > + index = 0;
> > +
> > + for (dev = devlist; dev && *dev; dev++) {
> > + d = g_new0(SpiceUsbBackendDevice, 1);
> > + d->isLibUsb = 1;
> > + d->refCount = 1;
> > + d->mutex = g_mutex;
> > + d->d.libusb_device = *dev;
>
> You want a spice_usb_backend_device_new() method as this code is
> duplicated twice.
>
> > + if (index >= n || !fill_usb_info(d)) {
> > + g_free(d);
> > + libusb_unref_device(*dev);
> > + }
> > + else {
> > + SPICE_DEBUG("created dev %p, usblib dev %p", d, *dev);
> > + list[index++] = d;
> > + }
> > + }
>
> Have you considered using GPtrArray? It looks like it would avoid the
> iteration to count the number of items, and make that function overall
> simpler.
>
>
IMO, this does not make the function simpler and requires additional
changes in callers code.
> > +
> > + if (devlist) {
> > + libusb_free_device_list(devlist, 0);
> > + }
> > +
> > + LOUD_DEBUG("%s <<", __FUNCTION__);
> > + return list;
> > +}
> > +
> > +gboolean spice_usb_backend_device_is_hub(SpiceUsbBackendDevice *dev)
> > +{
> > + return dev->device_info.class == LIBUSB_CLASS_HUB;
> > +}
> > +
> > +static uint8_t is_libusb_isochronous(libusb_device *libdev)
>
> This could return bool I think?
>
>
Please let me know whether this change is mandatory.
> > +{
> > + struct libusb_config_descriptor *conf_desc;
> > + uint8_t isoc_found = FALSE;
> > + gint i, j, k;
> > +
> > + if (!libdev) {
> > + SPICE_DEBUG("%s - unexpected libdev = 0", __FUNCTION__);
> > + return 0;
> > + }
>
> If we should never pass a NULL libdev, I would keep the
> g_return_val_if_fail() the initial code has
>
> > +
> > + if (libusb_get_active_config_descriptor(libdev, &conf_desc) != 0) {
> > + SPICE_DEBUG("%s - no active configuration for libdev %p",
> __FUNCTION__, libdev);
> > + return 0;
>
> Initial code had a more verbose g_return_val_if_reached()
>
> > + }
> > +
> > + for (i = 0; !isoc_found && i < conf_desc->bNumInterfaces; i++) {
> > + for (j = 0; !isoc_found && j <
> conf_desc->interface[i].num_altsetting; j++) {
> > + for (k = 0; !isoc_found && k <
> conf_desc->interface[i].altsetting[j].bNumEndpoints;k++) {
> > + gint attributes =
> conf_desc->interface[i].altsetting[j].endpoint[k].bmAttributes;
> > + gint type = attributes & LIBUSB_TRANSFER_TYPE_MASK;
> > + if (type == LIBUSB_TRANSFER_TYPE_ISOCHRONOUS)
> > + isoc_found = TRUE;
> > + }
> > + }
> > + }
> > +
> > + libusb_free_config_descriptor(conf_desc);
> > + return isoc_found;
> > +}
> > +
> > +const UsbDeviceInformation*
> spice_usb_backend_device_get_info(SpiceUsbBackendDevice *dev)
>
> spacing looks off, should be const UsbDeviceInformation
> *spice_usb_backend_device_get_info
>
> > +{
> > + dev->device_info.isochronous = dev->isLibUsb ?
> is_libusb_isochronous(dev->d.libusb_device) : 0;
>
> Why is it not done in fill_info?
>
> > + return &dev->device_info;
> > +}
> > +
> > +gboolean spice_usb_backend_devices_same(
> > + SpiceUsbBackendDevice *dev1,
> > + SpiceUsbBackendDevice *dev2)
>
> Indentation
>
> > +{
> > + if (dev1->isLibUsb != dev2->isLibUsb) {
> > + return FALSE;
> > + }
> > + if (dev1->isLibUsb) {
> > + return dev1->d.libusb_device == dev2->d.libusb_device;
> > + }
> > + // assuming CD redir devices are static
> > + return dev1 == dev2;
> > +}
> > +
> > +gconstpointer spice_usb_backend_device_get_libdev(SpiceUsbBackendDevice
> *dev)
> > +{
> > + if (dev->isLibUsb) {
> > + return dev->d.libusb_device;
> > + }
> > + return NULL;
> > +}
> > +
> > +void spice_usb_backend_free_device_list(SpiceUsbBackendDevice **devlist)
> > +{
> > + LOUD_DEBUG("%s >>", __FUNCTION__);
> > + SpiceUsbBackendDevice **dev;
> > + for (dev = devlist; *dev; dev++) {
> > + SpiceUsbBackendDevice *d = *dev;
> > + spice_usb_backend_device_release(d);
> > + }
> > + g_free(devlist);
> > + LOUD_DEBUG("%s <<", __FUNCTION__);
> > +}
> > +
> > +void spice_usb_backend_device_acquire(SpiceUsbBackendDevice *dev)
> > +{
> > + void *mutex = dev->mutex;
> > + LOUD_DEBUG("%s >> %p", __FUNCTION__, dev);
> > + usbredir_lock_lock(mutex);
> > + if (dev->isLibUsb) {
> > + libusb_ref_device(dev->d.libusb_device);
> > + }
> > + dev->refCount++;
> > + usbredir_unlock_lock(mutex);
>
> Why do you need to raise both 'libusb_device' refcount, and 'dev'
> refcount? It would seem to me that the ref on 'dev' would be enough, as
> as long as it's alive thanks to that ref, it won't release the ref it
> owns on its 'libusb_device'? If it can be done that way, then updating
> the refcount can be done with an atomic operation, and you don't need
> g_mutex anymore.
>
> > +}
> > +
> > +void spice_usb_backend_device_release(SpiceUsbBackendDevice *dev)
>
> Why not _ref/_unref btw?
>
>
Due to personal preference.
Please let me know whether this change is mandatory.
> > +{
> > + void *mutex = dev->mutex;
> > + LOUD_DEBUG("%s >> %p(%d)", __FUNCTION__, dev, dev->refCount);
> > + usbredir_lock_lock(mutex);
> > + if (dev->isLibUsb) {
> > + libusb_unref_device(dev->d.libusb_device);
> > + dev->refCount--;
> > + if (dev->refCount == 0) {
> > + LOUD_DEBUG("%s freeing %p (libusb %p)", __FUNCTION__, dev,
> dev->d.libusb_device);
> > + g_free(dev);
> > + }
> > + }
> > + else {
> > + dev->refCount--;
>
> This codepath won't get trigger here. However, in the previous series
> that you sent, it's done this way in the final patch. Is this correct?
> It's odd to always decrease this without never checking the value..
>
> > + }
> > + usbredir_unlock_lock(mutex);
> > + LOUD_DEBUG("%s <<", __FUNCTION__);
> > +}
> > +
> > +gboolean spice_usb_backend_device_need_thread(SpiceUsbBackendDevice
> *dev)
>
> 'needs_thread' I'd say. However it should probably be introduced later.
>
> > +{
> > + gboolean b = dev->isLibUsb != 0;
> > + SPICE_DEBUG("%s << %d", __FUNCTION__, b);
> > + return b;
> > +}
> > +
> > +int spice_usb_backend_device_check_filter(
> > + SpiceUsbBackendDevice *dev,
> > + const struct usbredirfilter_rule *rules,
> > + int count)
> > +{
> > + if (dev->isLibUsb) {
> > + return usbredirhost_check_device_filter(
> > + rules, count, dev->d.libusb_device, 0);
> > + }
> > + return -1;
> > +}
> > +
> > +static int usbredir_read_callback(void *user_data, uint8_t *data, int
> count)
> > +{
> > + SpiceUsbBackendChannel *ch = user_data;
> > +
> > + count = MIN(ch->read_buf_size, count);
> > +
> > + if (count != 0) {
> > + memcpy(data, ch->read_buf, count);
> > + }
> > +
> > + ch->read_buf_size -= count;
> > + if (ch->read_buf_size) {
> > + ch->read_buf += count;
> > + }
> > + else {
> > + ch->read_buf = NULL;
> > + }
> > + SPICE_DEBUG("%s ch %p, %d bytes", __FUNCTION__, ch, count);
> > +
> > + return count;
> > +}
> > +
> > +static const char *strip_usbredir_prefix(const char *msg)
> > +{
> > + if (strncmp(msg, "usbredirhost: ", 14) == 0) {
> > + msg += 14;
> > + }
> > + return msg;
> > +}
> > +
> > +static void usbredir_log(void *user_data, int level, const char *msg)
> > +{
> > + SpiceUsbBackendChannel *ch = (SpiceUsbBackendChannel *)user_data;
> > + const char *stripped_msg = strip_usbredir_prefix(msg);
> > + switch (level) {
> > + case usbredirparser_error:
> > + g_critical("%s", msg);
> > + ch->data.log(ch->data.user_data, stripped_msg, TRUE);
> > + break;
> > + case usbredirparser_warning:
> > + g_warning("%s", msg);
> > + ch->data.log(ch->data.user_data, stripped_msg, TRUE);
> > + break;
> > + default:
> > + ch->data.log(ch->data.user_data, stripped_msg, FALSE);
> > + break;
>
> usbredir_log in channel-usbredir.c will not do anything when the last
> arg is FALSE, in other words it only cares about errors. I would turn
> the 'log' callback into an 'error' callback and pass a GError.
>
> > + }
> > +}
> > +
> > +static int usbredir_write_callback(void *user_data, uint8_t *data, int
> count)
> > +{
> > + SpiceUsbBackendChannel *ch = user_data;
> > + int res;
> > + SPICE_DEBUG("%s ch %p, %d bytes", __FUNCTION__, ch, count);
> > + res = ch->data.write_callback(ch->data.user_data, data, count);
> > + return res;
> > +}
> > +
> > +#if USBREDIR_VERSION >= 0x000701
> > +static uint64_t usbredir_buffered_output_size_callback(void *user_data)
> > +{
> > + SpiceUsbBackendChannel *ch = user_data;
> > + return ch->data.get_queue_size(ch->data.user_data);
> > +}
> > +#endif
> > +
> > +int spice_usb_backend_provide_read_data(SpiceUsbBackendChannel *ch,
> uint8_t *data, int count)
> > +{
>
> The initial code had a
> - /* No recursion allowed! */
> - g_return_if_fail(priv->read_buf == NULL);
> guard which is gone now.
>
> > + int res = 0;
> > + if (!ch->read_buf) {
> > + typedef int(*readproc_t)(void *);
> > + readproc_t fn = NULL;
> > + void *param;
> > + ch->read_buf = data;
> > + ch->read_buf_size = count;
> > + if (ch->usbredirhost) {
> > + fn = (readproc_t)usbredirhost_read_guest_data;
> > + param = ch->usbredirhost;
> > + }
> > + res = fn ? fn(param) : USB_REDIR_ERROR_IO;
>
> Why not this:
> if (ch->usbredirhost) {
> res = usbredirhost_read_guest_data(ch->usbredirhost);
> } else {
> res = USB_REDIR_ERROR_IO;
> }
> ?
>
>
>
> > + switch (res)
> > + {
> > + case usbredirhost_read_io_error:
> > + res = USB_REDIR_ERROR_IO;
> > + break;
> > + case usbredirhost_read_parse_error:
> > + res = USB_REDIR_ERROR_READ_PARSE;
> > + break;
> > + case usbredirhost_read_device_rejected:
> > + res = USB_REDIR_ERROR_DEV_REJECTED;
> > + break;
> > + case usbredirhost_read_device_lost:
> > + res = USB_REDIR_ERROR_DEV_LOST;
> > + break;
> > + }
> > + SPICE_DEBUG("%s ch %p, %d bytes, res %d", __FUNCTION__, ch,
> count, res);
> > +
> > + } else {
> > + res = USB_REDIR_ERROR_READ_PARSE;
> > + SPICE_DEBUG("%s ch %p, %d bytes, already has data",
> __FUNCTION__, ch, count);
> > + }
> > + if (ch->rejected) {
> > + ch->rejected = 0;
> > + res = USB_REDIR_ERROR_DEV_REJECTED;
> > + }
> > + return res;
> > +}
> > +
> > +void spice_usb_backend_return_write_data(SpiceUsbBackendChannel *ch,
> void *data)
>
> _release_write_data?
>
>
Please let me know whether this change is mandatory.
> > +{
> > + typedef void(*retdata)(void *, void *);
> > + retdata fn = NULL;
> > + void *param;
> > + if (ch->usbredirhost) {
> > + fn = (retdata)usbredirhost_free_write_buffer;
> > + param = ch->usbredirhost;
> > + }
> > + if (fn) {
> > + SPICE_DEBUG("%s ch %p", __FUNCTION__, ch);
> > + fn(param, data);
> > + } else {
> > + SPICE_DEBUG("%s ch %p - NOBODY TO CALL", __FUNCTION__, ch);
> > + }
>
> Same question as above,
> if (ch->usbredirhost) {
> SPICE_DEBUG("%s ch %p", __FUNCTION__, ch);
> usbredirhost_free_write_buffer(ch->usbredirhost);
> } else {
> SPICE_DEBUG("%s ch %p - NOBODY TO CALL", __FUNCTION__, ch);
> }
> is both simpler and easier to read.
>
> > +}
> > +
> > +gboolean spice_usb_backend_channel_attach(SpiceUsbBackendChannel *ch,
> SpiceUsbBackendDevice *dev, const char **msg)
> > +{
> > + const char *dummy;
> > + if (!msg) {
> > + msg = &dummy;
> > + }
>
> It seems you could replace msg with a GError
>
> Please let me know whether this change is mandatory.
> > + SPICE_DEBUG("%s >> ch %p, dev %p (was attached %p)", __FUNCTION__,
> ch, dev, ch->attached);
> > + gboolean b = FALSE;
>
> Naming...
>
>
> > + if (!dev) {
> > + return b;
> > + }
> > +
> > + if (dev->isLibUsb) {
> > + libusb_device_handle *handle = NULL;
> > + int rc = libusb_open(dev->d.libusb_device, &handle);
> > + b = rc == 0 && handle;
>
> Do you really need to test both?
>
> http://libusb.sourceforge.net/api-1.0/group__dev.html#ga8163100afdf933fabed0db7fa81c89d1
> says
> "handle: output location for the returned device handle pointer. Only
> populated when the return code is 0." so it seems testing 'rc' should be
> enough? I guess I would set 'b' here, and just have if (rc == 0 &&
> handle != NULL) {
> ...
> }
>
> > + if (b) {
> > + rc = usbredirhost_set_device(ch->usbredirhost, handle);
> > + if (rc) {
> > + SPICE_DEBUG("%s ch %p, dev %p usbredirhost error %d",
> __FUNCTION__, ch, dev, rc);
> > + b = FALSE;
> > + } else {
> > + ch->attached = dev;
> > + dev->attached_to = ch;
> > + }
> > + } else {
> > + const char *desc = spice_usbutil_libusb_strerror(rc);
> > + g_warning("Error libusb_open: %s [%i]", desc, rc);
> > + *msg = desc;
> > + }
> > + }
> > +
> > + return b;
> > +}
> > +
> > +void spice_usb_backend_channel_detach(SpiceUsbBackendChannel *ch)
> > +{
> > + SPICE_DEBUG("%s >> ch %p, was attached %p", __FUNCTION__, ch,
> ch->attached);
> > + if (!ch->attached) {
> > + SPICE_DEBUG("%s: nothing to detach", __FUNCTION__);
> > + return;
> > + }
> > + if (ch->usbredirhost) {
> > + // it will call libusb_close internally
> > + usbredirhost_set_device(ch->usbredirhost, NULL);
> > + }
> > + SPICE_DEBUG("%s ch %p, detach done", __FUNCTION__, ch);
> > + ch->attached->attached_to = NULL;
> > + ch->attached = NULL;
> > +}
> > +
> > +SpiceUsbBackendChannel *spice_usb_backend_channel_initialize(
> > + SpiceUsbBackend *be,
> > + const SpiceUsbBackendChannelInitData *init_data)
>
> Indentation
>
> > +{
> > + SpiceUsbBackendChannel *ch = g_new0(SpiceUsbBackendChannel, 1);
> > + SPICE_DEBUG("%s >>", __FUNCTION__);
> > + gboolean ok = FALSE;
> > + ch->data = *init_data;
> > + ch->usbredirhost = !be->libusbContext ? NULL :
> > + usbredirhost_open_full(
>
> Please,
> if (be->libusbContext == NULL) {
> ch->usbredirhost = NULL;
> ok = true;
> } else {
> ch->usbredirhost = usbredirhost_open_full(...);
> if (ch->usbredirhost != NULL) {
> #if USBREDIR_VERSION >= 0x000701
> usbredirhost_set_buffered_output_size_cb(ch->usbredirhost,
> usbredir_buffered_output_size_callback);
> #endif
> ok = true;
> }
> }
>
>
>
>
> > + be->libusbContext,
> > + NULL,
> > + usbredir_log,
> > + usbredir_read_callback,
> > + usbredir_write_callback,
> > + usbredir_write_flush_callback,
> > + usbredir_alloc_lock,
> > + usbredir_lock_lock,
> > + usbredir_unlock_lock,
> > + usbredir_free_lock,
> > + ch, PACKAGE_STRING,
> > + init_data->debug ? usbredirparser_debug :
> usbredirparser_warning,
>
> Why not call spice_util_get_debug() here rather than passing it through
> init_data?
>
> > + usbredirhost_fl_write_cb_owns_buffer);
> > + ok = be->libusbContext == NULL || ch->usbredirhost != NULL;
> > + if (ch->usbredirhost) {
> > +#if USBREDIR_VERSION >= 0x000701
> > + usbredirhost_set_buffered_output_size_cb(ch->usbredirhost,
> usbredir_buffered_output_size_callback);
> > +#endif
> > + }
> > +
> > + if (!ok) {
> > + g_free(ch);
> > + ch = NULL;
>
> The previous code was aborting (g_error()) when ch->usbredirhost was
> NULL, now we just return NULL, and apparently don't handle this case in the
> caller?
>
> > + }
> > + SPICE_DEBUG("%s << %p", __FUNCTION__, ch);
> > + return ch;
> > +}
> > +
> > +void spice_usb_backend_channel_up(SpiceUsbBackendChannel *ch)
> > +{
> > + SPICE_DEBUG("%s %p, host %p", __FUNCTION__, ch, ch->usbredirhost);
> > + if (ch->usbredirhost) {
> > + usbredirhost_write_guest_data(ch->usbredirhost);
> > + }
> > +}
> > +
> > +void spice_usb_backend_channel_finalize(SpiceUsbBackendChannel *ch)
>
> I think I would name it '_free', as _finalize for me is associated with
> gobject destruction, which is not what is happening here.
>
>
Please let me know whether this change is mandatory.
> > +{
> > + SPICE_DEBUG("%s >> %p", __FUNCTION__, ch);
> > + if (ch->usbredirhost) {
> > + usbredirhost_close(ch->usbredirhost);
> > + }
> > +
> > + if (ch->rules) {
> > + // is it ok to g_free the memory that was allocated by parser?
>
> Yes, I think you are supposed to free the memory for the rules you got
> from filter_filter_func. free() should be used though.
>
> > + g_free(ch->rules);
> > + }
> > +
> > + g_free(ch);
> > + SPICE_DEBUG("%s << %p", __FUNCTION__, ch);
> > +}
> > +
> > +void spice_usb_backend_channel_get_guest_filter(
> > + SpiceUsbBackendChannel *ch,
> > + const struct usbredirfilter_rule **r,
> > + int *count)
>
> Indentation
>
> > +{
> > + int i;
> > + *r = NULL;
> > + *count = 0;
> > + if (ch->usbredirhost) {
> > + usbredirhost_get_guest_filter(ch->usbredirhost, r, count);
> > + }
> > + if (*r == NULL) {
> > + *r = ch->rules;
> > + *count = ch->rules_count;
>
> Not really clear why we are not returning right away? This is what the
> initial code was doing
>
>
If there are guest filters, we print it out.
Please let me know whether removal of this printout is mandatory.
> > + }
> > +
> > + if (*count) {
> > + SPICE_DEBUG("%s ch %p: %d filters", __FUNCTION__, ch, *count);
> > + }
> > + for (i = 0; i < *count; i++) {
> > + const struct usbredirfilter_rule *ra = *r;
> > + SPICE_DEBUG("%s class %d, %X:%X",
> > + ra[i].allow ? "allowed" : "denied", ra[i].device_class,
> > + (uint32_t)ra[i].vendor_id, (uint32_t)ra[i].product_id);
> > + }
> > +}
> > +
> > +#endif // USB_REDIR
> > diff --git a/src/usb-backend.h b/src/usb-backend.h
> > new file mode 100644
> > index 0000000..f6a3604
> > --- /dev/null
> > +++ b/src/usb-backend.h
> > @@ -0,0 +1,97 @@
> > +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> > +/*
> > + Copyright (C) 2018 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_BACKEND_H__
> > +#define __SPICE_USB_BACKEND_H__
> > +
> > +#include <usbredirfilter.h>
> > +#include "usb-device-manager.h"
> > +
> > +G_BEGIN_DECLS
> > +
> > +typedef struct _SpiceUsbBackend SpiceUsbBackend;
> > +typedef struct _SpiceUsbBackendDevice SpiceUsbBackendDevice;
> > +typedef struct _SpiceUsbBackendChannel SpiceUsbBackendChannel;
> > +
> > +typedef struct UsbDeviceInformation
> > +{
> > + uint16_t bus;
> > + uint16_t address;
> > + uint16_t vid;
> > + uint16_t pid;
> > + uint8_t class;
> > + uint8_t subclass;
> > + uint8_t protocol;
> > + uint8_t isochronous;
> > + uint8_t max_luns;
>
> max_luns is not used.
>
> > +} UsbDeviceInformation;
> > +
> > +typedef struct SpiceUsbBackendChannelInitData
> > +{
> > + void *user_data;
> > + void (*log)(void *user_data, const char *msg, gboolean error);
> > + int (*write_callback)(void *user_data, uint8_t *data, int count);
> > + int (*is_channel_ready)(void *user_data);
> > + uint64_t (*get_queue_size)(void *user_data);
> > + gboolean debug;
> > +} SpiceUsbBackendChannelInitData;
> > +
> > +typedef void(*usb_hot_plug_callback)(
> > + void *user_data, SpiceUsbBackendDevice *dev, gboolean added);
>
> Indentation
>
> > +
> > +enum {
> > + USB_REDIR_ERROR_IO = -1,
> > + USB_REDIR_ERROR_READ_PARSE = -2,
> > + USB_REDIR_ERROR_DEV_REJECTED = -3,
> > + USB_REDIR_ERROR_DEV_LOST = -4,
> > +};
> > +
> > +SpiceUsbBackend *spice_usb_backend_initialize(void);
> > +gboolean spice_usb_backend_handle_events(SpiceUsbBackend *);
> > +gboolean spice_usb_backend_handle_hotplug(SpiceUsbBackend *, void
> *user_data, usb_hot_plug_callback proc);
> > +void spice_usb_backend_finalize(SpiceUsbBackend *context);
> > +// returns NULL-terminated array of SpiceUsbBackendDevice *
> > +SpiceUsbBackendDevice
> **spice_usb_backend_get_device_list(SpiceUsbBackend *backend);
> > +gboolean spice_usb_backend_device_is_hub(SpiceUsbBackendDevice *dev);
> > +gboolean spice_usb_backend_device_need_thread(SpiceUsbBackendDevice
> *dev);
> > +void spice_usb_backend_free_device_list(SpiceUsbBackendDevice
> **devlist);
> > +void spice_usb_backend_device_acquire(SpiceUsbBackendDevice *dev);
> > +void spice_usb_backend_device_release(SpiceUsbBackendDevice *dev);
> > +gboolean spice_usb_backend_devices_same(SpiceUsbBackendDevice *dev1,
> SpiceUsbBackendDevice *dev2);
> > +gconstpointer spice_usb_backend_device_get_libdev(SpiceUsbBackendDevice
> *dev);
> > +const UsbDeviceInformation*
> spice_usb_backend_device_get_info(SpiceUsbBackendDevice *dev);
> const UsbDeviceInformation *spice_usb_backend_device_get_info
>
>
> > +gboolean spice_usb_backend_device_get_info_by_address(guint8 bus,
> guint8 addr, UsbDeviceInformation *info);
>
> Extra space after gboolean
>
> > +// returns 0 if the device passes the filter
> > +int spice_usb_backend_device_check_filter(SpiceUsbBackendDevice *dev,
> const struct usbredirfilter_rule *rules, int count);
> > +
> > +SpiceUsbBackendChannel
> *spice_usb_backend_channel_initialize(SpiceUsbBackend *context, const
> SpiceUsbBackendChannelInitData *init_data);
> > +// returns 0 for success or error code
> > +int spice_usb_backend_provide_read_data(SpiceUsbBackendChannel *ch,
> uint8_t *data, int count);
> > +gboolean spice_usb_backend_channel_attach(SpiceUsbBackendChannel *ch,
> SpiceUsbBackendDevice *dev, const char **msg);
> > +void spice_usb_backend_channel_detach(SpiceUsbBackendChannel *ch);
> > +void spice_usb_backend_channel_up(SpiceUsbBackendChannel *ch);
> > +void spice_usb_backend_channel_get_guest_filter(SpiceUsbBackendChannel
> *ch, const struct usbredirfilter_rule **rules, int *count);
> > +void spice_usb_backend_return_write_data(SpiceUsbBackendChannel *ch,
> void *data);
> > +void spice_usb_backend_channel_finalize(SpiceUsbBackendChannel *ch);
> > +
> > +G_END_DECLS
> > +
> > +#endif
> > --
> > 2.9.4
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180927/c2ff65ba/attachment-0001.html>
More information about the Spice-devel
mailing list