[Spice-devel] [spice-gtk] Don't call channel_connect from channel_disconnect in SWITCHING state

[Marc-André Lureau]

Hi

----- Original Message -----
> commit 26b7767 changed channel_disconnect() to call channel_connect()
> instead of spice_channel_connect() when the channel state is
> SPICE_CHANNEL_STATE_SWITCHING. However spice_channel_connect()
> returns early when c->state >= SPICE_CHANNEL_STATE_CONNECTING without
> doing anything, so calling channel_connect() instead is a significant
> change of behaviour (SPICE_CHANNEL_STATE_SWITCHING int value is 4 while
> SPICE_CHANNEL_STATE_CONNECTING is 2).

connect is necessary for "switch-host" migration to happen, iirc

> 
> This is actually causing issues when a migration is cancelled.
> If channel_disconnect is called from coroutine context when state is
> SPICE_CHANNEL_STATE_SWITCHING:
> - channel_connect() is called and queues an idle which will call
>   connect_delayed()
> - then spice_session_set_migration_state() is called and will call
>   g_coroutine_object_notify(), which will queue an idle to emit the
>   notification, and yield to the main context
> - connect_delayed() gets called in an idle, resets the current
>   coroutine, and then yield back to the coroutine context
> - g_coroutine_object_notify() resumes and is confused as the
>   notification did not happen.

I wonder how I managed to test 8f40839 (I remember I did!): I guess
spice_channel_connect() was discarded so no warning, but how did
switching happen?

Now that connect() actually happens as it should imho,
the two lines could be swapped, that should remove the warning.

I am a bit worried about the connect_delayed() resetting the coroutine
that might not be finished, it should probably resume it until it ends.
I think gcoroutine has a chance to improve things here with better checks
and API.

> 
> This can be triggered by adding a g_usleep(10000000); at the beginning
> of spice_session_start_migrating(), and by migrating a VM after
> connecting to it.
> ---
>  gtk/spice-channel.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/gtk/spice-channel.c b/gtk/spice-channel.c
> index eec63b1..99c6748 100644
> --- a/gtk/spice-channel.c
> +++ b/gtk/spice-channel.c
> @@ -2654,7 +2654,6 @@ static void channel_disconnect(SpiceChannel *channel)
>      g_return_if_fail(SPICE_IS_CHANNEL(channel));
>  
>      if (c->state == SPICE_CHANNEL_STATE_SWITCHING) {
> -        channel_connect(channel, c->tls);
>          spice_session_set_migration_state(spice_channel_get_session(channel),
>                                            SPICE_SESSION_MIGRATION_NONE);
>      }
> --
> 2.1.0
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>