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)
+{
+    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);
 }
 
-- 
2.50.1

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);
>  }