[Spice-devel] [PATCH v5 06/13] UsbDeviceManager: Track device redirection operations in progress

[Jonathon Jongsma]

On Thu, 2015-10-29 at 17:27 +0200, Dmitry Fleytman wrote:
> From: Kirill Moizik <kmoizik at redhat.com>
> 
> During device connection, unwanted hotplug events may happen.
> We need to ignore those therefore we track redirection operations
> in progress.
> 
> See also comment to commit
> "Do not process USB hotplug events while redirection is in progress"
> that introduces corresponding filtering out logic.
> 
> Signed-off-by: Kirill Moizik <kmoizik at redhat.com>
> Signed-off-by: Dmitry Fleytman <dfleytma at redhat.com>
> ---
>  src/usb-device-manager.c | 69 +++++++++++++++++++++++++++++++++++++++--------
> -
>  1 file changed, 56 insertions(+), 13 deletions(-)
> 
> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> index 4d376b6..0626923 100644
> --- a/src/usb-device-manager.c
> +++ b/src/usb-device-manager.c
> @@ -122,6 +122,7 @@ struct _SpiceUsbDeviceManagerPrivate {
>      GUdevClient *udev;
>      libusb_device **coldplug_list; /* Avoid needless reprobing during init */
>  #else
> +    gboolean redirecting;

I'd prefer a comment here indicating that in the gudev case, the 'redirecting'
status is handled by the GUdevClient.

>      libusb_hotplug_callback_handle hp_handle;
>  #endif
>  #ifdef G_OS_WIN32
> @@ -208,10 +209,25 @@
> _spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
>                                                 GAsyncReadyCallback callback,
>                                                 gpointer user_data);
>  
> +static
> +void _connect_device_async_cb(GObject *gobject,
> +                              GAsyncResult *channel_res,
> +                              gpointer user_data);
> +
>  G_DEFINE_BOXED_TYPE(SpiceUsbDevice, spice_usb_device,
>                      (GBoxedCopyFunc)spice_usb_device_ref,
>                      (GBoxedFreeFunc)spice_usb_device_unref)
>  
> +static void
> +_set_redirecting(SpiceUsbDeviceManager *self, gboolean is_redirecting)
> +{
> +#ifdef USE_GUDEV
> +    g_object_set(self->priv->udev, "redirecting", is_redirecting, NULL);
> +#else
> +    self->priv->redirecting = is_redirecting;
> +#endif
> +}
> +
>  #else
>  G_DEFINE_BOXED_TYPE(SpiceUsbDevice, spice_usb_device, g_object_ref,
> g_object_unref)
>  #endif
> @@ -1135,7 +1151,7 @@ static void
> spice_usb_device_manager_drv_install_cb(GObject *gobject,
>      SpiceUsbDevice *device;
>      UsbInstallCbInfo *cbinfo;
>      GCancellable *cancellable;
> -    GAsyncReadyCallback callback;
> +    gpointer data;
>  
>      g_return_if_fail(user_data != NULL);
>  
> @@ -1144,8 +1160,7 @@ static void
> spice_usb_device_manager_drv_install_cb(GObject *gobject,
>      device      = cbinfo->device;
>      installer   = cbinfo->installer;
>      cancellable = cbinfo->cancellable;
> -    callback    = cbinfo->callback;
> -    user_data   = cbinfo->user_data;
> +    data        = cbinfo->user_data;
>  
>      g_free(cbinfo);
>  
> @@ -1167,8 +1182,8 @@ static void
> spice_usb_device_manager_drv_install_cb(GObject *gobject,
>      _spice_usb_device_manager_connect_device_async(self,
>                                                     device,
>                                                     cancellable,
> -                                                   callback,
> -                                                   user_data);
> +                                                   _connect_device_async_cb,
> +                                                   data);
>  
>      spice_usb_device_unref(device);
>  }
> @@ -1496,6 +1511,8 @@
> _spice_usb_device_manager_uninstall_driver_async(SpiceUsbDeviceManager *self,
>  
>  #endif
>  
> +#ifdef USE_USBREDIR
> +

Moving the #ifdef outside of the function here seems like a fine change, but it
doesn't appear to be necessary for this patch and makes the patch slightly
harder to review. Can this change be split?

>  static void
>  _spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
>                                                 SpiceUsbDevice *device,
> @@ -1513,7 +1530,6 @@
> _spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
>      result = g_simple_async_result_new(G_OBJECT(self), callback, user_data,
>                                
>  spice_usb_device_manager_connect_device_async);
>  
> -#ifdef USE_USBREDIR
>      SpiceUsbDeviceManagerPrivate *priv = self->priv;
>      libusb_device *libdev;
>      guint i;
> @@ -1559,18 +1575,17 @@
> _spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
>          libusb_unref_device(libdev);
>          return;
>      }
> -#endif
>  
>      g_simple_async_result_set_error(result,
>                              SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
>                              _("No free USB channel"));
> -#ifdef USE_USBREDIR
>  done:
> -#endif
>      g_simple_async_result_complete_in_idle(result);
>      g_object_unref(result);
>  }
>  
> +#endif
> +
>  /**
>   * spice_usb_device_manager_connect_device_async:
>   * @self: a #SpiceUsbDeviceManager.
> @@ -1589,11 +1604,20 @@ void
> spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
>                                               GAsyncReadyCallback callback,
>                                               gpointer user_data)
>  {
> +    g_return_if_fail(SPICE_IS_USB_DEVICE_MANAGER(self));
>  
> -#if defined(USE_USBREDIR) && defined(G_OS_WIN32)
> +#ifdef USE_USBREDIR
> +
> +    GSimpleAsyncResult *result =
> +        g_simple_async_result_new(G_OBJECT(self), callback, user_data,
> +                                 
>  spice_usb_device_manager_connect_device_async);
> +
> +    _set_redirecting(self, TRUE);
> +
> +#ifdef G_OS_WIN32
>      if (self->priv->use_usbclerk) {
>          _spice_usb_device_manager_install_driver_async(self, device,
> cancellable,
> -                                                       callback, user_data);
> +                                                       callback, result);
>          return;
>      }
>  #endif
> @@ -1601,10 +1625,13 @@ void
> spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
>      _spice_usb_device_manager_connect_device_async(self,
>                                                     device,
>                                                     cancellable,
> -                                                   callback,
> -                                                   user_data);
> +                                                   _connect_device_async_cb,
> +                                                   result);
> +
> +#endif
>  }
>  
> +
>  /**
>   * spice_usb_device_manager_connect_device_finish:
>   * @self: a #SpiceUsbDeviceManager.
> @@ -1630,6 +1657,22 @@ gboolean
> spice_usb_device_manager_connect_device_finish(
>      return TRUE;
>  }
>  
> +#ifdef USE_USBREDIR
> +static
> +void _connect_device_async_cb(GObject *gobject,
> +                              GAsyncResult *channel_res,
> +                              gpointer user_data)
> +{
> +    SpiceUsbDeviceManager *self = SPICE_USB_DEVICE_MANAGER(gobject);
> +    GSimpleAsyncResult *result = user_data;
> +
> +    _set_redirecting(self, FALSE);
> +
> +    g_simple_async_result_complete(result);
> +    g_object_unref(result);
> +}
> +#endif
> +
>  /**
>   * spice_usb_device_manager_disconnect_device:
>   * @manager: the #SpiceUsbDeviceManager manager

ACK with minor changes

Acked-by: Jonathon Jongsma <jjongsma at redhat.com>