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