Re: [PATCH v3 28/51] tests/qtest/migration: Stop invoking migrate_incoming from hooks

[Peter Xu <peterx@redhat.com>]

On Mon, Dec 15, 2025 at 07:00:14PM -0300, Fabiano Rosas wrote:
> Now that the listen_uri is being properly used, tests can stop calling
> migrate_incoming from their hooks. The _common functions and
> migrate_start should take care of that.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

IMHO this patch is almost fine,

Reviewed-by: Peter Xu <peterx@redhat.com>

still, some thoughts inline.

> ---
>  tests/qtest/migration/compression-tests.c |  6 ++++++
>  tests/qtest/migration/framework.c         | 14 +++++++++++---
>  tests/qtest/migration/precopy-tests.c     |  7 ++++---
>  tests/qtest/migration/tls-tests.c         |  8 ++++++++
>  4 files changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/qtest/migration/compression-tests.c b/tests/qtest/migration/compression-tests.c
> index eb0b7d6b4b..bed39dece0 100644
> --- a/tests/qtest/migration/compression-tests.c
> +++ b/tests/qtest/migration/compression-tests.c
> @@ -33,6 +33,7 @@ migrate_hook_start_precopy_tcp_multifd_zstd(QTestState *from,
>  
>  static void test_multifd_tcp_zstd(char *name, MigrateCommon *args)
>  {
> +    args->listen_uri = "tcp:127.0.0.1:0";

Definitely a step forward to unify migrate_incoming into the precopy
framework, however lots duplication of this "tcp:*" string..

Shall we provide some migrate_common_set_listen_uri_default()?

>      args->start_hook = migrate_hook_start_precopy_tcp_multifd_zstd;
>  
>      args->start.incoming_defer = true;
> @@ -43,6 +44,7 @@ static void test_multifd_tcp_zstd(char *name, MigrateCommon *args)
>  
>  static void test_multifd_postcopy_tcp_zstd(char *name, MigrateCommon *args)
>  {
> +    args->listen_uri = "tcp:127.0.0.1:0";
>      args->start_hook = migrate_hook_start_precopy_tcp_multifd_zstd,
>  
>      args->start.incoming_defer = true;
> @@ -66,6 +68,7 @@ migrate_hook_start_precopy_tcp_multifd_qatzip(QTestState *from,
>  
>  static void test_multifd_tcp_qatzip(char *name, MigrateCommon *args)
>  {
> +    args->listen_uri = "tcp:127.0.0.1:0";
>      args->start_hook = migrate_hook_start_precopy_tcp_multifd_qatzip;
>  
>      args->start.incoming_defer = true;
> @@ -85,6 +88,7 @@ migrate_hook_start_precopy_tcp_multifd_qpl(QTestState *from,
>  
>  static void test_multifd_tcp_qpl(char *name, MigrateCommon *args)
>  {
> +    args->listen_uri = "tcp:127.0.0.1:0";
>      args->start_hook = migrate_hook_start_precopy_tcp_multifd_qpl;
>  
>      args->start.incoming_defer = true;
> @@ -104,6 +108,7 @@ migrate_hook_start_precopy_tcp_multifd_uadk(QTestState *from,
>  
>  static void test_multifd_tcp_uadk(char *name, MigrateCommon *args)
>  {
> +    args->listen_uri = "tcp:127.0.0.1:0";
>      args->start_hook = migrate_hook_start_precopy_tcp_multifd_uadk;
>  
>      args->start.incoming_defer = true;
> @@ -156,6 +161,7 @@ migrate_hook_start_precopy_tcp_multifd_zlib(QTestState *from,
>  
>  static void test_multifd_tcp_zlib(char *name, MigrateCommon *args)
>  {
> +    args->listen_uri = "tcp:127.0.0.1:0";
>      args->start_hook = migrate_hook_start_precopy_tcp_multifd_zlib;
>  
>      args->start.incoming_defer = true;
> diff --git a/tests/qtest/migration/framework.c b/tests/qtest/migration/framework.c
> index e811945122..199e439263 100644
> --- a/tests/qtest/migration/framework.c
> +++ b/tests/qtest/migration/framework.c
> @@ -820,6 +820,9 @@ int test_precopy_common(MigrateCommon *args)
>      QObject *out_channels = NULL;
>  
>      g_assert(!args->cpr_channel || args->connect_channels);
> +    if (args->start.incoming_defer) {
> +        g_assert(args->listen_uri || args->connect_channels);
> +    }
>  
>      if (migrate_start(&from, &to, args->listen_uri, &args->start)) {
>          return -1;
> @@ -829,6 +832,14 @@ int test_precopy_common(MigrateCommon *args)
>          data_hook = args->start_hook(from, to);
>      }
>  
> +    if (args->start.incoming_defer && !args->start.defer_target_connect) {
> +        if (args->connect_channels) {
> +            in_channels = qobject_from_json(args->connect_channels,
> +                                            &error_abort);
> +        }
> +        migrate_incoming_qmp(to, args->listen_uri, in_channels, "{}");
> +    }

The changes here in the framework code looks all correct, even though I
don't think "connect_channels" is used here in this patch.

Said that, IMHO the channel management is chaos right now in our qtest..
At least it took me some time staring at this path when reviewing.

IMHO a major reason is due to the cpr complexities.

E.g. test_mode_transfer_common() used different things to specify incoming
channels (cpr_channel, opts_target, connect_channels).  We should clean
them up at some point..

> +
>      /* Wait for the first serial output from the source */
>      if (args->result == MIG_TEST_SUCCEED) {
>          wait_for_serial("src_serial");
> @@ -1060,9 +1071,6 @@ void *migrate_hook_start_precopy_tcp_multifd_common(QTestState *from,
>      migrate_set_parameter_str(from, "multifd-compression", method);
>      migrate_set_parameter_str(to, "multifd-compression", method);
>  
> -    /* Start incoming migration from the 1st socket */
> -    migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
> -
>      return NULL;
>  }
>  
> diff --git a/tests/qtest/migration/precopy-tests.c b/tests/qtest/migration/precopy-tests.c
> index d9c463dd0f..ab5789717f 100644
> --- a/tests/qtest/migration/precopy-tests.c
> +++ b/tests/qtest/migration/precopy-tests.c
> @@ -239,9 +239,6 @@ static void *migrate_hook_start_fd(QTestState *from,
>                                   "  'arguments': { 'fdname': 'fd-mig' }}");
>      close(pair[0]);
>  
> -    /* Start incoming migration from the 1st socket */
> -    migrate_incoming_qmp(to, "fd:fd-mig", NULL, "{}");
> -
>      /* Send the 2nd socket to the target */
>      qtest_qmp_fds_assert_success(from, &pair[1], 1,
>                                   "{ 'execute': 'getfd',"
> @@ -283,6 +280,7 @@ static void migrate_hook_end_fd(QTestState *from,
>  static void test_precopy_fd_socket(char *name, MigrateCommon *args)
>  {
>      args->connect_uri = "fd:fd-mig";
> +    args->listen_uri = "fd:fd-mig";
>      args->start_hook = migrate_hook_start_fd;
>      args->end_hook = migrate_hook_end_fd;
>  
> @@ -484,6 +482,7 @@ static void test_multifd_tcp_uri_none(char *name, MigrateCommon *args)
>       * everything will work alright even if guest page is changing.
>       */
>      args->live = true;
> +    args->listen_uri = "tcp:127.0.0.1:0";
>  
>      args->start.incoming_defer = true;
>      args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true;
> @@ -500,6 +499,7 @@ static void test_multifd_tcp_zero_page_legacy(char *name, MigrateCommon *args)
>       * everything will work alright even if guest page is changing.
>       */
>      args->live = true;
> +    args->listen_uri = "tcp:127.0.0.1:0";
>  
>      args->start.incoming_defer = true;
>      args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true;
> @@ -516,6 +516,7 @@ static void test_multifd_tcp_no_zero_page(char *name, MigrateCommon *args)
>       * everything will work alright even if guest page is changing.
>       */
>      args->live = true;
> +    args->listen_uri = "tcp:127.0.0.1:0";
>  
>      args->start.incoming_defer = true;
>      args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true;
> diff --git a/tests/qtest/migration/tls-tests.c b/tests/qtest/migration/tls-tests.c
> index 166f27f478..f63f37132a 100644
> --- a/tests/qtest/migration/tls-tests.c
> +++ b/tests/qtest/migration/tls-tests.c
> @@ -677,6 +677,7 @@ static void test_multifd_tcp_tls_psk_match(char *name, MigrateCommon *args)
>  {
>      args->start_hook = migrate_hook_start_multifd_tcp_tls_psk_match;
>      args->end_hook = migrate_hook_end_tls_psk;
> +    args->listen_uri = "tcp:127.0.0.1:0";
>  
>      args->start.incoming_defer = true;
>      args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true;
> @@ -689,6 +690,7 @@ static void test_multifd_tcp_tls_psk_mismatch(char *name, MigrateCommon *args)
>      args->start_hook = migrate_hook_start_multifd_tcp_tls_psk_mismatch;
>      args->end_hook = migrate_hook_end_tls_psk;
>      args->result = MIG_TEST_FAIL;
> +    args->listen_uri = "tcp:127.0.0.1:0";
>  
>      args->start.hide_stderr = true;
>      args->start.incoming_defer = true;
> @@ -702,6 +704,7 @@ static void test_multifd_postcopy_tcp_tls_psk_match(char *name,
>  {
>      args->start_hook = migrate_hook_start_multifd_tcp_tls_psk_match;
>      args->end_hook = migrate_hook_end_tls_psk;
> +    args->listen_uri = "tcp:127.0.0.1:0";
>  
>      args->start.incoming_defer = true;
>      args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true;
> @@ -716,6 +719,7 @@ static void test_multifd_tcp_tls_x509_default_host(char *name,
>  {
>      args->start_hook = migrate_hook_start_multifd_tls_x509_default_host;
>      args->end_hook = migrate_hook_end_tls_x509;
> +    args->listen_uri = "tcp:127.0.0.1:0";
>  
>      args->start.incoming_defer = true;
>      args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true;
> @@ -728,6 +732,7 @@ static void test_multifd_tcp_tls_x509_override_host(char *name,
>  {
>      args->start_hook = migrate_hook_start_multifd_tls_x509_override_host;
>      args->end_hook = migrate_hook_end_tls_x509;
> +    args->listen_uri = "tcp:127.0.0.1:0";
>  
>      args->start.incoming_defer = true;
>      args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true;
> @@ -754,6 +759,7 @@ static void test_multifd_tcp_tls_x509_mismatch_host(char *name,
>      args->start_hook = migrate_hook_start_multifd_tls_x509_mismatch_host;
>      args->end_hook = migrate_hook_end_tls_x509;
>      args->result = MIG_TEST_FAIL;
> +    args->listen_uri = "tcp:127.0.0.1:0";
>  
>      args->start.incoming_defer = true;
>      args->start.hide_stderr = true;
> @@ -767,6 +773,7 @@ static void test_multifd_tcp_tls_x509_allow_anon_client(char *name,
>  {
>      args->start_hook = migrate_hook_start_multifd_tls_x509_allow_anon_client;
>      args->end_hook = migrate_hook_end_tls_x509;
> +    args->listen_uri = "tcp:127.0.0.1:0";
>  
>      args->start.incoming_defer = true;
>      args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true;
> @@ -780,6 +787,7 @@ static void test_multifd_tcp_tls_x509_reject_anon_client(char *name,
>      args->start_hook = migrate_hook_start_multifd_tls_x509_reject_anon_client;
>      args->end_hook = migrate_hook_end_tls_x509;
>      args->result = MIG_TEST_FAIL;
> +    args->listen_uri = "tcp:127.0.0.1:0";
>  
>      args->start.incoming_defer = true;
>      args->start.hide_stderr = true;
> -- 
> 2.51.0
> 

-- 
Peter Xu