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

[Jonathon Jongsma]

Sorry, I missed a few more things on this patch the last time I reviewed it :/

On Tue, 2016-03-08 at 16:05 +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 | 48 +++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 41 insertions(+), 7 deletions(-)
> 
> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> index 7253ce6..ba9e4ab 100644
> --- a/src/channel-usbredir.c
> +++ b/src/channel-usbredir.c
> @@ -113,20 +113,54 @@ static void
> spice_usbredir_channel_init(SpiceUsbredirChannel *channel)
>  }
>  
>  #ifdef USE_USBREDIR
> +
> +static void _channel_reset_finish(SpiceUsbredirChannel *channel)
> +{
> +    SpiceUsbredirChannelPrivate *priv = channel->priv;
> +
> +    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_usbredir_channel_unlock(channel);
> +}
> +
> +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);
> +    gboolean migrating = GPOINTER_TO_UINT(user_data);
> +    GError *err = NULL;
> +
> +    _channel_reset_finish(channel);
> +
> +    SPICE_CHANNEL_CLASS(spice_usbredir_channel_parent_class)
> ->channel_reset(spice_channel, migrating);
> +
> +    spice_usbredir_channel_disconnect_device_finish(channel, result, &err);

err is leaked if this call is returns a failure.

> +    g_object_unref(result);

result should not be unreffed here. The only reference should be held by the
async operation that invokes this callback. Presumably it will be released after
this function returns and you will get a crash. If there's no crash, there may
be a leak, but it should be fixed elsewhere.

> +}
> +
>  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));
> +        } else {
> +            _channel_reset_finish(channel);
> +        }
> +    } 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>