[Spice-devel] [PATCH v6 10/14] usbredir: Disconnect USB device asynchronously

[Jonathon Jongsma]

On Sun, 2016-02-28 at 11:54 +0200, Dmitry Fleytman wrote:
> From: Kirill Moizik <kmoizik at redhat.com>
> 
> Signed-off-by: Kirill Moizik <kmoizik at redhat.com>
> Signed-off-by: Dmitry Fleytman <dfleytma at redhat.com>
> ---
>  src/channel-usbredir.c | 38 +++++++++++++++++++++++++++++++-------
>  1 file changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> index ebf9fce..3a10823 100644
> --- a/src/channel-usbredir.c
> +++ b/src/channel-usbredir.c
> @@ -114,20 +114,44 @@ static void
> spice_usbredir_channel_init(SpiceUsbredirChannel *channel)
>  }
>  
>  #ifdef USE_USBREDIR
> +
> +static void _channel_reset_cb(GObject *gobject,
> +                              GAsyncResult *result,
> +                              gpointer user_data)
> +{
> +    SpiceChannel *spice_channel =  SPICE_CHANNEL(gobject);
> +    SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(spice_channel);
> +    SpiceUsbredirChannelPrivate *priv = channel->priv;
> +    gboolean migrating = GPOINTER_TO_UINT(user_data);
> +    GError *err = NULL;
> +
> +    spice_usbredir_channel_lock(channel);
> +
> +    usbredirhost_close(priv->host);
> +    priv->host = NULL;
> +    /* Call set_context to re-create the host */
> +    spice_usbredir_channel_set_context(channel, priv->context);
> +    SPICE_CHANNEL_CLASS(spice_usbredir_channel_parent_class)
> ->channel_reset(spice_channel, migrating);
> +
> +    spice_usbredir_channel_unlock(channel);
> +
> +    spice_usbredir_channel_disconnect_device_finish(channel, result, &err);
> +    g_object_unref(result);
> +}
> +
>  static void spice_usbredir_channel_reset(SpiceChannel *c, gboolean migrating)
>  {
>      SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(c);
>      SpiceUsbredirChannelPrivate *priv = channel->priv;
>  
>      if (priv->host) {
> -        if (priv->state == STATE_CONNECTED)
> -            spice_usbredir_channel_disconnect_device(channel);
> -        usbredirhost_close(priv->host);
> -        priv->host = NULL;
> -        /* Call set_context to re-create the host */
> -        spice_usbredir_channel_set_context(channel, priv->context);
> +        if (priv->state == STATE_CONNECTED) {
> +            spice_usbredir_channel_disconnect_device_async(channel, NULL,
> +                _channel_reset_cb, GUINT_TO_POINTER(migrating));

I don't think I noticed this on the previous review, but it seems there is a
change in behavior introduced here. Previously, if priv->state was not
STATE_CONNECTED, it would omit the call to
 spice_usbredir_channel_disconnect_device(), but would proceed to call
usbredirhost_close(), etc. In the new version, these additional calls are done
in the async callback. And the async function is only executed if state is
STATE_CONNECTED. I'm afraid I don't know enough about this code to know whether
that's going to be an issue. Was this change intentional?

> +        }
> +    } else {
> +        SPICE_CHANNEL_CLASS(spice_usbredir_channel_parent_class)
> ->channel_reset(c, migrating);
>      }
> -    SPICE_CHANNEL_CLASS(spice_usbredir_channel_parent_class)
> ->channel_reset(c, migrating);
>  }
>  #endif
>  

Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>