Multifd has a separate loop to do TLS terminations gracefully.  Meanwhile,
it depends on two variables which records thread creations.

It works perfectly before, however relying on "whether some threads are
created" flag might be not as straightforward to decide a graceful
shutdown.

Since we'll need to dynamically identify TLS channels anyway with the new
helper (which is needed for main and postcopy channels), use the same
simple API for multifd channels too.  Also, we only need graceful shutdown
on success of migrations.

With that, we can remove the loop and drop migration_tls_channel_end().

The comment there is still a good explanation, move it over to the new
helper instead.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/tls.h     |  1 -
 migration/channel.c |  7 +++++++
 migration/multifd.c | 40 +++++++---------------------------------
 migration/tls.c     |  5 -----
 4 files changed, 14 insertions(+), 39 deletions(-)

diff --git a/migration/tls.h b/migration/tls.h
index 58b25e1228..75c918e156 100644
--- a/migration/tls.h
+++ b/migration/tls.h
@@ -36,7 +36,6 @@ void migration_tls_channel_connect(MigrationState *s,
                                    QIOChannel *ioc,
                                    const char *hostname,
                                    Error **errp);
-void migration_tls_channel_end(QIOChannel *ioc, Error **errp);
 /* Whether the QIO channel requires further TLS handshake? */
 bool migrate_channel_requires_tls_upgrade(QIOChannel *ioc);
 
