[Spice-devel] [PATCH v5 07/13] UsbDeviceManager: Implement asynchronous disconnect device flow
Jonathon Jongsma
jjongsma at redhat.com
Wed Feb 10 23:24:59 UTC 2016
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().
>
> 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.
>
> > +
> > /* 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()
> > 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