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

[Dmitry Fleytman]

> On 5 Feb 2016, at 18:31 PM, Jonathon Jongsma <jjongsma at redhat.com> wrote:
> 
> On Thu, 2015-10-29 at 17:26 +0200, Dmitry Fleytman wrote:
>> Signed-off-by: Dmitry Fleytman <dmitry at daynix.com>
>> ---
>> 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 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/0743b79d/attachment-0001.html>