[PATCH] migration/multifd/tls: Cleanup BYE message processing on sender side

[PATCH] migration/multifd/tls: Cleanup BYE message processing on sender side

[Peter Xu]

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

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/multifd.c | 65 ++++++++++++++++++++++++---------------------
 1 file changed, 34 insertions(+), 31 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index b255778855..98873cee74 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.50.1

Re: [PATCH] migration/multifd/tls: Cleanup BYE message processing on sender side

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> 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
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/multifd.c | 65 ++++++++++++++++++++++++---------------------
>  1 file changed, 34 insertions(+), 31 deletions(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index b255778855..98873cee74 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

s/also//

> + * 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

s/writting/writing/

> + * the channel, because GnuTLS doesn't support concurrent writers.
> + */
> +static void migration_ioc_shutdown_gracefully(QIOChannel *ioc)

Reviewed-by: Fabiano Rosas <farosas@suse.de>