[Spice-devel] [spice-gtk v1 1/2] usb-redirection: introduce usb backend layer
Christophe Fergeau
cfergeau at redhat.com
Tue Sep 25 13:39:12 UTC 2018
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.. ;)
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?
> + 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'?
> + UsbDeviceInformation device_info;
> +};
> +
> +struct _SpiceUsbBackend
I think I would call this SpiceUsbContext
> +{
> + 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.
> +};
> +
> +/* 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 (?)
> +
> +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?
> + 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...
> + 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?
> +{
> + 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.
> +
> + 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?
> +{
> + 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?
> +{
> + 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?
> +{
> + 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
> + 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.
> +{
> + 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 (*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 --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180925/55b12bff/attachment-0001.sig>
More information about the Spice-devel
mailing list