[Spice-devel] [PATCH v6 08/10] usbdk: Load hide rules for auto-redirected devices
Dmitry Fleytman
dmitry at daynix.com
Thu Feb 11 14:20:02 UTC 2016
> On 5 Feb 2016, at 18:38 PM, Jonathon Jongsma <jjongsma at redhat.com> wrote:
>
> On Fri, 2016-02-05 at 10:21 -0600, Jonathon Jongsma wrote:
>> On Thu, 2016-02-04 at 16:40 -0600, Jonathon Jongsma 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
>>
>> Another thing:
>>
>> Here you introduce this code within an
>>
>> #ifdef G_OS_WIN32
>>
>> block, but in the next commit, you change this to
>>
>> #if defined(G_OS_WIN32) && defined(USE_USBREDIR)
>>
>> If this change is necessary, it should be done in this commit, not in the next
>> one. But it seems that you could simply move it up a few lines to the block
>> where you unref the priv->installer variable, no? Or is there some reason this
>> has to be done after freeing auto_connect_filter and redirect_on_connect?
>>
>>
>>>> /* 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.
>>>
>>>> +#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.
>>>
>>>> + 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.
>
> ... And now I notice that priv->usbdk_api has not actually been initialized in
> this patch. It's not until patch 10 that usbdk_api_load() is called. So this
> call will segfault unless these patches are re-ordered so that the api is loaded
> before it's used.
This code will not segfault because it protected by if(!use_usbclerk) condition.
This is by design to make history simpler, we introduce everything step by step
protected by if(!use_usbclerk) condition which is always false until the last patch that
sets use_usbclerk = FALSE when UsbDk is available and successfully linked.
>
>
>>>
>>>> + 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>
>>> _______________________________________________
>>> Spice-devel mailing list
>>> Spice-devel at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel at lists.freedesktop.org <mailto:Spice-devel at lists.freedesktop.org>
>> http://lists.freedesktop.org/mailman/listinfo/spice-devel <http://lists.freedesktop.org/mailman/listinfo/spice-devel>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160211/b2e4b028/attachment.html>
More information about the Spice-devel
mailing list