[Spice-devel] [PATCH v3 05/13] UsbDeviceManager and redirect channel: spawn a different thread for connect device flow
Dmitry Fleytman
dmitry at daynix.com
Sun Aug 16 04:42:33 PDT 2015
> On Aug 11, 2015, at 15:14 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
>
> On Mon, Aug 03, 2015 at 04:10:45PM +0300, Kirill Moizik wrote:
>> From: Kirill Moizik <kmoizik at redhat.com>
>>
>> ---
>> 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)
>
> This should be named _open_device_async_cb()
Fixed.
>
>> +{
>> + GError *err = NULL;
>> + SpiceUsbredirChannel *channel= (SpiceUsbredirChannel *)object;
>
> SPICE_USBREDIR_CHANNEL(object) would be better than the direct cast.
Sure. Fixed.
>
>> + 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
>
> I don't think you need this _ref() here,
> g_simple_async_result_new(channel, ...) will take a reference for you
> for as long as the GSimpleAsyncResult object is alive.
You are right, removed here and in a similar code on disconnection.
>
>> +
>> 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
>
> All the changes below in usb-device-manager.c seems to be more or less
> unrelated to the changes above, and aimed at setting 'redirecting' to
> TRUE/FALSE when redirection starts/ends, I suspect it would be clearer
> to do this in a separate commit.
Agree, moving to a separate commit.
>
>> 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 {
> [snip]
>> @@ -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
>> +
>
> Can't this be done in spice_usb_device_manager_channel_connect_cb()
> rather than introducing an extra async_cb step?
That would be better, but unfortunately spice_usb_device_manager_channel_connect_cb
doesn’t have access to device manager/udev objects and we’d like to preserve that type of encapsulation.
>
> Christophe
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150816/b5f49316/attachment-0001.html>
More information about the Spice-devel
mailing list