Re: [PATCH v3 05/13] UsbDeviceManager and redirect channel: spawn a different thread for connect device flow

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> On Aug 11, 2015, at 14:22 PM, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote:
> 
> The commit log should explain _why_ this is needed ("on Windows when
> using usbdk, calling libusb_xxxx can block for N seconds. libusb_xxxx is
> called by usbredir_yyyy, which is called from the main thread. If it
> blocks, this will cause the UI to freeze. This commit makes sure
> usbredir_yyyy is called from a different thread to avoid blocking the
> mainloop when usbdk is used.")
> 
> It should also explain why it's safe to call this from a different
> thread.

Added explanations in v4.

> 
> Christophe
> 
> On Mon, Aug 03, 2015 at 04:10:45PM +0300, Kirill Moizik wrote:
>> From: Kirill Moizik <kmoizik@xxxxxxxxxx>
>> 
>> ---
>> src/channel-usbredir.c   | 46 +++++++++++++++++++++++---------
>> src/usb-device-manager.c | 69 +++++++++++++++++++++++++++++++++++++++---------
>> 2 files changed, 90 insertions(+), 25 deletions(-)
>> 
>> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
>> index 69cb565..7394b81 100644
>> --- a/src/channel-usbredir.c
>> +++ b/src/channel-usbredir.c
>> @@ -315,6 +315,27 @@ static void spice_usbredir_channel_open_acl_cb(
>> }
>> #endif
>> 
>> +#ifndef USE_POLKIT
>> +static void
>> +spice_usbredir_channel_open_device_async(GSimpleAsyncResult *simple,
>> +                                         GObject *object,
>> +                                         GCancellable *cancellable)
>> +{
>> +    GError *err = NULL;
>> +    SpiceUsbredirChannel *channel= (SpiceUsbredirChannel *)object;
>> +    SpiceUsbredirChannelPrivate *priv = channel->priv;
>> +    g_mutex_lock(priv->flows_mutex);
>> +    if (!spice_usbredir_channel_open_device(channel, &err)) {
>> +        g_simple_async_result_take_error(simple, err);
>> +        libusb_unref_device(priv->device);
>> +        priv->device = NULL;
>> +        g_boxed_free(spice_usb_device_get_type(), priv->spice_device);
>> +        priv->spice_device = NULL;
>> +     }
>> +    g_mutex_unlock(priv->flows_mutex);
>> +}
>> +#endif
>> +
>> G_GNUC_INTERNAL
>> void spice_usbredir_channel_connect_device_async(
>>                                           SpiceUsbredirChannel *channel,
>> @@ -324,15 +345,16 @@ void spice_usbredir_channel_connect_device_async(
>>                                           GAsyncReadyCallback   callback,
>>                                           gpointer              user_data)
>> {
>> -    SpiceUsbredirChannelPrivate *priv = channel->priv;
>> -    GSimpleAsyncResult *result;
>> -#if ! USE_POLKIT
>> -    GError *err = NULL;
>> -#endif
>> 
>>     g_return_if_fail(SPICE_IS_USBREDIR_CHANNEL(channel));
>>     g_return_if_fail(device != NULL);
>> 
>> +    SpiceUsbredirChannelPrivate *priv = channel->priv;
>> +    GSimpleAsyncResult *result;
>> +#ifndef USE_POLKIT
>> +    g_object_ref(channel);
>> +#endif
>> +
>>     CHANNEL_DEBUG(channel, "connecting usb channel %p", channel);
>> 
>>     result = g_simple_async_result_new(G_OBJECT(channel), callback, user_data,
>> @@ -369,18 +391,18 @@ void spice_usbredir_channel_connect_device_async(
>>                                   channel);
>>     return;
>> #else
>> -    if (!spice_usbredir_channel_open_device(channel, &err)) {
>> -        g_simple_async_result_take_error(result, err);
>> -        libusb_unref_device(priv->device);
>> -        priv->device = NULL;
>> -        g_boxed_free(spice_usb_device_get_type(), priv->spice_device);
>> -        priv->spice_device = NULL;
>> -    }
>> +    g_simple_async_result_run_in_thread(result,
>> +                                        spice_usbredir_channel_open_device_async,
>> +                                        G_PRIORITY_DEFAULT,
>> +                                        cancellable);
>> +    g_object_unref(result);
>> +    return;
>> #endif
>> 
>> done:
>>     g_simple_async_result_complete_in_idle(result);
>>     g_object_unref(result);
>> +    g_object_unref(channel);
>> }
>> 
>> G_GNUC_INTERNAL
>> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
>> index e58450d..65c6568 100644
>> --- a/src/usb-device-manager.c
>> +++ b/src/usb-device-manager.c
>> @@ -207,6 +207,11 @@ _spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
>>                                                GAsyncReadyCallback callback,
>>                                                gpointer user_data);
>> 
>> +static
>> +void spice_usb_device_manager_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)
>> @@ -1128,8 +1133,16 @@ static void spice_usb_device_manager_channel_connect_cb(
>>     }
>>     g_simple_async_result_complete(result);
>>     g_object_unref(result);
>> +    g_object_unref(channel);
>> }
>> 
>> +typedef struct _connect_cb_data
>> +{
>> +    SpiceUsbDeviceManager  *self;
>> +    GAsyncReadyCallback     callback;
>> +    gpointer                user_data;
>> +} connect_cb_data;
>> +
>> #ifdef G_OS_WIN32
>> 
>> typedef struct _UsbInstallCbInfo {
>> @@ -1165,7 +1178,7 @@ static void spice_usb_device_manager_drv_install_cb(GObject *gobject,
>>     SpiceUsbDevice *device;
>>     UsbInstallCbInfo *cbinfo;
>>     GCancellable *cancellable;
>> -    GAsyncReadyCallback callback;
>> +    connect_cb_data *data;
>> 
>>     g_return_if_fail(user_data != NULL);
>> 
>> @@ -1174,8 +1187,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);
>> 
>> @@ -1197,8 +1209,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);
>> +                                                   spice_usb_device_manager_connect_device_async_cb,
>> +                                                   data);
>> 
>>     spice_usb_device_unref(device);
>> }
>> @@ -1485,6 +1497,7 @@ gboolean spice_usb_device_manager_is_device_connected(SpiceUsbDeviceManager *sel
>>     return !!spice_usb_device_manager_get_channel_for_dev(self, device);
>> }
>> 
>> +#ifdef USE_USBREDIR
>> /**
>>  * spice_usb_device_manager_connect_device_async:
>>  * @manager: the #SpiceUsbDeviceManager manager
>> @@ -1510,7 +1523,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;
>> @@ -1556,18 +1568,16 @@ _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
>> 
>> void spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
>>                                              SpiceUsbDevice *device,
>> @@ -1575,8 +1585,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));
>> +#ifdef USE_USBREDIR
>> +    g_return_if_fail(self->priv->redirecting == FALSE);
>> +    g_object_set(self,"redirecting", TRUE, NULL);
>> +#ifndef USE_LIBUSB_HOTPLUG
>> +    spice_g_udev_set_redirecting(TRUE);
>> +#endif
>> +    connect_cb_data *data = g_new(connect_cb_data,1);
>> +    data->self = self;
>> +    data->callback  = callback;
>> +    data->user_data = user_data;
>> +
>>     if (self->priv->use_usbclerk) {
>> -#if defined(USE_USBREDIR) && defined(G_OS_WIN32)
>> +#ifdef G_OS_WIN32
>>         SpiceWinUsbDriver *installer;
>>         UsbInstallCbInfo *cbinfo;
>> 
>> @@ -1591,7 +1613,7 @@ void spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
>>         cbinfo->installer   = installer;
>>         cbinfo->cancellable = cancellable;
>>         cbinfo->callback    = callback;
>> -        cbinfo->user_data   = user_data;
>> +        cbinfo->user_data   = data;
>> 
>>         spice_win_usb_driver_install_async(installer, device, cancellable,
>>                                            spice_usb_device_manager_drv_install_cb,
>> @@ -1603,9 +1625,10 @@ void spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
>>         _spice_usb_device_manager_connect_device_async(self,
>>                                                        device,
>>                                                        cancellable,
>> -                                                       callback,
>> -                                                       user_data);
>> +                                                       spice_usb_device_manager_connect_device_async_cb,
>> +                                                       data);
>>     }
>> +#endif
>> }
>> 
>> gboolean spice_usb_device_manager_connect_device_finish(
>> @@ -1623,6 +1646,26 @@ gboolean spice_usb_device_manager_connect_device_finish(
>>     return TRUE;
>> }
>> 
>> +#ifdef USE_USBREDIR
>> +static
>> +void spice_usb_device_manager_connect_device_async_cb(GObject *gobject,
>> +                                                      GAsyncResult *channel_res,
>> +                                                      gpointer user_data)
>> +{
>> +    connect_cb_data    *data    = user_data;
>> +    GSimpleAsyncResult *result  = g_simple_async_result_new(G_OBJECT(data->self), data->callback,
>> +                                  data->user_data,
>> +                                  spice_usb_device_manager_disconnect_device_async);
>> +    g_object_set(data->self,"redirecting", FALSE, NULL);
>> +#ifndef USE_LIBUSB_HOTPLUG
>> +    spice_g_udev_set_redirecting(FALSE);
>> +#endif
>> +    g_simple_async_result_complete(result);
>> +    g_object_unref(result);
>> +    g_free(data);
>> +}
>> +#endif
>> +
>> /**
>>  * spice_usb_device_manager_disconnect_device:
>>  * @manager: the #SpiceUsbDeviceManager manager
>> -- 
>> 2.4.3
>> 
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/spice-devel

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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]