[Spice-devel] [spice-gtk] [PATCH] Provide a method to check if a USB device matches a specific class, subclass and protocol

[Uri Lublin]

Hi Fabiano,

Please see some comments below.

On 11/11/2014 04:29 PM, Fabiano Fidêncio wrote:
> As we only can filter USB devices by their Classes and sometimes it is
> not enough (eg: I do not want to have Keyboard and Mouse, but want to
> have have Joysticks, being all of them part of HID Class), provide to
> the applications a way match the device itself with the desired Class,
> SubClass and Protocol, so the applications refine the filter provided
> by usbredir.

Note that the filter can act upon VID/PID such that any specific device can
have its own rule, e.g. -1, VID, PID, -1, 1
> ---
> This patch was written based on https://bugzilla.gnome.org/show_bug.cgi?id=698430
> Any suggestion to have a shorter short-log is welcome :-)
> ---
>   gtk/map-file             |  1 +
>   gtk/spice-glib-sym-file  |  1 +
>   gtk/usb-device-manager.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++
>   gtk/usb-device-manager.h |  1 +
>   4 files changed, 99 insertions(+)
>
> diff --git a/gtk/map-file b/gtk/map-file
> index 9f8d04e..6bbf407 100644
> --- a/gtk/map-file
> +++ b/gtk/map-file
> @@ -124,6 +124,7 @@ spice_usb_device_manager_get_devices;
>   spice_usb_device_manager_get_devices_with_filter;
>   spice_usb_device_manager_get_type;
>   spice_usb_device_manager_is_device_connected;
> +spice_usb_device_matches_interface;
>   spice_usb_device_widget_get_type;
>   spice_usb_device_widget_new;
>   spice_usbredir_channel_get_type;
> diff --git a/gtk/spice-glib-sym-file b/gtk/spice-glib-sym-file
> index 2189fa5..83f7f18 100644
> --- a/gtk/spice-glib-sym-file
> +++ b/gtk/spice-glib-sym-file
> @@ -101,6 +101,7 @@ spice_usb_device_manager_get_devices
>   spice_usb_device_manager_get_devices_with_filter
>   spice_usb_device_manager_get_type
>   spice_usb_device_manager_is_device_connected
> +spice_usb_device_matches_interface
>   spice_usbredir_channel_get_type
>   spice_util_get_debug
>   spice_util_get_version_string
> diff --git a/gtk/usb-device-manager.c b/gtk/usb-device-manager.c
> index 5013b6c..6749b0d 100644
> --- a/gtk/usb-device-manager.c
> +++ b/gtk/usb-device-manager.c
> @@ -706,6 +706,30 @@ static gboolean spice_usb_device_manager_get_device_descriptor(
>       return TRUE;
>   }
>   
> +static gboolean spice_usb_device_manager_get_config_descriptor(
> +    libusb_device *libdev,
> +    struct libusb_config_descriptor **config)
> +{
> +    int errcode;
> +    const gchar *errstr;
> +
> +    g_return_val_if_fail(libdev != NULL, FALSE);
> +    g_return_val_if_fail(config != NULL, FALSE);
> +
> +    errcode = libusb_get_active_config_descriptor(libdev, config);

I think you should first look at the device descriptor for that information
and only if it's specified there that the information is to be found
in the interface, look at the config_descriptor. (But I'm not
sure, maybe the config_descriptor always contains that
information)

> +    if (errcode < 0) {
> +        int bus, addr;
> +
> +        bus = libusb_get_bus_number(libdev);
> +        addr = libusb_get_device_address(libdev);
> +        errstr = spice_usbutil_libusb_strerror(errcode);
> +        g_warning("Cannot get config descriptor for (%p) %d.%d -- %s(%d)",
> +                  libdev, bus, addr, errstr, errcode);
> +        return FALSE;
> +    }
> +    return TRUE;
> +}
> +
>   static gboolean spice_usb_device_manager_get_libdev_vid_pid(
>       libusb_device *libdev, int *vid, int *pid)
>   {
> @@ -1726,7 +1750,79 @@ gchar *spice_usb_device_get_description(SpiceUsbDevice *device, const gchar *for
>   #endif
>   }
>   
> +static guint8 spice_usb_device_get_interface_class(libusb_device *libdev)
> +{
> +    struct libusb_config_descriptor *config;
> +    guint8 interface_class;
> +
> +    g_return_val_if_fail(libdev != NULL, 0);
> +
> +    config = g_malloc(sizeof(*config));

You do not need to allocate memory here.
libusb_get_active_config_descriptor allocates memory for you (if 
successful).
So, this memory is leaked.
Same below.

> +    spice_usb_device_manager_get_config_descriptor(libdev, &config);
> +    interface_class = config->interface->altsetting->bInterfaceClass;
> +    libusb_free_config_descriptor(config);
> +
> +    return interface_class;
> +}
> +
> +static guint8 spice_usb_device_get_interface_subclass(libusb_device *libdev)
> +{
> +    struct libusb_config_descriptor *config;
> +    guint8 interface_subclass;
> +
> +    g_return_val_if_fail(libdev != NULL, 0);
> +
> +    config = g_malloc(sizeof(*config));
> +    spice_usb_device_manager_get_config_descriptor(libdev, &config);
> +    interface_subclass = config->interface->altsetting->bInterfaceSubClass;
> +    libusb_free_config_descriptor(config);
> +
> +    return interface_subclass;
> +}
> +
> +static guint8 spice_usb_device_get_interface_protocol(libusb_device *libdev)
> +{
> +    struct libusb_config_descriptor *config;
> +    guint8 interface_protocol;
> +
> +    g_return_val_if_fail(libdev != NULL, 0);
> +
> +    config = g_malloc(sizeof(*config));
> +    spice_usb_device_manager_get_config_descriptor(libdev, &config);
> +    interface_protocol = config->interface->altsetting->bInterfaceProtocol;
> +    libusb_free_config_descriptor(config);
> +
> +    return interface_protocol;
> +}
> +
> +/**
> + * spice_usb_device_matches_interface:
> + * @device: #SpiceUsbDevice to check if the interface info matches with
> + * @class: the interface class to be checked
> + * @subclass: the interface usbclass to be checked
s/usbclass/subclass/
> + * @protocol; the interface protocol to be checked
> + *
> + * Compares if the device's interface class, subclass and protocol match with
> + * the received interface class, subclass and protocol.
> + *
> + * Returns: TRUE if the device's interface matches with the receveid one.
> + *          FALSE, otherwise.
> + *
> + * Since: 0.27
> + **/
> +gboolean spice_usb_device_matches_interface(const SpiceUsbDevice *device, guint8 class, guint8 subclass, guint8 protocol)
> +{
> +    const SpiceUsbDeviceInfo *info = (const SpiceUsbDeviceInfo *)device;
> +
> +    g_return_val_if_fail(info != NULL, FALSE);

Note that on Windows, currently, we do not keep info->libdev, as
it may change upon device installation.

Regards,
     Uri.
> +
> +    if ((spice_usb_device_get_interface_class(info->libdev) == class) &&
> +        (spice_usb_device_get_interface_subclass(info->libdev) == subclass) &&
> +        (spice_usb_device_get_interface_protocol(info->libdev) == protocol))
> +        return TRUE;
>   
> +    return FALSE;
> +}
>   
>   #ifdef USE_USBREDIR
>   /*
> diff --git a/gtk/usb-device-manager.h b/gtk/usb-device-manager.h
> index a7e3515..9959fe5 100644
> --- a/gtk/usb-device-manager.h
> +++ b/gtk/usb-device-manager.h
> @@ -89,6 +89,7 @@ GType spice_usb_device_get_type(void);
>   GType spice_usb_device_manager_get_type(void);
>   
>   gchar *spice_usb_device_get_description(SpiceUsbDevice *device, const gchar *format);
> +gboolean spice_usb_device_matches_interface(const SpiceUsbDevice *device, guint8 class, guint8 subclass, guint8 protocol);
>   
>   SpiceUsbDeviceManager *spice_usb_device_manager_get(SpiceSession *session,
>                                                       GError **err);