Re: [PATCH v6 09/10] win-usbredir: Do not use UsbClerk for non-WinUsb backends

[Dmitry Fleytman <dmitry@xxxxxxxxxx>]

On 5 Feb 2016, at 18:31 PM, Jonathon Jongsma <jjongsma@xxxxxxxxxx> wrote:

On Thu, 2015-10-29 at 17:26 +0200, Dmitry Fleytman wrote:

Signed-off-by: Dmitry Fleytman <dmitry@xxxxxxxxxx>
---
src/usb-device-manager.c | 50 ++++++++++++++++++++++++++++++-----------------
-
1 file changed, 31 insertions(+), 19 deletions(-)

diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
index d5fec8f..53820b4 100644
--- a/src/usb-device-manager.c
+++ b/src/usb-device-manager.c
@@ -230,7 +230,7 @@ static void
spice_usb_device_manager_init(SpiceUsbDeviceManager *self)
    priv = SPICE_USB_DEVICE_MANAGER_GET_PRIVATE(self);
    self->priv = priv;

-#ifdef G_OS_WIN32
+#if defined(G_OS_WIN32) && defined(USE_USBREDIR)
    priv->use_usbclerk = TRUE;
#endif

This particular change seems a bit unrelated to this commit. It probably should
be moved to the patch where priv->use_usbclerk was introduced, no?

Moved, thanks.

    priv->channels = g_ptr_array_new();
@@ -255,10 +255,12 @@ static gboolean
spice_usb_device_manager_initable_init(GInitable  *initable,
#endif

#ifdef G_OS_WIN32
-    priv->installer = spice_win_usb_driver_new(err);
-    if (!priv->installer) {
-        SPICE_DEBUG("failed to initialize winusb driver");
-        return FALSE;
+    if (priv->use_usbclerk) {
+        priv->installer = spice_win_usb_driver_new(err);
+        if (!priv->installer) {
+            SPICE_DEBUG("failed to initialize winusb driver");
+            return FALSE;
+        }
    }
#endif

@@ -365,15 +367,17 @@ static void spice_usb_device_manager_finalize(GObject
*gobject)
    free(priv->auto_conn_filter_rules);
    free(priv->redirect_on_connect_rules);
#ifdef G_OS_WIN32
-    if (priv->installer)
+    if (priv->installer) {
+        g_warn_if_fail(priv->use_usbclerk);
        g_object_unref(priv->installer);
+    }
#endif
#endif

    g_free(priv->auto_connect_filter);
    g_free(priv->redirect_on_connect);

-#ifdef G_OS_WIN32
+#if defined(G_OS_WIN32) && defined(USE_USBREDIR)

As I mentioned in the review for patch 8, this change shouldn be done earlier in
an earlier patch. it's not directly related to this patch.

Moved, thanks.

    if (!priv->use_usbclerk) {
        if(priv->auto_connect)
            _usbdk_autoredir_disable(self);
@@ -430,7 +434,7 @@ static void spice_usb_device_manager_set_property(GObject 
     *gobject,
        break;
    case PROP_AUTO_CONNECT:
        priv->auto_connect = g_value_get_boolean(value);
-#ifdef G_OS_WIN32
+#if defined(G_OS_WIN32) && defined(USE_USBREDIR)

Same as above. Consider moving to an earlier patch if the change is needed.

Moved, thanks.

        if (!priv->use_usbclerk) {
            if (priv->auto_connect) {
                _usbdk_autoredir_enable(self);
@@ -935,12 +939,14 @@ static void
spice_usb_device_manager_remove_dev(SpiceUsbDeviceManager *self,
    }

#ifdef G_OS_WIN32
-    const guint8 state = spice_usb_device_get_state(device);
-    if ((state == SPICE_USB_DEVICE_STATE_INSTALLING) ||
-        (state == SPICE_USB_DEVICE_STATE_UNINSTALLING)) {
-        SPICE_DEBUG("skipping " DEV_ID_FMT ". It is un/installing its
driver",
-                    bus, address);
-        return;
+    if (priv->use_usbclerk) {
+        const guint8 state = spice_usb_device_get_state(device);
+        if ((state == SPICE_USB_DEVICE_STATE_INSTALLING) ||
+            (state == SPICE_USB_DEVICE_STATE_UNINSTALLING)) {
+            SPICE_DEBUG("skipping " DEV_ID_FMT ". It is un/installing its
driver",
+                        bus, address);
+            return;
+        }
    }
#endif

@@ -1141,6 +1147,7 @@ static void
spice_usb_device_manager_drv_install_cb(GObject *gobject,
    g_free(cbinfo);

    g_return_if_fail(SPICE_IS_USB_DEVICE_MANAGER(self));
+    g_return_if_fail(self->priv->use_usbclerk);
    g_return_if_fail(SPICE_IS_WIN_USB_DRIVER(installer));
    g_return_if_fail(device!= NULL);

@@ -1173,6 +1180,7 @@ static void
spice_usb_device_manager_drv_uninstall_cb(GObject *gobject,

    SPICE_DEBUG("Win USB driver uninstall finished");
    g_return_if_fail(SPICE_IS_USB_DEVICE_MANAGER(self));
+    g_return_if_fail(self->priv->use_usbclerk);

Minor issue since these statements are only executed on a programming error, but
returning early here will leak cbinfo.

Right. This was broken before this patch also.

I’m adding another patch on top of this series that addresses this issue.

    if (!spice_win_usb_driver_uninstall_finish(cbinfo->installer, res, &err))
{
        g_warning("win usb driver uninstall failed -- %s", err->message);
@@ -1576,15 +1584,18 @@ void
spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
{

#if defined(USE_USBREDIR) && defined(G_OS_WIN32)
-    _spice_usb_device_manager_install_driver_async(self, device, cancellable,
-                                                   callback, user_data);
-#else
+    if (self->priv->use_usbclerk) {
+        _spice_usb_device_manager_install_driver_async(self, device,
cancellable,
+                                                       callback, user_data);
+        return;
+    }
+#endif
+
    _spice_usb_device_manager_connect_device_async(self,
                                                   device,
                                                   cancellable,
                                                   callback,
                                                   user_data);
-#endif
}

/**
@@ -1637,7 +1648,8 @@ void
spice_usb_device_manager_disconnect_device(SpiceUsbDeviceManager *self,
        spice_usbredir_channel_disconnect_device(channel);

#ifdef G_OS_WIN32
-    _spice_usb_device_manager_uninstall_driver_async(self, device);
+    if(self->priv->use_usbclerk)
+        _spice_usb_device_manager_uninstall_driver_async(self, device);
#endif

#endif

Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>

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