[Spice-devel] [spice-gtk v1] Revert "channel: Abort migration in delayed unref"

[Victor Toso]

From: Victor Toso <me at victortoso.com>

This reverts commit 109e575 "channel: Abort migration in delayed
unref" in 2016-05-02 by Pavel Grunt <pgrunt at redhat.com>

This commit is being reverted because it calls
spice_session_abort_migration() on the SpiceSession for the target
host while the function should receive the SpiceSession for current
host.

I can actually reproduce a bug where the password expires for the
current Spice session and the migration is going to fail because of
that. Before reverting this commit, remote-viewer would hang and the
following logs would occur:

- Migration starts on channel-main
 > ../src/channel-main.c:2174 migrate_channel_connect 1:0

- Using TLS
 > ../src/spice-channel.c:1934 main-1:0: switching to tls

- Following logs are related to failure to connect due Authentication
 > ../src/spice-channel.c:2000 main-1:0: use mini header: 1
 > ../src/spice-channel.c:1274 main-1:0: link result: reply 7
 > ../src/spice-channel.c:2680 main-1:0: Coroutine exit main-1:0
 > ../src/spice-channel.c:2871 main-1:0: reset

Remote-viewer would hang because we are in a state of migration and
SpiceMainChannel does not know that it failed because
spice_session_abort_migration() is being called on SpiceSession of
target host and no SpiceChannel::channel-event is being emitted.

Reverting this patch does abort migration thanks to
SpiceChannel::channel-event being emitted and caught by
SpiceMainChannel at migrate_channel_event_cb() with the log:

 > ../src/channel-main.c:2247 main-1:0: error or unhandled channel event during migration: 23
 > ../src/channel-main.c:2373 main-1:0: migrate failed: some channels failed to connect

Note that the reverted patch mentions the fix of bug [0] which is also
a virt-viewer with a hanging state. Looking back to the logs, the
interesting part around issues around usbredir, that is:

 > GSpice-DEBUG: channel-main.c:2236 migration: channel opened chan:0x29ddce0, left 6
 > GSpice-DEBUG: spice-channel.c:916 usbredir-9:0: Read error Error receiving data: Connection reset by peer
 > GSpice-DEBUG: spice-channel.c:1210 usbredir-9:0: error, switching to protocol 1 (spice 0.4)
 > GSpice-DEBUG: spice-channel.c:2433 usbredir-9:0: Coroutine exit usbredir-9:0
 > GSpice-DEBUG: spice-channel.c:2455 usbredir-9:0: Open coroutine starting 0x29e10d0
 > GSpice-DEBUG: spice-channel.c:2289 usbredir-9:0: Started background coroutine 0x29e0750
 > GSpice-DEBUG: spice-channel.c:916 usbredir-9:1: Read error Error receiving data: Connection reset by peer
 > GSpice-DEBUG: spice-channel.c:1076 usbredir-9:1: incomplete auth reply (-104/4)
 > GSpice-DEBUG: spice-channel.c:916 playback-5:0: Read error Error receiving data: Connection reset by peer
 > GSpice-DEBUG: spice-channel.c:1076 playback-5:0: incomplete auth reply (-104/4)
 > GSpice-DEBUG: spice-channel.c:916 display-2:0: Read error Error receiving data: Connection reset by peer
 > GSpice-DEBUG: spice-channel.c:1076 display-2:0: incomplete auth reply (-104/4)
 > GSpice-DEBUG: spice-session.c:1775 connecting 0x7f12bffffa50...
 > GSpice-DEBUG: spice-session.c:1850 open host lab.test.me:5900
 > GSpice-DEBUG: channel-main.c:2241 error or unhandled channel event during migration: 22
 > GSpice-DEBUG: spice-session.c:1665 session: disconnecting 0
 > GSpice-DEBUG: spice-channel.c:2160 usbredir-9:1: channel got error
 > GSpice-DEBUG: spice-channel.c:2433 usbredir-9:1: Coroutine exit usbredir-9:1
 > GSpice-DEBUG: channel-main.c:2241 error or unhandled channel event during migration: 22

So, the expected behavior was happening on error, which is
channel-main aborting the migration due SpiceChannel::channel-event
being emitted with some failure.

Also note that, with this commit reverted, I don't experience any
hanging after migration is aborted. Likely the original bug was fixed.

[0] https://bugzilla.redhat.com/show_bug.cgi?id=1318574

Related: https://bugzilla.redhat.com/show_bug.cgi?id=1695618

Signed-off-by: Victor Toso <victortoso at redhat.com>
---
 src/spice-channel.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/src/spice-channel.c b/src/spice-channel.c
index 61de177..315e287 100644
--- a/src/spice-channel.c
+++ b/src/spice-channel.c
@@ -2420,7 +2420,6 @@ static gboolean spice_channel_delayed_unref(gpointer data)
     SpiceChannel *channel = SPICE_CHANNEL(data);
     SpiceChannelPrivate *c = channel->priv;
     gboolean was_ready = c->state == SPICE_CHANNEL_STATE_READY;
-    SpiceSession *session;
 
     CHANNEL_DEBUG(channel, "Delayed unref channel %p", channel);
 
@@ -2428,13 +2427,6 @@ static gboolean spice_channel_delayed_unref(gpointer data)
 
     c->state = SPICE_CHANNEL_STATE_UNCONNECTED;
 
-    session = spice_channel_get_session(channel);
-    if (session && spice_session_is_for_migration(session)) {
-        /* error during migration - abort migration */
-        spice_session_abort_migration(session);
-        return FALSE;
-    }
-
     if (c->event != SPICE_CHANNEL_NONE) {
         g_coroutine_signal_emit(channel, signals[SPICE_CHANNEL_EVENT], 0, c->event);
         c->event = SPICE_CHANNEL_NONE;
-- 
2.21.0