Re: [PATCH 1/3] migration/tls: Gracefully shutdown main and preempt channels

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> QEMU supported graceful shutdowns for multifd channels starting from commit
> 48796f6b44 ("migration/multifd: Terminate the TLS connection").  Then error
> check was enabled for premature TLS terminations.
>
> Now if we run the preempt TLS unit test, the test would pass, but there
> will be a warning reported:
>
> qemu-system-x86_64: Cannot read from TLS channel: The TLS connection was non-properly terminated.
> ok 1 /x86_64/migration/postcopy/preempt/tls/psk
>
> To fix it, make the rest channels to be gracefully terminated too when it's
> a TLS channel.
>
> One note is that the qemufile helper needs to be in migration.c not
> qemu-file.c, because qemu-file.c will be linked in unit tests, which will
> not link channel.c unfortunately.
>
> Reported-by: Xiaohui Li <xiaohli@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/channel.h   |  3 +++
>  migration/migration.h |  2 ++
>  migration/channel.c   | 13 +++++++++++++
>  migration/migration.c | 24 +++++++++++++++++++++++-
>  4 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/migration/channel.h b/migration/channel.h
> index 5bdb8208a7..0b25dd7c5b 100644
> --- a/migration/channel.h
> +++ b/migration/channel.h
> @@ -29,4 +29,7 @@ int migration_channel_read_peek(QIOChannel *ioc,
>                                  const char *buf,
>                                  const size_t buflen,
>                                  Error **errp);
> +
> +bool migration_channel_shutdown_gracefully(QIOChannel *c, Error **errp);
> +
>  #endif
> diff --git a/migration/migration.h b/migration/migration.h
> index 01329bf824..b5763af057 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -594,4 +594,6 @@ void migration_bitmap_sync_precopy(bool last_stage);
>  void dirty_bitmap_mig_init(void);
>  bool should_send_vmdesc(void);
>  
> +bool qemu_file_shutdown_gracefully(QEMUFile *f, Error **errp);
> +
>  #endif
> diff --git a/migration/channel.c b/migration/channel.c
> index a547b1fbfe..1ae839e5fe 100644
> --- a/migration/channel.c
> +++ b/migration/channel.c
> @@ -145,3 +145,16 @@ int migration_channel_read_peek(QIOChannel *ioc,
>  
>      return 0;
>  }
> +
> +/*
> + * This is only needed for a successful migration, no-op for non-TLS
> + * channels.  For unexpected interruptions, use qio_channel_shutdown().
> + */
> +bool migration_channel_shutdown_gracefully(QIOChannel *c, Error **errp)
> +{

    ERRP_GUARD();

due to dereferencing errp below

> +    if (object_dynamic_cast((Object *)c, TYPE_QIO_CHANNEL_TLS)) {
> +        qio_channel_tls_bye(QIO_CHANNEL_TLS(c), errp);
> +    }
> +
> +    return *errp == NULL;
> +}
> diff --git a/migration/migration.c b/migration/migration.c
> index 10c216d25d..7015c2b5e0 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -113,6 +113,27 @@ static bool close_return_path_on_source(MigrationState *s);
>  static void migration_completion_end(MigrationState *s);
>  static void migrate_hup_delete(MigrationState *s);
>  
> +/*
> + * See migration_channel_shutdown_gracefully().  The "graceful" versions
> + * are only needed if migration succeeded.
> + */
> +bool qemu_file_shutdown_gracefully(QEMUFile *f, Error **errp)
> +{
> +    int ret;
> +
> +    if (!migration_channel_shutdown_gracefully(qemu_file_get_ioc(f), errp)) {
> +        return false;
> +    }
> +
> +    ret = qemu_file_shutdown(f);
> +    if (ret) {
> +        error_setg_errno(errp, -ret, "qemu_file_shutdown() failed");
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
>  static void migration_downtime_start(MigrationState *s)
>  {
>      trace_vmstate_downtime_checkpoint("src-downtime-start");
> @@ -2473,11 +2494,12 @@ static void migration_release_dst_files(MigrationState *ms)
>       */
>      if (ms->postcopy_qemufile_src) {
>          migration_ioc_unregister_yank_from_file(ms->postcopy_qemufile_src);
> -        qemu_file_shutdown(ms->postcopy_qemufile_src);
> +        qemu_file_shutdown_gracefully(ms->postcopy_qemufile_src, &error_warn);
>          qemu_fclose(ms->postcopy_qemufile_src);
>          ms->postcopy_qemufile_src = NULL;
>      }
>  
> +    qemu_file_shutdown_gracefully(file, &error_warn);
>      qemu_fclose(file);
>  }