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

[Jonathon Jongsma]

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?


>      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.

>      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.

>          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.

>  
>      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>