On Mon, Jul 06, 2015 at 08:59:06PM +0300, Kirill Moizik wrote:
> we will spawn a separate thread to disconnect usb device, when finish we will update widget to allow further
> usb redirection operations
> 1) expose spice_usbredir_channel_connect_device_async function for asynchronous disconnect
> 2) threads synchronization
Honestly, moving non-trivial existing functions from running in the main
thread to running in a separate thread with very little locking added,
and no high level explanation as to why everything will be fine and
nothing can be racy is very scary... One example below:
>
> Signed-off-by: Kirill Moizik <kmoizik@xxxxxxxxxx>
> ---
> src/channel-usbredir-priv.h | 8 ++++-
> src/channel-usbredir.c | 79 ++++++++++++++++++++++++++++++++++++++-------
> src/map-file | 1 +
> src/usb-device-manager.c | 62 ++++++++++++++++++++++++++++++++++-
> src/usb-device-manager.h | 8 +++++
> src/usb-device-widget.c | 19 +++++++++--
> 6 files changed, 161 insertions(+), 16 deletions(-)
>
> diff --git a/src/channel-usbredir-priv.h b/src/channel-usbredir-priv.h
> index 2c4c6f7..c3ff158 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);
> +
> /* 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(
> @@ -47,7 +51,9 @@ gboolean spice_usbredir_channel_connect_device_finish(
> GAsyncResult *res,
> GError **err);
>
> -void spice_usbredir_channel_disconnect_device(SpiceUsbredirChannel *channel);
> +void spice_usbredir_channel_disconnect_device(GSimpleAsyncResult *simple,
> + GObject *object,
> + GCancellable *cancellable);
>
> libusb_device *spice_usbredir_channel_get_device(SpiceUsbredirChannel *channel);
>
> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> index e7d5949..1e78350 100644
> --- a/src/channel-usbredir.c
> +++ b/src/channel-usbredir.c
> @@ -79,6 +79,7 @@ struct _SpiceUsbredirChannelPrivate {
> GSimpleAsyncResult *result;
> SpiceUsbAclHelper *acl_helper;
> #endif
> + void* redirect_mutex;
usbredir_alloc_lock returns a void* only because that's what usbredir
expects. Let's just cast it to GMutex * here, and use
g_mutex_lock/unlock.
[snip]
> @@ -440,7 +491,9 @@ void spice_usbredir_channel_disconnect_device(SpiceUsbredirChannel *channel)
> spice_usb_device_manager_get(session, NULL));
> }
> /* This also closes the libusb handle we passed from open_device */
> + usbredir_lock_lock(channel->priv->redirect_mutex);
> usbredirhost_set_device(priv->host, NULL);
> + usbredir_unlock_lock(channel->priv->redirect_mutex);
> libusb_unref_device(priv->device);
> priv->device = NULL;
> g_boxed_free(spice_usb_device_get_type(), priv->spice_device);
> priv->spice_device = NULL;
>
> @@ -652,7 +705,9 @@ static void usbredir_handle_msg(SpiceChannel *c, SpiceMsgIn *in)
> priv->read_buf = buf;
> priv->read_buf_size = size;
>
> + usbredir_lock_lock(priv->redirect_mutex);
> r = usbredirhost_read_guest_data(priv->host);
> + usbredir_unlock_lock(priv->redirect_mutex);
> if (r != 0) {
> SpiceUsbDevice *spice_device = priv->spice_device;
> gchar *desc;
> GError *err;
> g_return_if_fail(spice_device != NULL);
The 2 hunks I kept are the only 2 places loking/unlocking
redirect_mutex, I assume this means they can race with each other.
If that's true, then we can get in a situation where 2nd hunk code runs
till
if (r != 0) {
with r != 0, then 1st hunk runs and sets priv->spice_device to NULL,
then 2nd hunk resumes, and we trigger the g_return_if_fail() (which
usually is used to indicate a programming error)
Christophe
Attachment:
pgpNUIz8qJE_Q.pgp
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel