[Spice-devel] [PATCH v6 08/10] usbdk: Load hide rules for auto-redirected devices

[Dmitry Fleytman]

> On 5 Feb 2016, at 24:40 AM, Jonathon Jongsma <jjongsma at redhat.com> wrote:
> 
> On Thu, 2015-10-29 at 17:26 +0200, Dmitry Fleytman wrote:
>> Hide rules order UsbDk to avoid showing specific USB
>> devices to Windows PnP manager.
>> 
>> Spice-gtk loads hide rules for devices that should be
>> automatically redirected on connection to prevent Windows
>> from showing "New Hardware Found" wizard window for USB
>> devices that do not have driver on the local system.
>> 
>> Signed-off-by: Dmitry Fleytman <dmitry at daynix.com>
>> ---
>> src/usb-device-manager.c | 58
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 58 insertions(+)
>> 
>> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
>> index dd55276..d5fec8f 100644
>> --- a/src/usb-device-manager.c
>> +++ b/src/usb-device-manager.c
>> @@ -29,6 +29,10 @@
>> #include <errno.h>
>> #include <libusb.h>
>> 
>> +#ifdef G_OS_WIN32
>> +#include "usbdk_api.h"
>> +#endif
>> +
>> #if defined(USE_GUDEV)
>> #include <gudev/gudev.h>
>> #elif defined(G_OS_WIN32)
>> @@ -121,6 +125,8 @@ struct _SpiceUsbDeviceManagerPrivate {
>>     libusb_hotplug_callback_handle hp_handle;
>> #endif
>> #ifdef G_OS_WIN32
>> +    usbdk_api_wrapper     *usbdk_api;
>> +    HANDLE                 usbdk_hider_handle;
>>     SpiceWinUsbDriver     *installer;
>> #endif
>>     gboolean               use_usbclerk;
>> @@ -183,6 +189,9 @@ static void spice_usb_device_unref(SpiceUsbDevice
>> *device);
>> #ifdef G_OS_WIN32
>> static guint8 spice_usb_device_get_state(SpiceUsbDevice *device);
>> static void  spice_usb_device_set_state(SpiceUsbDevice *device, guint8 s);
>> +
>> +static void _usbdk_autoredir_enable(SpiceUsbDeviceManager *manager);
>> +static void _usbdk_autoredir_disable(SpiceUsbDeviceManager *manager);
>> #endif
>> 
>> static gboolean spice_usb_manager_device_equal_libdev(SpiceUsbDeviceManager
>> *manager,
>> @@ -364,6 +373,12 @@ static void spice_usb_device_manager_finalize(GObject
>> *gobject)
>>     g_free(priv->auto_connect_filter);
>>     g_free(priv->redirect_on_connect);
>> 
>> +#ifdef G_OS_WIN32
>> +    if (!priv->use_usbclerk) {
>> +        if(priv->auto_connect)
>> +            _usbdk_autoredir_disable(self);
>> +    }
>> +#endif
>>     /* Chain up to the parent class */
>>     if (G_OBJECT_CLASS(spice_usb_device_manager_parent_class)->finalize)
>>         G_OBJECT_CLASS(spice_usb_device_manager_parent_class)
>> ->finalize(gobject);
>> @@ -415,6 +430,15 @@ static void spice_usb_device_manager_set_property(GObject
>>       *gobject,
>>         break;
>>     case PROP_AUTO_CONNECT:
>>         priv->auto_connect = g_value_get_boolean(value);
> 
> It does not appear that PROP_AUTO_CONNECT is a construct-only property, so this
> property could theoretically be called multiple times. From looking at the
> _usb_autoredir_(en|dis)able() functions below, it doesn't look like a good idea
> to call them multiple times. In addition, it appears racy since
> _usbdk_autoredir_enable() depends on redirect_on_connect. If that property is
> not set before this one, it will return without doing anything. To avoid the
> race, these calls should probably go into a 'constructed' vfunc.

This was broken indeed. I redesigned this part and fixed issues you’re mentioning here, and now it
is safe to call these functions multiple times the way it should be.

I didn’t put those calls to a ‘constructed’ vfunc because the way UsbDk is working one should
hold hider interface handle when it has hide rules only, therefore this handle should be opened and closed
a few times during device manager life time.

> 
>> +#ifdef G_OS_WIN32
>> +        if (!priv->use_usbclerk) {
>> +            if (priv->auto_connect) {
>> +                _usbdk_autoredir_enable(self);
>> +            } else {
>> +                _usbdk_autoredir_disable(self);
>> +            }
>> +        }
>> +#endif
>>         break;
>>     case PROP_AUTO_CONNECT_FILTER: {
>>         const gchar *filter = g_value_get_string(value);
>> @@ -1856,6 +1880,40 @@ guint8 spice_usb_device_get_state(SpiceUsbDevice
>> *device)
>> 
>>     return info->state;
>> }
>> +static
>> +void _usbdk_autoredir_enable(SpiceUsbDeviceManager *manager)
>> +{
>> +    SpiceUsbDeviceManagerPrivate *priv = manager->priv;
>> +    g_return_if_fail(!priv->use_usbclerk);
>> +
>> +    if (priv->redirect_on_connect == NULL) {
> 
> as I said above, if the 'redirect-on-connect' property is not set before 'auto
> -connect', this function will return without doing anythign and print a debug
> statement.

Fixed.

> 
>> +        SPICE_DEBUG("No autoredirect rules, no hider setup needed");
>> +        return;
>> +    }
>> +
>> +    priv->usbdk_hider_handle = usbdk_create_hider_handle(priv->usbdk_api);
> 
> Also, if this function is called multiple times, I think we'll leak the handle
> here.


Fixed.

> 
>> +    if (priv->usbdk_hider_handle == NULL) {
>> +        g_warning("Failed to instantiate UsbDk hider interface");
>> +        return;
>> +    }
>> +
>> +    usbdk_api_set_hide_rules(priv->usbdk_api,
>> +                             priv->usbdk_hider_handle,
>> +                             priv->redirect_on_connect);
>> +}
>> +
>> +static
>> +void _usbdk_autoredir_disable(SpiceUsbDeviceManager *manager)
>> +{
>> +    SpiceUsbDeviceManagerPrivate *priv = manager->priv;
>> +    g_return_if_fail(!priv->use_usbclerk);
>> +
>> +    if (priv->usbdk_hider_handle != NULL) {
>> +        usbdk_clear_hide_rules(priv->usbdk_api, priv->usbdk_hider_handle);
>> +        usbdk_close_hider_handle(priv->usbdk_api, priv->usbdk_hider_handle);
>> +        priv->usbdk_hider_handle = NULL;
>> +    }
>> +}
>> #endif
>> 
>> static SpiceUsbDevice *spice_usb_device_ref(SpiceUsbDevice *device)
> 
> 
> Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com <mailto:jjongsma at redhat.com>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160211/b7a1dc26/attachment-0001.html>