From e3f24cdc345f888730ea679e482d6217fb3d2b2a Mon Sep 17 00:00:00 2001

From: Victor Toso <me@victortoso.com>

Date: Wed, 4 Sep 2019 12:37:34 +0200

Subject: [PATCH] Revert "channel: Abort migration in delayed unref"

This reverts commit 109e575 "channel: Abort migration in delayed

unref" in 2016-05-02 by Pavel Grunt <pgrunt@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@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 2eec4e0..9128e36 100644

--- a/src/spice-channel.c

+++ b/src/spice-channel.c

@@ -2410,7 +2410,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);

@@ -2418,13 +2417,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