[Spice-devel] [PATCH v5 07/13] UsbDeviceManager: Implement asynchronous disconnect device flow
Dmitry Fleytman
dmitry at daynix.com
Sun Feb 28 08:58:13 UTC 2016
> On 11 Feb 2016, at 01:24 AM, Jonathon Jongsma <jjongsma at redhat.com> wrote:
>
> On Wed, 2016-02-10 at 16:36 -0600, Jonathon Jongsma wrote:
>> On Thu, 2015-10-29 at 17:27 +0200, Dmitry Fleytman wrote:
>>
>>> From: Kirill Moizik <kmoizik at redhat.com>
>>>
>>> This commit introduces functions for asynchronous disconnection flows.
>>> Following commits will make use of those.
>>>
>>> Thread safety is ensured the same way it was done for connection
>>> flow in previous commits. Disconnect logic is protected by the same
>>> locks that protect connect/usbredir/channel management logic.
>>>
>>> Signed-off-by: Kirill Moizik <kmoizik at redhat.com>
>>> Signed-off-by: Dmitry Fleytman <dfleytma at redhat.com>
>>> ---
>>> src/channel-usbredir-priv.h | 4 +++
>>> src/channel-usbredir.c | 24 ++++++++++++++++++
>>> src/usb-device-manager.c | 62
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>> src/usb-device-manager.h | 8 ++++++
>>> 4 files changed, 98 insertions(+)
>>>
>>> diff --git a/src/channel-usbredir-priv.h b/src/channel-usbredir-priv.h
>>> index c987474..fc9a108 100644
>>> --- a/src/channel-usbredir-priv.h
>>> +++ b/src/channel-usbredir-priv.h
>>> @@ -33,6 +33,10 @@ G_BEGIN_DECLS
>>> void spice_usbredir_channel_set_context(SpiceUsbredirChannel *channel,
>>> libusb_context *context);
>>>
>>> +void spice_usbredir_channel_disconnect_device_async(SpiceUsbredirChannel
>>> *channel,
>>> + GSimpleAsyncResult
>>> *simple,
>>> + GCancellable
>>> *cancellable);
>>
>>
>> This is a strange function signature for an async method. Generally it should
>> accept a GAsyncReadyCallback rather than a GSimpleAsyncResult. The result
>> should
>> be handled internally and passed to the GAsyncReadyCallback. There should also
>> be a _finish() function that the callback should call. See
>> spice_usbredir_channel_connect_device_async/finish().
Fixed. Thanks.
>>
>> This stuff would also be a bit easier if we used the GTask API (internally)
>> instead of GSimpleAsyncResult. Fabiano has a bunch of patches that already
>> switch to GTask. We should probably rebase this stuff on top of those.
Yes, GTask API should make the code look better. Since these patches are stable and waiting on the list for a lot of time already, I’d prefer to have them accepted with GSimpleAsyncResult and introduce GTask transition later to have a stable point in history. Unless you insist of course...
>>
>>> +
>>> /* Note the context must be set, and the channel must be brought up
>>> (through spice_channel_connect()), before calling this. */
>>> void spice_usbredir_channel_connect_device_async(
>>> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
>>> index 302a2aa..8519957 100644
>>> --- a/src/channel-usbredir.c
>>> +++ b/src/channel-usbredir.c
>>> @@ -430,6 +430,7 @@ void
>>> spice_usbredir_channel_disconnect_device(SpiceUsbredirChannel *channel)
>>>
>>> CHANNEL_DEBUG(channel, "disconnecting device from usb channel %p",
>>> channel);
>>>
>>> + g_mutex_lock(priv->flows_mutex);
>>> switch (priv->state) {
>>> case STATE_DISCONNECTED:
>>> case STATE_DISCONNECTING:
>>> @@ -463,6 +464,29 @@ void
>>> spice_usbredir_channel_disconnect_device(SpiceUsbredirChannel *channel)
>>> priv->state = STATE_DISCONNECTED;
>>> break;
>>> }
>>> + g_mutex_unlock(priv->flows_mutex);
>>> +}
>>> +
>>> +static void
>>> +_disconnect_device_thread(GSimpleAsyncResult *simple,
>>> + GObject *object,
>>> + GCancellable *cancellable)
>>> +{
>>> +
>>> spice_usbredir_channel_disconnect_device(SPICE_USBREDIR_CHANNEL(object));
>>> +}
>>> +
>>> +void spice_usbredir_channel_disconnect_device_async(SpiceUsbredirChannel
>>> *channel,
>>> + GSimpleAsyncResult
>>> *simple,
>>> + GCancellable
>>> *cancellable)
>>> +{
>>> + if (channel) {
>>> + g_simple_async_result_run_in_thread(simple,
>>> + _disconnect_device_thread,
>>> + G_PRIORITY_DEFAULT,
>>> + cancellable);
>>> + } else {
>>> + g_simple_async_result_complete_in_idle(simple);
>>> + }
>>> }
>>>
>>> G_GNUC_INTERNAL
>>> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
>>> index 0626923..51b6c6d 100644
>>> --- a/src/usb-device-manager.c
>>> +++ b/src/usb-device-manager.c
>>> @@ -1705,6 +1705,68 @@ void
>>> spice_usb_device_manager_disconnect_device(SpiceUsbDeviceManager *self,
>>> #endif
>>> }
>>>
>>> +typedef struct _disconnect_cb_data
>>> +{
>>> + SpiceUsbDeviceManager *self;
>>> + GSimpleAsyncResult *result;
>>> + SpiceUsbDevice *device;
>>> +} disconnect_cb_data;
>>> +
>>> +#ifdef USE_USBREDIR
>>> +static
>>> +void _disconnect_device_async_cb(GObject *gobject,
>>> + GAsyncResult *channel_res,
>>> + gpointer user_data)
>>> +{
>>> + disconnect_cb_data *data = user_data;
>>> + GSimpleAsyncResult *result = G_SIMPLE_ASYNC_RESULT(data->result);
>>> + SpiceUsbDeviceManager *self = SPICE_USB_DEVICE_MANAGER(data->self);
>>> +
>>> +#ifdef G_OS_WIN32
>>> + if (self->priv->use_usbclerk) {
>>> + _spice_usb_device_manager_uninstall_driver_async(self, data
>>> ->device);
>>> + }
>>> +#endif
>>> +
>>> + g_simple_async_result_complete(result);
>>> + g_object_unref(result);
>>> + g_free(data);
>>> +}
>>> +#endif
>>> +
>>> +void spice_usb_device_manager_disconnect_device_async(SpiceUsbDeviceManager
>>> *self,
>>> + SpiceUsbDevice
>>> *device,
>>> + GCancellable
>>> *cancellable,
>>> + GAsyncReadyCallback
>>> callback,
>>> + gpointer user_data)
>>> +{
>>> +#ifdef USE_USBREDIR
>>> + GSimpleAsyncResult *nested;
>>> + GSimpleAsyncResult *result;
>>> + g_return_if_fail(SPICE_IS_USB_DEVICE_MANAGER(self));
>>> +
>>> + g_return_if_fail(device != NULL);
>>> +
>>> + SPICE_DEBUG("disconnecting device %p", device);
>>> +
>>> + SpiceUsbredirChannel *channel;
>>> +
>>> + channel = spice_usb_device_manager_get_channel_for_dev(self, device);
>>> + nested = g_simple_async_result_new(G_OBJECT(self), callback,
>>> user_data,
>>> +
>>> spice_usb_device_manager_disconnect_device_async);
>>> + disconnect_cb_data *data = g_new(disconnect_cb_data, 1);
>>> + data->self = self;
>>> + data->result = nested;
>>> + data->device = device;
>>> +
>>> + result = g_simple_async_result_new(G_OBJECT(channel),
>>> + _disconnect_device_async_cb, data,
>>> + spice_usb_device_manager_disconnect_device_async);
>>> +
>>> + spice_usbredir_channel_disconnect_device_async(channel, result,
>>> cancellable);
>>> +#endif
>>> +}
>>> +
>>> /**
>>> * spice_usb_device_manager_can_redirect_device:
>>> * @self: the #SpiceUsbDeviceManager manager
>>> diff --git a/src/usb-device-manager.h b/src/usb-device-manager.h
>>> index e05ebae..d705e74 100644
>>> --- a/src/usb-device-manager.h
>>> +++ b/src/usb-device-manager.h
>>> @@ -116,6 +116,14 @@ void spice_usb_device_manager_connect_device_async(
>>> GCancellable *cancellable,
>>> GAsyncReadyCallback callback,
>>> gpointer user_data);
>>> +
>>> +void spice_usb_device_manager_disconnect_device_async(
>>> + SpiceUsbDeviceManager
>>> *manager,
>>> + SpiceUsbDevice *device,
>>> + GCancellable *cancellable,
>>> + GAsyncReadyCallback callback,
>>> + gpointer user_data);
>>> +
>
> Also, generally the convention with glib async functions is that there is a
> paired _finish() function which the user is required to call to retrieve the
> result of the async operation (and possibly free any internal resources used by
> the async task). I think we should follow the same convention here as well.
> Notice below how there is a _finish() function for connect_device_async()
_finish function added.
>
>
>>> gboolean spice_usb_device_manager_connect_device_finish(
>>> SpiceUsbDeviceManager *self, GAsyncResult *res, GError **err);
>>>
>>
>> Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list