[Spice-devel] [spice-gtk] [PATCH] Provide a method to check if a USB device matches a specific class, subclass and protocol
Uri Lublin
uril at redhat.com
Wed Nov 12 09:58:57 PST 2014
On 11/12/2014 07:22 PM, Fabiano Fidêncio wrote:
> Hi Uri,
>
> On Wed, Nov 12, 2014 at 6:01 PM, Uri Lublin <uril at redhat.com> wrote:
>> 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
> Yeah, but still hard to filter for all keyboard devices, for instance.
The default can be to block all HID devices.
If a user wants to usbredir a specific device, e.g. a Joystick,
s/he can find out its VID/PID (using lsusb) and add a rule
to the usb-filter accordingly.
I agree it's not as convenient as filtering by subclass/protocol.
Another comment with regards to your patch is:
config->interface may hold an array of (bNumInterfaces) interfaces
and you are only looking at the first one.
Same for config->interface->altsetting
As for the Windows comment, with the patch applied windows
build is going to fail, isn't it ?
Regards,
Uri.
>
>>> ---
>>> 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)
> Hmmm. You're right. When bDeviceClass is 0, means that I should check
> in the Interface level, as shown in the lsusb -v
>
> Bus 003 Device 013: ID 05ac:0220 Apple, Inc. Aluminum Keyboard (ANSI)
> Couldn't open device, some information will be missing
> Device Descriptor:
> bLength 18
> bDescriptorType 1
> bcdUSB 2.00
> bDeviceClass 0 (Defined at Interface level)
> bDeviceSubClass 0
> bDeviceProtocol 0
> bMaxPacketSize0 8
> idVendor 0x05ac Apple, Inc.
> idProduct 0x0220 Aluminum Keyboard (ANSI)
> bcdDevice 0.69
> iManufacturer 1
> iProduct 2
> iSerial 0
> bNumConfigurations 1
> Configuration Descriptor:
> bLength 9
> bDescriptorType 2
> wTotalLength 59
> bNumInterfaces 2
> bConfigurationValue 1
> iConfiguration 0
> bmAttributes 0xa0
> (Bus Powered)
> Remote Wakeup
> MaxPower 20mA
> Interface Descriptor:
> bLength 9
> bDescriptorType 4
> bInterfaceNumber 0
> bAlternateSetting 0
> bNumEndpoints 1
> bInterfaceClass 3 Human Interface Device
> bInterfaceSubClass 1 Boot Interface Subclass
> bInterfaceProtocol 1 Keyboard
> iInterface 0
> HID Device Descriptor:
> bLength 9
> bDescriptorType 33
> bcdHID 1.11
> bCountryCode 33 US
> bNumDescriptors 1
> bDescriptorType 34 Report
> wDescriptorLength 75
> Report Descriptors:
> ** UNAVAILABLE **
> Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x81 EP 1 IN
> bmAttributes 3
> Transfer Type Interrupt
> Synch Type None
> Usage Type Data
> wMaxPacketSize 0x0008 1x 8 bytes
> bInterval 10
> Interface Descriptor:
> bLength 9
> bDescriptorType 4
> bInterfaceNumber 1
> bAlternateSetting 0
> bNumEndpoints 1
> bInterfaceClass 3 Human Interface Device
> bInterfaceSubClass 0 No Subclass
> bInterfaceProtocol 0 None
> iInterface 0
> HID Device Descriptor:
> bLength 9
> bDescriptorType 33
> bcdHID 1.11
> bCountryCode 0 Not supported
> bNumDescriptors 1
> bDescriptorType 34 Report
> wDescriptorLength 47
> Report Descriptors:
> ** UNAVAILABLE **
> Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x82 EP 2 IN
> bmAttributes 3
> Transfer Type Interrupt
> Synch Type None
> Usage Type Data
> wMaxPacketSize 0x0001 1x 1 bytes
> bInterval 10
>
>>
>>> + 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.
> Ooops, thanks!
>
>>
>>> + 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.
>>
> Yeah, unfortunately Windows is not going to take advantage of these bits.
>
>> 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);
>>
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
> Best Regards,
More information about the Spice-devel
mailing list