Re: [PATCH] tests/qtest/migration: Add mapped-ram/postcopy validation test

[Fabiano Rosas <farosas@suse.de>]

Takeru Hayasaka <hayatake396@gmail.com> writes:

> The migration capability checks reject enabling postcopy-ram together
> with mapped-ram, but there is no qtest covering this incompatibility.
>
> Add a validation test that verifies QMP rejects the combination in
> both capability ordering cases and returns the expected error.
>
> This keeps the existing capability boundary covered without changing
> migration behavior.
>
> Signed-off-by: Takeru Hayasaka <hayatake396@gmail.com>
> ---
> Built with:
>   ./configure --target-list=x86_64-softmmu --disable-werror
>   ninja -C build tests/qtest/migration-test qemu-system-x86_64
>
> Tested with:
>   QTEST_QEMU_BINARY=./build/qemu-system-x86_64 \
>   ./build/tests/qtest/migration-test --full \
>   -p /x86_64/migration/validate_caps/mapped_ram_postcopy
> ---
>  tests/qtest/migration/misc-tests.c | 45 ++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>
> diff --git a/tests/qtest/migration/misc-tests.c b/tests/qtest/migration/misc-tests.c
> index 196f1ca84287..ae247250713b 100644
> --- a/tests/qtest/migration/misc-tests.c
> +++ b/tests/qtest/migration/misc-tests.c
> @@ -215,6 +215,49 @@ static void do_test_validate_uri_channel(MigrateCommon *args)
>      migrate_end(from, to, false);
>  }
>  
> +static void do_test_validate_capability_pair(MigrateCommon *args,

You could choose a shorter name here to make it easier on the eyes when
calling this multiple times. "do_test" is certainly superfluous.

> +                                             const char *first_capability,
> +                                             const char *second_capability,
> +                                             const char *expected_error)
> +{
> +    QTestState *from, *to;
> +    QDict *rsp;
> +    const char *error_desc;
> +
> +    args->start.hide_stderr = true;

Could set args->start.only_source here to save a bit of time.

> +
> +    if (migrate_start(&from, &to, "defer", &args->start)) {
> +        return;
> +    }

If you move the start and end into the caller, we could use this
function to test several pairs of capabilities without having to start a
new QEMU instance every time.

> +
> +    migrate_set_capability(from, first_capability, true);
> +
> +    rsp = qtest_qmp_assert_failure_ref(
> +        from,
> +        "{ 'execute': 'migrate-set-capabilities',"
> +        "  'arguments': { 'capabilities': [ { "
> +        "      'capability': %s, 'state': true } ] } }",
> +        second_capability);
> +
> +    error_desc = qdict_get_str(rsp, "desc");
> +    g_assert_cmpstr(error_desc, ==, expected_error);
> +    qobject_unref(rsp);
> +
> +    migrate_end(from, to, false);
> +}
> +
> +static void test_validate_caps_mapped_ram_postcopy(char *name,
> +                                                   MigrateCommon *args)
> +{

I think this would be more useful if it were not restricted to just
mapped-ram and postcopy. Take a look at migration_test_add_suffix() and
test_cancel_src_after_status() on how to define a test that can be
applied to multiple strings. Something roughly like this:


  g_autofree char *cap_pair = g_path_get_basename(test_path);
  
  if (g_str_equal(cap_pair, "mapped_ram_postcopy")) {
      const char *error =
          "Mapped-ram migration is incompatible with postcopy";
  
      validate_caps_pair(args, "mapped-ram", "postcopy-ram", error);
      validate_caps_pair(args, "postcopy-ram", "mapped-ram", error);
  }

> +    const char *error =
> +        "Mapped-ram migration is incompatible with postcopy";
> +
> +    do_test_validate_capability_pair(args, "mapped-ram", "postcopy-ram",
> +                                     error);
> +    do_test_validate_capability_pair(args, "postcopy-ram", "mapped-ram",
> +                                     error);
> +}
> +
>  static void test_validate_uri_channels_both_set(char *name, MigrateCommon *args)
>  {
>      args->listen_uri = "defer",
> @@ -276,4 +319,6 @@ void migration_test_add_misc(MigrationTestEnv *env)
>                         test_validate_uri_channels_both_set);
>      migration_test_add("/migration/validate_uri/channels/none_set",
>                         test_validate_uri_channels_none_set);
> +    migration_test_add("/migration/validate_caps/mapped_ram_postcopy",
> +                       test_validate_caps_mapped_ram_postcopy);
>  }