diff --git a/migration/channel.c b/migration/channel.c
index 1ae839e5fe..a481b45eae 100644
--- a/migration/channel.c
+++ b/migration/channel.c
@@ -153,6 +153,13 @@ int migration_channel_read_peek(QIOChannel *ioc,
 bool migration_channel_shutdown_gracefully(QIOChannel *c, Error **errp)
 {
     if (object_dynamic_cast((Object *)c, 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.
+         */
         qio_channel_tls_bye(QIO_CHANNEL_TLS(c), errp);
     }
 
diff --git a/migration/multifd.c b/migration/multifd.c
index b255778855..cb0262076b 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -439,7 +439,7 @@ static void multifd_send_set_error(Error *err)
     }
 }
 
-static void multifd_send_terminate_threads(void)
+static void multifd_send_terminate_threads(bool succeeded)
 {
     int i;
 
@@ -460,6 +460,9 @@ static void multifd_send_terminate_threads(void)
 
         qemu_sem_post(&p->sem);
         if (p->c) {
+            if (succeeded) {
+                migration_channel_shutdown_gracefully(p->c, &error_warn);
+            }
             qio_channel_shutdown(p->c, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
         }
     }
@@ -541,50 +544,21 @@ static void multifd_send_cleanup_state(void)
 
 void multifd_send_shutdown(void)
 {
+    MigrationState *s = migrate_get_current();
     int i;
 
     if (!migrate_multifd()) {
         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();
+    multifd_send_terminate_threads(!migration_has_failed(s));
 
     for (i = 0; i < migrate_multifd_channels(); i++) {
         MultiFDSendParams *p = &multifd_send_state->params[i];
         Error *local_err = NULL;
 
         if (!multifd_send_cleanup_channel(p, &local_err)) {
-            migrate_set_error(migrate_get_current(), local_err);
+            migrate_set_error(s, local_err);
             error_free(local_err);
         }
     }
diff --git a/migration/tls.c b/migration/tls.c
index 284a6194b2..ca1595e05d 100644
--- a/migration/tls.c
+++ b/migration/tls.c
@@ -165,11 +165,6 @@ void migration_tls_channel_connect(MigrationState *s,
                               NULL);
 }
 
-void migration_tls_channel_end(QIOChannel *ioc, Error **errp)
-{
-    qio_channel_tls_bye(QIO_CHANNEL_TLS(ioc), errp);
-}
-
 bool migrate_channel_requires_tls_upgrade(QIOChannel *ioc)
 {
     if (!migrate_tls()) {
-- 
2.50.1

Peter Xu <peterx@redhat.com> writes:

> Multifd has a separate loop to do TLS terminations gracefully.  Meanwhile,
> it depends on two variables which records thread creations.
>
> It works perfectly before, however relying on "whether some threads are
> created" flag might be not as straightforward to decide a graceful
> shutdown.
>
> Since we'll need to dynamically identify TLS channels anyway with the new
> helper (which is needed for main and postcopy channels), use the same
> simple API for multifd channels too.  Also, we only need graceful shutdown
> on success of migrations.
>
> With that, we can remove the loop and drop migration_tls_channel_end().
>
> The comment there is still a good explanation, move it over to the new
> helper instead.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/tls.h     |  1 -
>  migration/channel.c |  7 +++++++
>  migration/multifd.c | 40 +++++++---------------------------------
>  migration/tls.c     |  5 -----
>  4 files changed, 14 insertions(+), 39 deletions(-)
>
> diff --git a/migration/tls.h b/migration/tls.h
> index 58b25e1228..75c918e156 100644
> --- a/migration/tls.h
> +++ b/migration/tls.h
> @@ -36,7 +36,6 @@ void migration_tls_channel_connect(MigrationState *s,
>                                     QIOChannel *ioc,
>                                     const char *hostname,
>                                     Error **errp);
> -void migration_tls_channel_end(QIOChannel *ioc, Error **errp);
>  /* Whether the QIO channel requires further TLS handshake? */
>  bool migrate_channel_requires_tls_upgrade(QIOChannel *ioc);
>  
> diff --git a/migration/channel.c b/migration/channel.c
> index 1ae839e5fe..a481b45eae 100644
> --- a/migration/channel.c
> +++ b/migration/channel.c
> @@ -153,6 +153,13 @@ int migration_channel_read_peek(QIOChannel *ioc,
>  bool migration_channel_shutdown_gracefully(QIOChannel *c, Error **errp)
>  {
>      if (object_dynamic_cast((Object *)c, 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.
> +         */
>          qio_channel_tls_bye(QIO_CHANNEL_TLS(c), errp);
>      }
>  
> diff --git a/migration/multifd.c b/migration/multifd.c
> index b255778855..cb0262076b 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -439,7 +439,7 @@ static void multifd_send_set_error(Error *err)
>      }
>  }
>  
> -static void multifd_send_terminate_threads(void)
> +static void multifd_send_terminate_threads(bool succeeded)
>  {
>      int i;
>  
> @@ -460,6 +460,9 @@ static void multifd_send_terminate_threads(void)
>  
>          qemu_sem_post(&p->sem);
>          if (p->c) {
> +            if (succeeded) {
> +                migration_channel_shutdown_gracefully(p->c, &error_warn);

Maybe keep the warning on failure to gracefully terminate?

> +            }
>              qio_channel_shutdown(p->c, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);

This triggers my multifd-code-smell detector. Reusing the loop. Could
the destination perhaps see this shutdown and start cleanup of it's
channels, causing the subsequent bye to fail?

>          }
>      }
> @@ -541,50 +544,21 @@ static void multifd_send_cleanup_state(void)
>  
>  void multifd_send_shutdown(void)
>  {
> +    MigrationState *s = migrate_get_current();
>      int i;
>  
>      if (!migrate_multifd()) {
>          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();
> +    multifd_send_terminate_threads(!migration_has_failed(s));

Looks awkward to pass this value in instead of caching inside the
function. But no big deal.

>  
>      for (i = 0; i < migrate_multifd_channels(); i++) {
>          MultiFDSendParams *p = &multifd_send_state->params[i];
>          Error *local_err = NULL;
>  
>          if (!multifd_send_cleanup_channel(p, &local_err)) {
> -            migrate_set_error(migrate_get_current(), local_err);
> +            migrate_set_error(s, local_err);
>              error_free(local_err);
>          }
>      }
> diff --git a/migration/tls.c b/migration/tls.c
> index 284a6194b2..ca1595e05d 100644
> --- a/migration/tls.c
> +++ b/migration/tls.c
> @@ -165,11 +165,6 @@ void migration_tls_channel_connect(MigrationState *s,
>                                NULL);
>  }
>  
> -void migration_tls_channel_end(QIOChannel *ioc, Error **errp)
> -{
> -    qio_channel_tls_bye(QIO_CHANNEL_TLS(ioc), errp);
> -}
> -
>  bool migrate_channel_requires_tls_upgrade(QIOChannel *ioc)
>  {
>      if (!migrate_tls()) {