Re: [PATCH v2] 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
>
>  v2:
>    - Rename do_test_validate_capability_pair to validate_caps_pair
>    - Move migrate_start/migrate_end to caller for VM reuse
>    - Set only_source to skip target VM
>    - Generalize test using migration_test_add_suffix pattern

Hi, thank you for the patch. It looks good aside from a small functional
issue I'll mention below.

Let me just point out that is customary to not send patches in reply to
the previous thread. So this v2 should have been its own thread
completely independent of the first version. This helps the community
keep track of the various series on the list without patches getting
lost deep inside mail threads.

> ---
>  tests/qtest/migration/misc-tests.c | 50 ++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
>
> diff --git a/tests/qtest/migration/misc-tests.c b/tests/qtest/migration/misc-tests.c
> index 196f1ca84287..4131e7b0614d 100644
> --- a/tests/qtest/migration/misc-tests.c
> +++ b/tests/qtest/migration/misc-tests.c
> @@ -215,6 +215,53 @@ static void do_test_validate_uri_channel(MigrateCommon *args)
>      migrate_end(from, to, false);
>  }
>  
> +static void validate_caps_pair(QTestState *from,
> +                               const char *first_capability,
> +                               const char *second_capability,
> +                               const char *expected_error)
> +{
> +    QDict *rsp;
> +    const char *error_desc;
> +
> +    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_set_capability(from, first_capability, false);
> +}
> +
> +static void test_validate_caps_pair(char *test_path, MigrateCommon *args)
> +{
> +    g_autofree char *cap_pair = g_path_get_basename(test_path);
> +    QTestState *from, *to;
> +
> +    args->start.hide_stderr = true;
> +    args->start.only_source = true;
> +
> +    if (migrate_start(&from, &to, "defer", &args->start)) {
> +        return;
> +    }
> +
> +    if (g_str_equal(cap_pair, "mapped_ram_postcopy")) {
> +        const char *error =
> +            "Mapped-ram migration is incompatible with postcopy";
> +
> +        validate_caps_pair(from, "mapped-ram", "postcopy-ram", error);
> +        validate_caps_pair(from, "postcopy-ram", "mapped-ram", error);
> +    }
> +
> +    qtest_quit(from);

This will leave the "src_serial" file behind in the test directory and
will cause cleanup of the tests to fail. However, I see that using
migrate_end() here would cause an access to the not-initialized "to"
pointer. I actually misremembered the code we have in master when I
suggested to use only_source for this, I have a patch making
migrate_end() accept NULL that has not been merged yet.

As a temporary solution, we could just unlink() the src_serial file here
directly. I'll do that myself when applying the patch, you don't need to
send a new version.

> +}
> +
>  static void test_validate_uri_channels_both_set(char *name, MigrateCommon *args)
>  {
>      args->listen_uri = "defer",
> @@ -276,4 +323,7 @@ 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_suffix("/migration/validate_caps/",
> +                              "mapped_ram_postcopy",
> +                              test_validate_caps_pair);
>  }