Peter Xu <pet...@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 <pet...@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()) {
