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

[Dmitry Fleytman <dmitry@xxxxxxxxxx>]

On 5 Feb 2016, at 24:40 AM, Jonathon Jongsma <jjongsma@xxxxxxxxxx> 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@xxxxxxxxxx>
---
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@xxxxxxxxxx>

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel