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,
+                                             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;
+
+    if (migrate_start(&from, &to, "defer", &args->start)) {
+        return;
+    }
+
+    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)
+{
+    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);
 }
-- 
2.43.0

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

Hi Fabiano-san

Thanks for the review. Here's how I plan to address each point.

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

I'll rename `do_test_validate_capability_pair` to `validate_caps_pair`.

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

Good point. The capability validation only needs the source side, so
I'll set `args->start.only_source = true` to skip launching the target
VM.


> 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.

Understood. I'll make `validate_caps_pair` just take a `QTestState
*from` and let the caller manage the VM lifecycle. To allow testing
multiple pairs on the same VM instance, I'll reset the first
capability back to `false` after each validation.

> 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:
I'll follow the `test_cancel_src_after_status()` pattern -- use
`g_path_get_basename(test_path)` to extract the pair name and branch
accordingly. Registration will use
`migration_test_add_suffix("/migration/validate_caps/",
"mapped_ram_postcopy", ...)`, so adding a new incompatible pair in the
future is just a matter of adding a branch and a registration call.

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

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

Hi, Fabiano,

> 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.

Understood. I will send future revisions as a new thread.

> 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.

Thanks for pointing this out. I understand the cleanup issue with
src_serial, as well as the current limitation around migrate_end() and
only_source.

> 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.

That sounds good to me. Thank you for taking care of that when
applying the patch. I will not send a new version and will leave it as
is.

Thanks again for the review.

Best regards,
Takeru