[Spice-devel] [spice-gtk v1 3/4] channel-usbredir: Add FIXMEs on channel-reset issues

[Frediano Ziglio]

> 
> From: Victor Toso <me at victortoso.com>
> 
> This should not change code behavior, only add some comments.
> 
> Related: https://bugzilla.redhat.com/show_bug.cgi?id=1625550
> Signed-off-by: Victor Toso <victortoso at redhat.com>

Commit that introduced this regression is 9fbf679453d8dbfe797a738cb536136599d7adab,
not much comments on this specific commit but different explanation at near commits.

> ---
>  src/channel-usbredir.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> index 1d9c380..7f6e2e3 100644
> --- a/src/channel-usbredir.c
> +++ b/src/channel-usbredir.c
> @@ -158,16 +158,27 @@ 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_async(channel, NULL,
> -                _channel_reset_cb, GUINT_TO_POINTER(migrating));
> -        } else {
> -            _channel_reset_finish(channel);
> -        }
> -    } else {
> +    /* Host isn't running, just reset */
> +    if (!priv->host) {
>          SPICE_CHANNEL_CLASS(spice_usbredir_channel_parent_class)->channel_reset(c,
>          migrating);
> +        return;
>      }
> +
> +    /* Host is running, so we might need to disconnect the usb devices
> async.
> +     * This should not block channel_reset() otherwise we might run in
> reconnection
> +     * problems such as https://bugzilla.redhat.com/show_bug.cgi?id=1625550
> +     * No operation from here on should rely on SpiceChannel as its
> coroutine
> +     * might be terminated. */
> +
> +    if (priv->state == STATE_CONNECTED) {
> +        /* FIXME: We should add GCancellable here */
> +        spice_usbredir_channel_disconnect_device_async(channel, NULL,
> +            _channel_reset_cb, GUINT_TO_POINTER(migrating));

I think here the main problem is that channel_reset is not designed to
be asynchronous but the caller is expected the channel to be in a consistent
state ready to be connected again (at least the calls from spice_channel_coroutine).
Probably works as when called inside spice_channel_coroutine the usb is never
connected.

> +        return;
> +    }
> +
> +    /* FIXME: This does not chain-up with parent's channel-reset, which is a
> must */
> +    _channel_reset_finish(channel);
>  }
>  #endif
>  

Frediano