From 82f038d5961a8c8f896b499e31c638266e0291e9 Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Thu, 25 Sep 2025 16:16:01 -0400 Subject: [PATCH] migration/multifd/tls: Cleanup BYE message processing on sender side This patch is a trivial cleanup to the BYE messages on the multifd sender side. It could also be a fix, but since we do not have a solid clue, taking this as a cleanup only. One trivial concern is, migration_tls_channel_end() might be unsafe to be invoked in the migration thread if migration is not successful, because when failed / cancelled we do not know whether the multifd sender threads can be writting to the channels, while GnuTLS library (when it's a TLS channel) logically doesn't support concurrent writes. When at it, cleanup on a few things. What changed: - Introduce a helper to do graceful shutdowns with rich comment, hiding the details - Only send bye() iff migration succeeded, skip if it failed / cancelled - Detect TLS channel using channel type rather than thread created flags - Move the loop into the existing one that will close the channels, but do graceful shutdowns before channel shutdowns - local_err seems to have been leaked if set, fix it along the way Reviewed-by: Fabiano Rosas Link: https://lore.kernel.org/r/20250925201601.290546-1-peterx@redhat.com Signed-off-by: Peter Xu --- migration/multifd.c | 65 ++++++++++++++++++++++++--------------------- 1 file changed, 34 insertions(+), 31 deletions(-) diff --git a/migration/multifd.c b/migration/multifd.c index b2557788555..98873cee74f 100644 --- a/migration/multifd.c +++ b/migration/multifd.c @@ -439,6 +439,39 @@ static void multifd_send_set_error(Error *err) } } +/* + * Gracefully shutdown IOChannels. Only needed for successful migrations on + * top of TLS channels. Otherwise it is same to qio_channel_shutdown(). + * + * A successful migration also guarantees multifd sender threads are + * properly flushed and halted. It is only safe to send BYE in the + * migration thread here when we know there's no other thread writting to + * the channel, because GnuTLS doesn't support concurrent writers. + */ +static void migration_ioc_shutdown_gracefully(QIOChannel *ioc) +{ + g_autoptr(Error) local_err = NULL; + + if (!migration_has_failed(migrate_get_current()) && + object_dynamic_cast((Object *)ioc, TYPE_QIO_CHANNEL_TLS)) { + + /* + * The destination expects the TLS session to always be properly + * terminated. This helps to detect a premature termination in the + * middle of the stream. Note that older QEMUs always break the + * connection on the source and the destination always sees + * GNUTLS_E_PREMATURE_TERMINATION. + */ + migration_tls_channel_end(ioc, &local_err); + if (local_err) { + warn_report("Failed to gracefully terminate TLS connection: %s", + error_get_pretty(local_err)); + } + } + + qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL); +} + static void multifd_send_terminate_threads(void) { int i; @@ -460,7 +493,7 @@ static void multifd_send_terminate_threads(void) qemu_sem_post(&p->sem); if (p->c) { - qio_channel_shutdown(p->c, QIO_CHANNEL_SHUTDOWN_BOTH, NULL); + migration_ioc_shutdown_gracefully(p->c); } } @@ -547,36 +580,6 @@ void multifd_send_shutdown(void) return; } - for (i = 0; i < migrate_multifd_channels(); i++) { - MultiFDSendParams *p = &multifd_send_state->params[i]; - - /* thread_created implies the TLS handshake has succeeded */ - if (p->tls_thread_created && p->thread_created) { - Error *local_err = NULL; - /* - * The destination expects the TLS session to always be - * properly terminated. This helps to detect a premature - * termination in the middle of the stream. Note that - * older QEMUs always break the connection on the source - * and the destination always sees - * GNUTLS_E_PREMATURE_TERMINATION. - */ - migration_tls_channel_end(p->c, &local_err); - - /* - * The above can return an error in case the migration has - * already failed. If the migration succeeded, errors are - * not expected but there's no need to kill the source. - */ - if (local_err && !migration_has_failed(migrate_get_current())) { - warn_report( - "multifd_send_%d: Failed to terminate TLS connection: %s", - p->id, error_get_pretty(local_err)); - break; - } - } - } - multifd_send_terminate_threads(); for (i = 0; i < migrate_multifd_channels(); i++) { -- 2.47.3