[PATCH v2 00/18] migration: MigrationParameters changes

[PATCH v2 00/18] migration: MigrationParameters changes

[Fabiano Rosas]

since v1:

- Made the commit message clearer on the checkpatch change.

- Renamed the variables in the merge function to indicate which
  structure is the one receiving the update.

- Rewrote the string parsing on the HMP test because it was allowing
  false positives and tripping on readline escape sequences.

- Dropped the has_* fields validation patch because it was simply
  doing nothing (thank you Peter for spotting it).

- Due to the above I decided to leave MigrateParameter (singular)
  untouched. The enum max is just too convenient to lose.

- Used an array instead of a switch for the capabilities compat
  function.

- Fixed the copy of MigrationCapability documentation over to
  MigrationParameters in migration.json which was not verbatim.

- Added missing return statement in migrate_params_check.

- Added CONFIG_HMP checks after rebase on master

CI run: https://gitlab.com/farosas/qemu/-/pipelines/2834127646

---
v1:
https://lore.kernel.org/r/20260902221547.1812481-1-farosas@suse.de

Hi, this contains the pending work from my previous two series to
reduce duplication in the migration parameters handling and unify
migration parameters and capabilities.

Supersedes these series:
- [PATCH v3 00/51] migration: Unify capabilities and parameters
https://lore.kernel.org/r/20251215220041.12657-1-farosas@suse.de

- [PATCH v2 0/9] qapi: Use visitors for migration parameters handling
https://lore.kernel.org/r/20260202224101.20568-1-farosas@suse.de

I held on to the "pass full config to migration commands" changes for
now, let's put this^ scaffolding in first.

Changes from previous versions:

1) hacky qapi dealloc visitor replaced with a simple merge using
QDict.

  Suggested as one of the alternatives by Markus in:
  https://lore.kernel.org/r/871pio3d3h.fsf@pond.sub.org

This^ made me realise we've been doing manual checking in a lot of
places where a QAPI->QDict serialization would do the work
automatically, so:

2) New qtest to validate migration HMP commands.

  Good to have in general, but also this series touches that code
  heavily.

3) Rewrite of hmp_migrate_set_parameters, hmp_info_migrate_parameters
and hmp_migrate_set_parameter_completion.

  I want to remove all manual handling of migration parameters. These
  commands were the last heavy users of the "if params->has_foo,
  handle params->foo" pattern. We lose some user-friendliness here,
  but hopefully it's ok.

4) Remove MigrationParameter (singular).

  With this, migration.json now has only one place to define and
  document migration parameters, the MigrationParameters (plural).

5) New routine to validate has_* fields.

  Due to the above change, we lose MIGRATION_PARAMETER__MAX. Add a
  routine that ensures s->parameters has all has_ fields set to
  true. This is required for all the QAPI cloning and merging, etc.

CI run: https://gitlab.com/farosas/qemu/-/pipelines/2814719051

Fabiano Rosas (18):
  checkpatch: Fix checking of newlines in error messages
  migration/options.c: Don't export migrate_tls_opts_free
  migration: Rename variables in qmp_migrate_set_parameters
  migration: Use QAPI_CLONE_MEMBERS in migrate_params_apply
  migration: Merge parameter structs instead of assigning one by one
  migration: Open code migrate_params_apply
  migration: Stop freeing s->parameters members individually
  migration: Use migrate_params_free during finalize
  tests/qtest/migration: Add a test for HMP
  tests/qtest/migration: Add a test for HMP completion
  migration: HMP: Fix bandwidth parameters
  migration: Change HMP 'info migrate_parameters' output
  migration: Use keyval input visitor in HMP set command
  migration: Use output visitor in info command
  migration: Rewrite migrate_set_parameter_completion using QDict
  migration: Add capabilities into MigrationParameters
  migration: Remove s->capabilities
  qapi/migration: Deprecate capabilities commands

 docs/about/deprecated.rst          |  13 +
 migration/migration-hmp-cmds.c     | 457 ++++++------------
 migration/migration.c              |  17 +-
 migration/migration.h              |   2 +-
 migration/options.c                | 732 +++++++++++------------------
 migration/options.h                |  27 +-
 migration/savevm.c                 |   8 +-
 qapi/migration.json                | 140 +++++-
 scripts/checkpatch.pl              |  11 +-
 tests/qemu-iotests/300             |  20 +-
 tests/qtest/migration/misc-tests.c | 534 +++++++++++++++++++++
 11 files changed, 1155 insertions(+), 806 deletions(-)

-- 
2.53.0

[PATCH v2 01/18] checkpatch: Fix checking of newlines in error messages

[Fabiano Rosas]

Using newlines in error messages is not allowed as per qapi/error.h
documentation.

Checkpatch has been enforcing this for the error.h API, but also for
g_test_message() which doesn't fall under the qapi/error.h
prescription. Using newlines in that function's format string is fine
and can help with readability of test code when formatting test
results. Note that g_test_message() automatically adds the '#'
required by the TAP protocol to the start of each line.

Relax the regex for this function, but still forbid a trailing newline
because it's added automatically and usually not what the user wants.

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 scripts/checkpatch.pl | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 03f35e7501..8fd38868c5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3303,13 +3303,20 @@ sub process {
 					 info_vreport|
 					 error_report|
 					 warn_report|
-					 info_report|
-					 g_test_message}x;
+					 info_report}x;
 
 		if ($rawline =~ /\b(?:$qemu_error_funcs)\s*\(.*\".*\\n/) {
 			ERROR("Error messages should not contain newlines\n" . $herecurr);
 		}
 
+		# No newlines at the end
+		my $trail_newline_error_funcs = qr{g_test_message}x;
+
+		if ($rawline =~ /\b(?:$trail_newline_error_funcs)\s*\(.*\".*\\n\"/) {
+		    ERROR("Error messages should not contain trailing " .
+			  "newlines\n" . $herecurr);
+		}
+
 		# Continue checking for error messages that contains newlines.
 		# This check handles cases where string literals are spread
 		# over multiple lines.
-- 
2.53.0

[PATCH v2 02/18] migration/options.c: Don't export migrate_tls_opts_free

[Fabiano Rosas]

The migrate_tls_opts_free function was never used outside
options.c. Make it static.

Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/options.c | 2 +-
 migration/options.h | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/migration/options.c b/migration/options.c
index d2575edb6c..79a61ac60d 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -1055,7 +1055,7 @@ AnnounceParameters *migrate_announce_params(void)
     return &ap;
 }
 
-void migrate_tls_opts_free(MigrationParameters *params)
+static void migrate_tls_opts_free(MigrationParameters *params)
 {
     qapi_free_StrOrNull(params->tls_creds);
     qapi_free_StrOrNull(params->tls_hostname);
diff --git a/migration/options.h b/migration/options.h
index b46221998a..c272eb6208 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -93,5 +93,4 @@ uint64_t migrate_rdma_chunk_size(void);
 
 bool migrate_params_check(MigrationParameters *params, Error **errp);
 void migrate_params_init(MigrationParameters *params);
-void migrate_tls_opts_free(MigrationParameters *params);
 #endif
-- 
2.53.0

[PATCH v2 03/18] migration: Rename variables in qmp_migrate_set_parameters

[Fabiano Rosas]

Give the variables in qmp_migrate_set_parameters more semantic
names.

s/params/input/
this is the user input from qapi

s/tmp/new/
this is the combination of the current parameters and the input

Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/options.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/migration/options.c b/migration/options.c
index 79a61ac60d..388cb07dd0 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -1583,9 +1583,9 @@ static void migrate_params_apply(MigrationParameters *params)
     }
 }
 
-void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
+void qmp_migrate_set_parameters(MigrationParameters *input, Error **errp)
 {
-    MigrationParameters tmp;
+    MigrationParameters new;
 
     /*
      * Convert QTYPE_QNULL and NULL to the empty string (""). Even
@@ -1595,18 +1595,18 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
      * the options to the rest of the migration code already use
      * return NULL when the empty string is found.
      */
-    tls_opt_to_str(params->tls_creds);
-    tls_opt_to_str(params->tls_hostname);
-    tls_opt_to_str(params->tls_authz);
+    tls_opt_to_str(input->tls_creds);
+    tls_opt_to_str(input->tls_hostname);
+    tls_opt_to_str(input->tls_authz);
 
-    migrate_params_test_apply(params, &tmp);
+    migrate_params_test_apply(input, &new);
 
-    if (migrate_params_check(&tmp, errp)) {
-        migrate_params_apply(params);
-        migrate_post_update_params(params, errp);
+    if (migrate_params_check(&new, errp)) {
+        migrate_params_apply(input);
+        migrate_post_update_params(input, errp);
     }
 
-    migrate_tls_opts_free(&tmp);
-    qapi_free_BitmapMigrationNodeAliasList(tmp.block_bitmap_mapping);
-    qapi_free_strList(tmp.cpr_exec_command);
+    migrate_tls_opts_free(&new);
+    qapi_free_BitmapMigrationNodeAliasList(new.block_bitmap_mapping);
+    qapi_free_strList(new.cpr_exec_command);
 }
-- 
2.53.0

[PATCH v2 04/18] migration: Use QAPI_CLONE_MEMBERS in migrate_params_apply

[Fabiano Rosas]

Instead of setting parameters one by one, use the temporary object,
which already contains the current migration parameters plus the new
ones and was just validated by migration_params_check(). Use cloning
to overwrite it.

This avoids the need to alter this function every time a new parameter
is added.

Since parameters are not individually checked anymore, the setting of
s->has_block_bitmap_mapping moves into migrate_post_update_params().

Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/options.c | 141 +++++---------------------------------------
 1 file changed, 14 insertions(+), 127 deletions(-)

diff --git a/migration/options.c b/migration/options.c
index 388cb07dd0..6b78795080 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -1160,6 +1160,10 @@ static void migrate_post_update_params(MigrationParameters *new, Error **errp)
             migration_rate_set(new->max_postcopy_bandwidth);
         }
     }
+
+    if (new->has_block_bitmap_mapping) {
+        s->has_block_bitmap_mapping = true;
+    }
 }
 
 /*
@@ -1451,136 +1455,19 @@ static void migrate_params_test_apply(MigrationParameters *params,
     }
 }
 
+/*
+ * Caller must ensure the has_* fields of @params are true so they all
+ * get copied and the pointer members don't dangle.
+ */
 static void migrate_params_apply(MigrationParameters *params)
 {
     MigrationState *s = migrate_get_current();
+    MigrationParameters *cur = &s->parameters;
 
-    /* TODO use QAPI_CLONE() instead of duplicating it inline */
-
-    if (params->has_throttle_trigger_threshold) {
-        s->parameters.throttle_trigger_threshold = params->throttle_trigger_threshold;
-    }
-
-    if (params->has_cpu_throttle_initial) {
-        s->parameters.cpu_throttle_initial = params->cpu_throttle_initial;
-    }
-
-    if (params->has_cpu_throttle_increment) {
-        s->parameters.cpu_throttle_increment = params->cpu_throttle_increment;
-    }
-
-    if (params->has_cpu_throttle_tailslow) {
-        s->parameters.cpu_throttle_tailslow = params->cpu_throttle_tailslow;
-    }
-
-    if (params->tls_creds) {
-        qapi_free_StrOrNull(s->parameters.tls_creds);
-        s->parameters.tls_creds = QAPI_CLONE(StrOrNull, params->tls_creds);
-    }
-
-    if (params->tls_hostname) {
-        qapi_free_StrOrNull(s->parameters.tls_hostname);
-        s->parameters.tls_hostname = QAPI_CLONE(StrOrNull,
-                                                params->tls_hostname);
-    }
-
-    if (params->tls_authz) {
-        qapi_free_StrOrNull(s->parameters.tls_authz);
-        s->parameters.tls_authz = QAPI_CLONE(StrOrNull, params->tls_authz);
-    }
-
-    if (params->has_max_bandwidth) {
-        s->parameters.max_bandwidth = params->max_bandwidth;
-    }
-
-    if (params->has_avail_switchover_bandwidth) {
-        s->parameters.avail_switchover_bandwidth = params->avail_switchover_bandwidth;
-    }
-
-    if (params->has_downtime_limit) {
-        s->parameters.downtime_limit = params->downtime_limit;
-    }
-
-    if (params->has_x_checkpoint_delay) {
-        s->parameters.x_checkpoint_delay = params->x_checkpoint_delay;
-    }
-
-    if (params->has_multifd_channels) {
-        s->parameters.multifd_channels = params->multifd_channels;
-    }
-    if (params->has_multifd_compression) {
-        s->parameters.multifd_compression = params->multifd_compression;
-    }
-    if (params->has_multifd_qatzip_level) {
-        s->parameters.multifd_qatzip_level = params->multifd_qatzip_level;
-    }
-    if (params->has_multifd_zlib_level) {
-        s->parameters.multifd_zlib_level = params->multifd_zlib_level;
-    }
-    if (params->has_multifd_zstd_level) {
-        s->parameters.multifd_zstd_level = params->multifd_zstd_level;
-    }
-    if (params->has_xbzrle_cache_size) {
-        s->parameters.xbzrle_cache_size = params->xbzrle_cache_size;
-    }
-    if (params->has_max_postcopy_bandwidth) {
-        s->parameters.max_postcopy_bandwidth = params->max_postcopy_bandwidth;
-    }
-    if (params->has_max_cpu_throttle) {
-        s->parameters.max_cpu_throttle = params->max_cpu_throttle;
-    }
-    if (params->has_announce_initial) {
-        s->parameters.announce_initial = params->announce_initial;
-    }
-    if (params->has_announce_max) {
-        s->parameters.announce_max = params->announce_max;
-    }
-    if (params->has_announce_rounds) {
-        s->parameters.announce_rounds = params->announce_rounds;
-    }
-    if (params->has_announce_step) {
-        s->parameters.announce_step = params->announce_step;
-    }
-
-    if (params->has_block_bitmap_mapping) {
-        qapi_free_BitmapMigrationNodeAliasList(
-            s->parameters.block_bitmap_mapping);
-
-        s->has_block_bitmap_mapping = true;
-        s->parameters.block_bitmap_mapping =
-            QAPI_CLONE(BitmapMigrationNodeAliasList,
-                       params->block_bitmap_mapping);
-    }
-
-    if (params->has_x_vcpu_dirty_limit_period) {
-        s->parameters.x_vcpu_dirty_limit_period =
-            params->x_vcpu_dirty_limit_period;
-    }
-    if (params->has_vcpu_dirty_limit) {
-        s->parameters.vcpu_dirty_limit = params->vcpu_dirty_limit;
-    }
-
-    if (params->has_mode) {
-        s->parameters.mode = params->mode;
-    }
-
-    if (params->has_zero_page_detection) {
-        s->parameters.zero_page_detection = params->zero_page_detection;
-    }
-
-    if (params->has_direct_io) {
-        s->parameters.direct_io = params->direct_io;
-    }
-
-    if (params->has_x_rdma_chunk_size) {
-        s->parameters.x_rdma_chunk_size = params->x_rdma_chunk_size;
-    }
-
-    if (params->has_cpr_exec_command) {
-        qapi_free_strList(s->parameters.cpr_exec_command);
-        s->parameters.cpr_exec_command =
-            QAPI_CLONE(strList, params->cpr_exec_command);
-    }
+    migrate_tls_opts_free(cur);
+    qapi_free_BitmapMigrationNodeAliasList(cur->block_bitmap_mapping);
+    qapi_free_strList(cur->cpr_exec_command);
+    QAPI_CLONE_MEMBERS(MigrationParameters, cur, params);
 }
 
 void qmp_migrate_set_parameters(MigrationParameters *input, Error **errp)
@@ -1602,7 +1489,7 @@ void qmp_migrate_set_parameters(MigrationParameters *input, Error **errp)
     migrate_params_test_apply(input, &new);
 
     if (migrate_params_check(&new, errp)) {
-        migrate_params_apply(input);
+        migrate_params_apply(&new);
         migrate_post_update_params(input, errp);
     }
 
-- 
2.53.0

[PATCH v2 05/18] migration: Merge parameter structs instead of assigning one by one

[Fabiano Rosas]

Convert the code in migrate_params_test_apply() from an open-coded
copy of every migration parameter to a merge operation using QAPI
visitors and QDict.

The purpose of that routine is to update a temporary structure
(pre-populated with the current migration parameters), with the values
received from the user via QAPI. As a result, the temporary structure
will then contain the "to be applied" parameters and it's validated
before being used to overwrite the parameters currently in use.

The update is currently done as follows:

where 'params' is the user input from QAPI,
for each parameter:

  a) check if the option is present
     params->has_<name> == true
     params-><name> != NULL // for strings

  b) if the parameter is a pointer, free the to-be-assigned member and
     allocate memory for the copy from params

  c) assign the user provided value to the temporary structure.

Step (a) is the same in principle as what the QAPI visitors do at
visit_type_MigrationParameters_members().

Steps (b) and (c) are roughly the same as what the QDict
implementation does when qdict_del() and qdict_put_obj() are combined.

Therefore, replace the open-coded function with
migrate_params_merge(), which achieves the same goal, but uses
visitors and QDict. This hides the details of QAPI (has_*) from the
migration code and avoids the need to update
migrate_params_test_apply() every time a new migration parameter is
added.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/options.c | 200 ++++++++++++++------------------------------
 1 file changed, 65 insertions(+), 135 deletions(-)

diff --git a/migration/options.c b/migration/options.c
index 6b78795080..143710815a 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -20,6 +20,9 @@
 #include "qapi/qapi-commands-migration.h"
 #include "qapi/qapi-visit-migration.h"
 #include "qapi/qmp/qerror.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qobject-output-visitor.h"
+#include "qobject/qdict.h"
 #include "qobject/qnull.h"
 #include "system/runstate.h"
 #include "migration/colo.h"
@@ -1074,6 +1077,28 @@ static void tls_opt_to_str(StrOrNull *opt)
     opt->u.s = g_strdup("");
 }
 
+static QDict *migrate_params_to_dict(MigrationParameters *p, Error **errp)
+{
+    QObject *obj = NULL;
+    Visitor *v = qobject_output_visitor_new(&obj);
+
+    if (visit_type_MigrationParameters(v, NULL, &p, errp)) {
+        visit_complete(v, &obj);
+    }
+    visit_free(v);
+    return qobject_to(QDict, obj);
+}
+
+static MigrationParameters *migrate_params_from_dict(QDict *d, Error **errp)
+{
+    Visitor *v = qobject_input_visitor_new(QOBJECT(d));
+    MigrationParameters *tmp = NULL;
+
+    visit_type_MigrationParameters(v, NULL, &tmp, errp);
+    visit_free(v);
+    return tmp;
+}
+
 /*
  * query-migrate-parameters expects all members of MigrationParameters
  * to be present, but we cannot mark them non-optional in QAPI because
@@ -1166,6 +1191,38 @@ static void migrate_post_update_params(MigrationParameters *new, Error **errp)
     }
 }
 
+static bool migrate_params_merge(MigrationParameters *base,
+                                 MigrationParameters *updates,
+                                 MigrationParameters **new,
+                                 Error **errp)
+{
+    g_autoptr(QDict) d_base = NULL;
+    g_autoptr(QDict) d_upd = NULL;
+    const QDictEntry *e;
+
+    d_base = migrate_params_to_dict(base, errp);
+    if (!d_base) {
+        return false;
+    }
+
+    d_upd = migrate_params_to_dict(updates, errp);
+    if (!d_upd) {
+        return false;
+    }
+
+    for (e = qdict_first(d_upd); e; e = qdict_next(d_upd, e)) {
+        const char *key = qdict_entry_key(e);
+        QObject *value = qdict_entry_value(e);
+
+        qobject_ref(value);
+        qdict_put_obj(d_base, key, value);
+    }
+
+    *new = migrate_params_from_dict(d_base, errp);
+
+    return !!*new;
+}
+
 /*
  * Check whether the parameters are valid. Error will be put into errp
  * (if provided). Return true if valid, otherwise false.
@@ -1328,133 +1385,6 @@ bool migrate_params_check(MigrationParameters *params, Error **errp)
     return true;
 }
 
-static void migrate_params_test_apply(MigrationParameters *params,
-                                      MigrationParameters *dest)
-{
-    MigrationState *s = migrate_get_current();
-
-    QAPI_CLONE_MEMBERS(MigrationParameters, dest, &s->parameters);
-
-    if (params->has_throttle_trigger_threshold) {
-        dest->throttle_trigger_threshold = params->throttle_trigger_threshold;
-    }
-
-    if (params->has_cpu_throttle_initial) {
-        dest->cpu_throttle_initial = params->cpu_throttle_initial;
-    }
-
-    if (params->has_cpu_throttle_increment) {
-        dest->cpu_throttle_increment = params->cpu_throttle_increment;
-    }
-
-    if (params->has_cpu_throttle_tailslow) {
-        dest->cpu_throttle_tailslow = params->cpu_throttle_tailslow;
-    }
-
-    if (params->tls_creds) {
-        qapi_free_StrOrNull(dest->tls_creds);
-        dest->tls_creds = QAPI_CLONE(StrOrNull, params->tls_creds);
-    }
-
-    if (params->tls_hostname) {
-        qapi_free_StrOrNull(dest->tls_hostname);
-        dest->tls_hostname = QAPI_CLONE(StrOrNull, params->tls_hostname);
-    }
-
-    if (params->tls_authz) {
-        qapi_free_StrOrNull(dest->tls_authz);
-        dest->tls_authz = QAPI_CLONE(StrOrNull, params->tls_authz);
-    }
-
-    if (params->has_max_bandwidth) {
-        dest->max_bandwidth = params->max_bandwidth;
-    }
-
-    if (params->has_avail_switchover_bandwidth) {
-        dest->avail_switchover_bandwidth = params->avail_switchover_bandwidth;
-    }
-
-    if (params->has_downtime_limit) {
-        dest->downtime_limit = params->downtime_limit;
-    }
-
-    if (params->has_x_checkpoint_delay) {
-        dest->x_checkpoint_delay = params->x_checkpoint_delay;
-    }
-
-    if (params->has_multifd_channels) {
-        dest->multifd_channels = params->multifd_channels;
-    }
-    if (params->has_multifd_compression) {
-        dest->multifd_compression = params->multifd_compression;
-    }
-    if (params->has_multifd_qatzip_level) {
-        dest->multifd_qatzip_level = params->multifd_qatzip_level;
-    }
-    if (params->has_multifd_zlib_level) {
-        dest->multifd_zlib_level = params->multifd_zlib_level;
-    }
-    if (params->has_multifd_zstd_level) {
-        dest->multifd_zstd_level = params->multifd_zstd_level;
-    }
-    if (params->has_xbzrle_cache_size) {
-        dest->xbzrle_cache_size = params->xbzrle_cache_size;
-    }
-    if (params->has_max_postcopy_bandwidth) {
-        dest->max_postcopy_bandwidth = params->max_postcopy_bandwidth;
-    }
-    if (params->has_max_cpu_throttle) {
-        dest->max_cpu_throttle = params->max_cpu_throttle;
-    }
-    if (params->has_announce_initial) {
-        dest->announce_initial = params->announce_initial;
-    }
-    if (params->has_announce_max) {
-        dest->announce_max = params->announce_max;
-    }
-    if (params->has_announce_rounds) {
-        dest->announce_rounds = params->announce_rounds;
-    }
-    if (params->has_announce_step) {
-        dest->announce_step = params->announce_step;
-    }
-
-    if (params->has_block_bitmap_mapping) {
-        qapi_free_BitmapMigrationNodeAliasList(dest->block_bitmap_mapping);
-        dest->block_bitmap_mapping = QAPI_CLONE(BitmapMigrationNodeAliasList,
-                                                params->block_bitmap_mapping);
-    }
-
-    if (params->has_x_vcpu_dirty_limit_period) {
-        dest->x_vcpu_dirty_limit_period =
-            params->x_vcpu_dirty_limit_period;
-    }
-    if (params->has_vcpu_dirty_limit) {
-        dest->vcpu_dirty_limit = params->vcpu_dirty_limit;
-    }
-
-    if (params->has_mode) {
-        dest->mode = params->mode;
-    }
-
-    if (params->has_zero_page_detection) {
-        dest->zero_page_detection = params->zero_page_detection;
-    }
-
-    if (params->has_direct_io) {
-        dest->direct_io = params->direct_io;
-    }
-
-    if (params->has_x_rdma_chunk_size) {
-        dest->x_rdma_chunk_size = params->x_rdma_chunk_size;
-    }
-
-    if (params->has_cpr_exec_command) {
-        qapi_free_strList(dest->cpr_exec_command);
-        dest->cpr_exec_command = QAPI_CLONE(strList, params->cpr_exec_command);
-    }
-}
-
 /*
  * Caller must ensure the has_* fields of @params are true so they all
  * get copied and the pointer members don't dangle.
@@ -1472,7 +1402,8 @@ static void migrate_params_apply(MigrationParameters *params)
 
 void qmp_migrate_set_parameters(MigrationParameters *input, Error **errp)
 {
-    MigrationParameters new;
+    MigrationParameters *cur = &migrate_get_current()->parameters;
+    g_autoptr(MigrationParameters) new = NULL;
 
     /*
      * Convert QTYPE_QNULL and NULL to the empty string (""). Even
@@ -1486,14 +1417,13 @@ void qmp_migrate_set_parameters(MigrationParameters *input, Error **errp)
     tls_opt_to_str(input->tls_hostname);
     tls_opt_to_str(input->tls_authz);
 
-    migrate_params_test_apply(input, &new);
+    /* merge input on top of current */
+    if (!migrate_params_merge(cur, input, &new, errp)) {
+        return;
+    }
 
-    if (migrate_params_check(&new, errp)) {
-        migrate_params_apply(&new);
+    if (migrate_params_check(new, errp)) {
+        migrate_params_apply(new);
         migrate_post_update_params(input, errp);
     }
-
-    migrate_tls_opts_free(&new);
-    qapi_free_BitmapMigrationNodeAliasList(new.block_bitmap_mapping);
-    qapi_free_strList(new.cpr_exec_command);
 }
-- 
2.53.0

Re: [PATCH v2 05/18] migration: Merge parameter structs instead of assigning one by one

[Peter Xu]

On Wed, Sep 09, 2026 at 06:44:55PM -0300, Fabiano Rosas wrote:
> Convert the code in migrate_params_test_apply() from an open-coded
> copy of every migration parameter to a merge operation using QAPI
> visitors and QDict.
> 
> The purpose of that routine is to update a temporary structure
> (pre-populated with the current migration parameters), with the values
> received from the user via QAPI. As a result, the temporary structure
> will then contain the "to be applied" parameters and it's validated
> before being used to overwrite the parameters currently in use.
> 
> The update is currently done as follows:
> 
> where 'params' is the user input from QAPI,
> for each parameter:
> 
>   a) check if the option is present
>      params->has_<name> == true
>      params-><name> != NULL // for strings
> 
>   b) if the parameter is a pointer, free the to-be-assigned member and
>      allocate memory for the copy from params
> 
>   c) assign the user provided value to the temporary structure.
> 
> Step (a) is the same in principle as what the QAPI visitors do at
> visit_type_MigrationParameters_members().
> 
> Steps (b) and (c) are roughly the same as what the QDict
> implementation does when qdict_del() and qdict_put_obj() are combined.
> 
> Therefore, replace the open-coded function with
> migrate_params_merge(), which achieves the same goal, but uses
> visitors and QDict. This hides the details of QAPI (has_*) from the
> migration code and avoids the need to update
> migrate_params_test_apply() every time a new migration parameter is
> added.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

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

-- 
Peter Xu

[PATCH v2 06/18] migration: Open code migrate_params_apply

[Fabiano Rosas]

Remove migrate_params_apply so the logic of setting migration
parameters is all in one spot.

Suggested-by: Prasad Pandit <ppandit@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/options.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/migration/options.c b/migration/options.c
index 143710815a..24799eba96 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -1385,21 +1385,6 @@ bool migrate_params_check(MigrationParameters *params, Error **errp)
     return true;
 }
 
-/*
- * Caller must ensure the has_* fields of @params are true so they all
- * get copied and the pointer members don't dangle.
- */
-static void migrate_params_apply(MigrationParameters *params)
-{
-    MigrationState *s = migrate_get_current();
-    MigrationParameters *cur = &s->parameters;
-
-    migrate_tls_opts_free(cur);
-    qapi_free_BitmapMigrationNodeAliasList(cur->block_bitmap_mapping);
-    qapi_free_strList(cur->cpr_exec_command);
-    QAPI_CLONE_MEMBERS(MigrationParameters, cur, params);
-}
-
 void qmp_migrate_set_parameters(MigrationParameters *input, Error **errp)
 {
     MigrationParameters *cur = &migrate_get_current()->parameters;
@@ -1422,8 +1407,15 @@ void qmp_migrate_set_parameters(MigrationParameters *input, Error **errp)
         return;
     }
 
-    if (migrate_params_check(new, errp)) {
-        migrate_params_apply(new);
-        migrate_post_update_params(input, errp);
+    if (!migrate_params_check(new, errp)) {
+        return;
     }
+
+    migrate_tls_opts_free(cur);
+    qapi_free_BitmapMigrationNodeAliasList(cur->block_bitmap_mapping);
+    qapi_free_strList(cur->cpr_exec_command);
+
+    QAPI_CLONE_MEMBERS(MigrationParameters, cur, new);
+
+    migrate_post_update_params(input, errp);
 }
-- 
2.53.0

[PATCH v2 07/18] migration: Stop freeing s->parameters members individually

[Fabiano Rosas]

Expecting every pointer member from s->parameters to be freed
individually is prone to leave some fields forgotten when the code is
eventually updated. Use a dealloc visitor to free them all at once.

Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/options.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/migration/options.c b/migration/options.c
index 24799eba96..f0774fc50b 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -16,6 +16,7 @@
 #include "qemu/units.h"
 #include "exec/target_page.h"
 #include "qapi/clone-visitor.h"
+#include "qapi/dealloc-visitor.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-migration.h"
 #include "qapi/qapi-visit-migration.h"
@@ -1058,11 +1059,21 @@ AnnounceParameters *migrate_announce_params(void)
     return &ap;
 }
 
-static void migrate_tls_opts_free(MigrationParameters *params)
+static bool migrate_params_free(MigrationParameters *params, Error **errp)
 {
-    qapi_free_StrOrNull(params->tls_creds);
-    qapi_free_StrOrNull(params->tls_hostname);
-    qapi_free_StrOrNull(params->tls_authz);
+    Visitor *v = qapi_dealloc_visitor_new();
+    bool ret;
+
+    /*
+     * qapi_free_MigrationParameters can't be used here because
+     * MigrationParameters is embedded in MigrationState due to qdev
+     * needing to access the offset of the migration properties inside
+     * the migration object.
+     */
+    ret = visit_type_MigrationParameters_members(v, params, errp);
+    visit_free(v);
+
+    return ret;
 }
 
 /* normalize QTYPE_QNULL to QTYPE_QSTRING "" */
@@ -1411,9 +1422,9 @@ void qmp_migrate_set_parameters(MigrationParameters *input, Error **errp)
         return;
     }
 
-    migrate_tls_opts_free(cur);
-    qapi_free_BitmapMigrationNodeAliasList(cur->block_bitmap_mapping);
-    qapi_free_strList(cur->cpr_exec_command);
+    if (!migrate_params_free(cur, errp)) {
+        return;
+    }
 
     QAPI_CLONE_MEMBERS(MigrationParameters, cur, new);
 
-- 
2.53.0

[PATCH v2 08/18] migration: Use migrate_params_free during finalize

[Fabiano Rosas]

Use the recently introduced migrate_params_free routine at
migration_instance_finalize() so that newly added pointers are already
freed by default.

Special case: The TLS options are currently the only pointers that
also have a qdev property implementation, so they will be freed by
qdev using the .release method. Update the method so that a second
invocation of qapi_free_StrOrNull doesn't assert.

Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c | 3 +--
 migration/options.c   | 7 ++++---
 migration/options.h   | 1 +
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index b413d28622..dab282dd2f 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -4039,8 +4039,7 @@ static void migration_instance_finalize(Object *obj)
 {
     MigrationState *ms = MIGRATION(obj);
 
-    qapi_free_BitmapMigrationNodeAliasList(ms->parameters.block_bitmap_mapping);
-    qapi_free_strList(ms->parameters.cpr_exec_command);
+    migrate_params_free(&ms->parameters, NULL);
     qemu_mutex_destroy(&ms->error_mutex);
     qemu_mutex_destroy(&ms->qemu_file_lock);
     qemu_sem_destroy(&ms->wait_unplug_sem);
diff --git a/migration/options.c b/migration/options.c
index f0774fc50b..990fcd6867 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -271,8 +271,9 @@ static void set_StrOrNull(Object *obj, Visitor *v, const char *name,
 
 static void release_StrOrNull(Object *obj, const char *name, void *opaque)
 {
-    const Property *prop = opaque;
-    qapi_free_StrOrNull(*(StrOrNull **)object_field_prop_ptr(obj, prop));
+    StrOrNull **ptr = object_field_prop_ptr(obj, opaque);
+
+    g_clear_pointer(ptr, qapi_free_StrOrNull);
 }
 
 static void set_default_value_tls_opt(ObjectProperty *op, const Property *prop)
@@ -1059,7 +1060,7 @@ AnnounceParameters *migrate_announce_params(void)
     return &ap;
 }
 
-static bool migrate_params_free(MigrationParameters *params, Error **errp)
+bool migrate_params_free(MigrationParameters *params, Error **errp)
 {
     Visitor *v = qapi_dealloc_visitor_new();
     bool ret;
diff --git a/migration/options.h b/migration/options.h
index c272eb6208..c7da2d0b5b 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -93,4 +93,5 @@ uint64_t migrate_rdma_chunk_size(void);
 
 bool migrate_params_check(MigrationParameters *params, Error **errp);
 void migrate_params_init(MigrationParameters *params);
+bool migrate_params_free(MigrationParameters *params, Error **errp);
 #endif
-- 
2.53.0

[PATCH v2 09/18] tests/qtest/migration: Add a test for HMP

[Fabiano Rosas]

The following patches will change how parameters are set and shown in
HMP, add a test for it.

The test issues HMP migrate_set_parameters for each of the existing
migration parameters and queries them back with the info command. A
list is kept with the expected strings. A substring match function
inspired by glib's g_str_match_string is implemented for this test so
the test can produce a decent error output instead of just assert
failure. E.g:

 # HMP output mismatch for entry at line 55:
 # expected vs. found:
 #
 # max-bandwidth: 10356305952768 bytes/hour
 # ---
 # max-bandwidth: 10356305952768 bytes/second

(note that line 55 above is the source line where the test case for
max-bandwith is, which helps find the failing test in the list)

Usage:
QTEST_QEMU_BINARY=./qemu-system-x86_64 \
./tests/qtest/migration-test --full -p /x86_64/migration/hmp/parameters

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration/misc-tests.c | 201 +++++++++++++++++++++++++++++
 1 file changed, 201 insertions(+)

diff --git a/tests/qtest/migration/misc-tests.c b/tests/qtest/migration/misc-tests.c
index 4e0deb7f18..533cf3d55d 100644
--- a/tests/qtest/migration/misc-tests.c
+++ b/tests/qtest/migration/misc-tests.c
@@ -22,6 +22,203 @@
 
 static char *tmpfs;
 
+#ifdef CONFIG_HMP
+static int test_case_line;
+
+#define TEST(i1, i2, e) { i1, i2, e , .line = __LINE__, }
+#define SKIP(i1, i2, e) { i1, i2, e , .skip = true, }
+#define BG_SNAP_MSG ("Error: Background-snapshot is not compatible with " \
+                     "currently set capabilities")
+
+typedef struct HMPTestData {
+    const char *input1;
+    const char *input2;
+    const char *output1;
+    bool skip;
+    int line;
+} HMPTestData;
+
+/*
+ * .input1: string to be used as parameter name
+ * .input2: string to be used as parameter value
+ * .output1: expected output of migrate_set_parameters
+ * E.g:
+ * (qemu) migrate_set_parameters .input1 .input2
+ * .output1
+ */
+HMPTestData test_cases[] = {
+    TEST("", "", "migrate_set_parameter: string expected"),
+    TEST("foo", "", "migrate_set_parameter: string expected"),
+    TEST("foo", "on", "Error: invalid parameter value: foo"),
+
+    /* bool */
+    TEST("cpu-throttle-tailslow", "on", "on"),
+    TEST("direct-io", "on", "on"),
+
+    /* uint64_t */
+    TEST("announce-initial", "60", "60 ms"),
+    TEST("announce-max", "600", "600 ms"),
+    TEST("announce-rounds", "6", "6"),
+    TEST("announce-step", "15", "15 ms"),
+    TEST("downtime-limit", "400", "400 ms"),
+    TEST("avail-switchover-bandwidth", "2097152", "2199023255552 bytes/second"),
+    TEST("max-bandwidth", "9876543", "10356305952768 bytes/second"),
+    TEST("max-postcopy-bandwidth", "1048576", "1048576 bytes/second"),
+    TEST("vcpu-dirty-limit", "20", "20 MB/s"),
+    TEST("x-rdma-chunk-size", "1048576", "1048576 bytes"),
+    TEST("x-vcpu-dirty-limit-period", "750", "750 ms"),
+    TEST("xbzrle-cache-size", "67108864", "67108864 bytes"),
+
+    /* uint32_t */
+    TEST("x-checkpoint-delay", "5000", "5000 ms"),
+
+    /* uint8_t */
+    TEST("cpu-throttle-increment", "15", "15"),
+    TEST("cpu-throttle-initial", "25", "25"),
+    TEST("max-cpu-throttle", "85", "85"),
+    TEST("multifd-channels", "8", "8"),
+    TEST("throttle-trigger-threshold", "65", "65"),
+
+    /* complex types */
+    TEST("mode", "cpr-exec", "cpr-exec"),
+    TEST("multifd-compression", "zlib", "zlib"),
+    TEST("zero-page-detection", "none", "none"),
+    TEST("tls-authz", "my_authz", "'my_authz'"),
+    TEST("tls-creds", "null", "'null'"),
+    TEST("tls-hostname", "localhost", "'localhost'"),
+    TEST("cpr-exec-command", "/bin/true foobar", "/bin/true foobar"),
+
+    /* can be set but are currently missing in the query output */
+    SKIP("multifd-qatzip-level", "5", "5"),
+    SKIP("multifd-zlib-level", "4", "4"),
+    SKIP("multifd-zstd-level", "6", "6"),
+
+    /* cannot be set */
+    TEST("block-bitmap-mapping", "[]",
+         "Error: The block-bitmap-mapping parameter "
+         "can only be set through QMP"),
+};
+
+/*
+ * Find a contiguous run of tokens in @larger that match the sequence
+ * of tokens in @smaller, ignoring mismatches due to sequences of
+ * empty tokens.
+ *
+ * Returns whether a match was found. @last is set if at least one
+ * token has matched.
+ */
+static bool token_list_is_substr(char **smaller, char **larger, int *last)
+{
+    int i, j, k = 0;
+    bool match = false;
+
+    for (i = 0; smaller[i]; i++) {
+        for (j = k; larger[j]; j++) {
+            if (!*larger[j]) {
+                continue;
+            }
+
+            /* readline adds several escape sequences */
+            if (*larger[j] == '\033') {
+                continue;
+            }
+
+            if (g_str_equal(larger[j], smaller[i])) {
+                match = true;
+                *last = j;
+                k = j + 1;
+                break;
+            }
+
+            if (match) {
+                return false;
+            } else {
+                match = false;
+            }
+        }
+    }
+
+    return match;
+}
+
+static void assert_hmp_match_line(const char *str, const char *text)
+{
+    g_auto(GStrv) tok_str = g_strsplit_set(str, " ", -1);
+    g_auto(GStrv) lines = g_strsplit_set(text, " \r\n", -1);
+    int i, idx = -1;
+
+    /*
+     * Note that the reason the 'str' above is split is to allow
+     * token_list_is_substr() to first match on the parameter name so
+     * matching can stop immediately after a mismatched value is
+     * found. This provides a better output for failing test cases
+     * than simply "str != line".
+     */
+
+    for (i = 0; lines[i]; i++) {
+        if (token_list_is_substr(tok_str, (char **)&lines[i], &idx)) {
+            return;
+        }
+
+        if (idx >= 0) {
+            break;
+        }
+    }
+
+    g_test_message("HMP output mismatch for entry at line %d:", test_case_line);
+    g_test_message("expected vs. found:\n\n%s\n---\n%s %s", str, lines[idx],
+                   lines[idx + 1]);
+    g_assert_not_reached();
+}
+
+static void assert_hmp_success(const char *str)
+{
+    if (!g_str_equal(str, "")) {
+        g_test_message("HMP command failed:\n\n%s", str);
+        g_assert_not_reached();
+    }
+}
+
+static void test_hmp_migration_parameters(char *name, MigrateCommon *args)
+{
+    QTestState *qts;
+
+    /* force TCG so it can run in all targets */
+    qts = qtest_init("-accel tcg -nodefaults -S");
+
+    for (int i = 0; i < G_N_ELEMENTS(test_cases); i++) {
+        g_autofree char *resp = NULL;
+        g_autofree char *line = NULL;
+        struct HMPTestData *t = &test_cases[i];
+
+        if (t->skip) {
+            continue;
+        }
+
+        test_case_line = t->line;
+
+        resp = qtest_hmp(qts, "migrate_set_parameter %s %s", t->input1,
+                         t->input2);
+
+        if (g_str_has_prefix(t->output1, "Error:") ||
+            g_str_has_prefix(resp, "migrate_set_parameter:")) {
+
+            assert_hmp_match_line(t->output1, resp);
+            continue;
+        }
+        assert_hmp_success(resp);
+        g_free(resp);
+
+        resp = qtest_hmp(qts, "info migrate_parameters");
+
+        line = g_strconcat(t->input1, ": ", t->output1, NULL);
+        assert_hmp_match_line(line, resp);
+    }
+
+    qtest_quit(qts);
+}
+#endif /* CONFIG_HMP */
+
 static void test_baddest(char *name, MigrateCommon *args)
 {
     QTestState *from, *to;
@@ -260,4 +457,8 @@ 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);
+#ifdef CONFIG_HMP
+    migration_test_add("/migration/hmp/parameters",
+                       test_hmp_migration_parameters);
+#endif
 }
-- 
2.53.0

Re: [PATCH v2 09/18] tests/qtest/migration: Add a test for HMP

[Peter Xu]

On Wed, Sep 09, 2026 at 06:44:59PM -0300, Fabiano Rosas wrote:
> The following patches will change how parameters are set and shown in
> HMP, add a test for it.
> 
> The test issues HMP migrate_set_parameters for each of the existing
> migration parameters and queries them back with the info command. A
> list is kept with the expected strings. A substring match function
> inspired by glib's g_str_match_string is implemented for this test so
> the test can produce a decent error output instead of just assert
> failure. E.g:
> 
>  # HMP output mismatch for entry at line 55:
>  # expected vs. found:
>  #
>  # max-bandwidth: 10356305952768 bytes/hour
>  # ---
>  # max-bandwidth: 10356305952768 bytes/second
> 
> (note that line 55 above is the source line where the test case for
> max-bandwith is, which helps find the failing test in the list)
> 
> Usage:
> QTEST_QEMU_BINARY=./qemu-system-x86_64 \
> ./tests/qtest/migration-test --full -p /x86_64/migration/hmp/parameters
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

I didn't read into details, but it's a nice good test case, let's ship it..

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

One thing to mention below, no need to change but maybe for future if that
makes sense..

> ---
>  tests/qtest/migration/misc-tests.c | 201 +++++++++++++++++++++++++++++
>  1 file changed, 201 insertions(+)
> 
> diff --git a/tests/qtest/migration/misc-tests.c b/tests/qtest/migration/misc-tests.c
> index 4e0deb7f18..533cf3d55d 100644
> --- a/tests/qtest/migration/misc-tests.c
> +++ b/tests/qtest/migration/misc-tests.c
> @@ -22,6 +22,203 @@
>  
>  static char *tmpfs;
>  
> +#ifdef CONFIG_HMP
> +static int test_case_line;
> +
> +#define TEST(i1, i2, e) { i1, i2, e , .line = __LINE__, }
> +#define SKIP(i1, i2, e) { i1, i2, e , .skip = true, }
> +#define BG_SNAP_MSG ("Error: Background-snapshot is not compatible with " \
> +                     "currently set capabilities")
> +
> +typedef struct HMPTestData {
> +    const char *input1;
> +    const char *input2;
> +    const char *output1;
> +    bool skip;
> +    int line;
> +} HMPTestData;
> +
> +/*
> + * .input1: string to be used as parameter name
> + * .input2: string to be used as parameter value
> + * .output1: expected output of migrate_set_parameters
> + * E.g:
> + * (qemu) migrate_set_parameters .input1 .input2
> + * .output1

This comment doesn't seem to be accurate, because the code seems to treat
success and failure cases differently based on the initial chars.

Since SKIP existed, we could also do SUCCESS() and FAIL() with another
bool, maybe.

-- 
Peter Xu

Re: [PATCH v2 09/18] tests/qtest/migration: Add a test for HMP

[Dr. David Alan Gilbert]

* Fabiano Rosas (farosas@suse.de) wrote:
> The following patches will change how parameters are set and shown in
> HMP, add a test for it.
> 
> The test issues HMP migrate_set_parameters for each of the existing
> migration parameters and queries them back with the info command. A
> list is kept with the expected strings. A substring match function
> inspired by glib's g_str_match_string is implemented for this test so
> the test can produce a decent error output instead of just assert
> failure. E.g:
> 
>  # HMP output mismatch for entry at line 55:
>  # expected vs. found:
>  #
>  # max-bandwidth: 10356305952768 bytes/hour
>  # ---
>  # max-bandwidth: 10356305952768 bytes/second
> 
> (note that line 55 above is the source line where the test case for
> max-bandwith is, which helps find the failing test in the list)
> 
> Usage:
> QTEST_QEMU_BINARY=./qemu-system-x86_64 \
> ./tests/qtest/migration-test --full -p /x86_64/migration/hmp/parameters
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Acked-by: Dr. David Alan Gilbert <dave@treblig.org>

Two minor thoughts:
  a) You might try a numeric out of range test?
  b) 'input1' and 'input2' seem odd names for something that oculd be .name and .value ?
    (But only if you need to rework anyway).

Dave

> ---
>  tests/qtest/migration/misc-tests.c | 201 +++++++++++++++++++++++++++++
>  1 file changed, 201 insertions(+)
> 
> diff --git a/tests/qtest/migration/misc-tests.c b/tests/qtest/migration/misc-tests.c
> index 4e0deb7f18..533cf3d55d 100644
> --- a/tests/qtest/migration/misc-tests.c
> +++ b/tests/qtest/migration/misc-tests.c
> @@ -22,6 +22,203 @@
>  
>  static char *tmpfs;
>  
> +#ifdef CONFIG_HMP
> +static int test_case_line;
> +
> +#define TEST(i1, i2, e) { i1, i2, e , .line = __LINE__, }
> +#define SKIP(i1, i2, e) { i1, i2, e , .skip = true, }
> +#define BG_SNAP_MSG ("Error: Background-snapshot is not compatible with " \
> +                     "currently set capabilities")
> +
> +typedef struct HMPTestData {
> +    const char *input1;
> +    const char *input2;
> +    const char *output1;
> +    bool skip;
> +    int line;
> +} HMPTestData;
> +
> +/*
> + * .input1: string to be used as parameter name
> + * .input2: string to be used as parameter value
> + * .output1: expected output of migrate_set_parameters
> + * E.g:
> + * (qemu) migrate_set_parameters .input1 .input2
> + * .output1
> + */
> +HMPTestData test_cases[] = {
> +    TEST("", "", "migrate_set_parameter: string expected"),
> +    TEST("foo", "", "migrate_set_parameter: string expected"),
> +    TEST("foo", "on", "Error: invalid parameter value: foo"),
> +
> +    /* bool */
> +    TEST("cpu-throttle-tailslow", "on", "on"),
> +    TEST("direct-io", "on", "on"),
> +
> +    /* uint64_t */
> +    TEST("announce-initial", "60", "60 ms"),
> +    TEST("announce-max", "600", "600 ms"),
> +    TEST("announce-rounds", "6", "6"),
> +    TEST("announce-step", "15", "15 ms"),
> +    TEST("downtime-limit", "400", "400 ms"),
> +    TEST("avail-switchover-bandwidth", "2097152", "2199023255552 bytes/second"),
> +    TEST("max-bandwidth", "9876543", "10356305952768 bytes/second"),
> +    TEST("max-postcopy-bandwidth", "1048576", "1048576 bytes/second"),
> +    TEST("vcpu-dirty-limit", "20", "20 MB/s"),
> +    TEST("x-rdma-chunk-size", "1048576", "1048576 bytes"),
> +    TEST("x-vcpu-dirty-limit-period", "750", "750 ms"),
> +    TEST("xbzrle-cache-size", "67108864", "67108864 bytes"),
> +
> +    /* uint32_t */
> +    TEST("x-checkpoint-delay", "5000", "5000 ms"),
> +
> +    /* uint8_t */
> +    TEST("cpu-throttle-increment", "15", "15"),
> +    TEST("cpu-throttle-initial", "25", "25"),
> +    TEST("max-cpu-throttle", "85", "85"),
> +    TEST("multifd-channels", "8", "8"),
> +    TEST("throttle-trigger-threshold", "65", "65"),
> +
> +    /* complex types */
> +    TEST("mode", "cpr-exec", "cpr-exec"),
> +    TEST("multifd-compression", "zlib", "zlib"),
> +    TEST("zero-page-detection", "none", "none"),
> +    TEST("tls-authz", "my_authz", "'my_authz'"),
> +    TEST("tls-creds", "null", "'null'"),
> +    TEST("tls-hostname", "localhost", "'localhost'"),
> +    TEST("cpr-exec-command", "/bin/true foobar", "/bin/true foobar"),
> +
> +    /* can be set but are currently missing in the query output */
> +    SKIP("multifd-qatzip-level", "5", "5"),
> +    SKIP("multifd-zlib-level", "4", "4"),
> +    SKIP("multifd-zstd-level", "6", "6"),
> +
> +    /* cannot be set */
> +    TEST("block-bitmap-mapping", "[]",
> +         "Error: The block-bitmap-mapping parameter "
> +         "can only be set through QMP"),
> +};
> +
> +/*
> + * Find a contiguous run of tokens in @larger that match the sequence
> + * of tokens in @smaller, ignoring mismatches due to sequences of
> + * empty tokens.
> + *
> + * Returns whether a match was found. @last is set if at least one
> + * token has matched.
> + */
> +static bool token_list_is_substr(char **smaller, char **larger, int *last)
> +{
> +    int i, j, k = 0;
> +    bool match = false;
> +
> +    for (i = 0; smaller[i]; i++) {
> +        for (j = k; larger[j]; j++) {
> +            if (!*larger[j]) {
> +                continue;
> +            }
> +
> +            /* readline adds several escape sequences */
> +            if (*larger[j] == '\033') {
> +                continue;
> +            }
> +
> +            if (g_str_equal(larger[j], smaller[i])) {
> +                match = true;
> +                *last = j;
> +                k = j + 1;
> +                break;
> +            }
> +
> +            if (match) {
> +                return false;
> +            } else {
> +                match = false;
> +            }
> +        }
> +    }
> +
> +    return match;
> +}
> +
> +static void assert_hmp_match_line(const char *str, const char *text)
> +{
> +    g_auto(GStrv) tok_str = g_strsplit_set(str, " ", -1);
> +    g_auto(GStrv) lines = g_strsplit_set(text, " \r\n", -1);
> +    int i, idx = -1;
> +
> +    /*
> +     * Note that the reason the 'str' above is split is to allow
> +     * token_list_is_substr() to first match on the parameter name so
> +     * matching can stop immediately after a mismatched value is
> +     * found. This provides a better output for failing test cases
> +     * than simply "str != line".
> +     */
> +
> +    for (i = 0; lines[i]; i++) {
> +        if (token_list_is_substr(tok_str, (char **)&lines[i], &idx)) {
> +            return;
> +        }
> +
> +        if (idx >= 0) {
> +            break;
> +        }
> +    }
> +
> +    g_test_message("HMP output mismatch for entry at line %d:", test_case_line);
> +    g_test_message("expected vs. found:\n\n%s\n---\n%s %s", str, lines[idx],
> +                   lines[idx + 1]);
> +    g_assert_not_reached();
> +}
> +
> +static void assert_hmp_success(const char *str)
> +{
> +    if (!g_str_equal(str, "")) {
> +        g_test_message("HMP command failed:\n\n%s", str);
> +        g_assert_not_reached();
> +    }
> +}
> +
> +static void test_hmp_migration_parameters(char *name, MigrateCommon *args)
> +{
> +    QTestState *qts;
> +
> +    /* force TCG so it can run in all targets */
> +    qts = qtest_init("-accel tcg -nodefaults -S");
> +
> +    for (int i = 0; i < G_N_ELEMENTS(test_cases); i++) {
> +        g_autofree char *resp = NULL;
> +        g_autofree char *line = NULL;
> +        struct HMPTestData *t = &test_cases[i];
> +
> +        if (t->skip) {
> +            continue;
> +        }
> +
> +        test_case_line = t->line;
> +
> +        resp = qtest_hmp(qts, "migrate_set_parameter %s %s", t->input1,
> +                         t->input2);
> +
> +        if (g_str_has_prefix(t->output1, "Error:") ||
> +            g_str_has_prefix(resp, "migrate_set_parameter:")) {
> +
> +            assert_hmp_match_line(t->output1, resp);
> +            continue;
> +        }
> +        assert_hmp_success(resp);
> +        g_free(resp);
> +
> +        resp = qtest_hmp(qts, "info migrate_parameters");
> +
> +        line = g_strconcat(t->input1, ": ", t->output1, NULL);
> +        assert_hmp_match_line(line, resp);
> +    }
> +
> +    qtest_quit(qts);
> +}
> +#endif /* CONFIG_HMP */
> +
>  static void test_baddest(char *name, MigrateCommon *args)
>  {
>      QTestState *from, *to;
> @@ -260,4 +457,8 @@ 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);
> +#ifdef CONFIG_HMP
> +    migration_test_add("/migration/hmp/parameters",
> +                       test_hmp_migration_parameters);
> +#endif
>  }
> -- 
> 2.53.0
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

[PATCH v2 10/18] tests/qtest/migration: Add a test for HMP completion

[Fabiano Rosas]

The following patches will touch HMP readline completion for migration
parameters, add a test case.

The test puts the monitor on a chardev via socket and bypasses qtest
facilities because it needs to emit raw codes to readline. It
therefore requires a couple of new helpers to read/write to the
monitor socket.

Usage:
QTEST_QEMU_BINARY=./qemu-system-x86_64 \
./tests/qtest/migration-test --full -p /x86_64/migration/hmp/completion

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration/misc-tests.c | 192 +++++++++++++++++++++++++++++
 1 file changed, 192 insertions(+)

diff --git a/tests/qtest/migration/misc-tests.c b/tests/qtest/migration/misc-tests.c
index 533cf3d55d..04f46d5e81 100644
--- a/tests/qtest/migration/misc-tests.c
+++ b/tests/qtest/migration/misc-tests.c
@@ -11,6 +11,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/sockets.h"
 #include "qapi/error.h"
 #include "qobject/qjson.h"
 #include "libqtest.h"
@@ -99,6 +100,46 @@ HMPTestData test_cases[] = {
          "can only be set through QMP"),
 };
 
+/*
+ * .input1:  partial string with an ending TAB (as if pressed by
+ *           the user).
+ * .input2:  common root of the completions, i.e. what the partial
+ *           part of .input1 string completes to.
+ * .output1: full list of completion suggestions for the string
+ *           in .input2.
+ * E.g:
+ * (qemu) .input1
+ * <after TAB>
+ * (qemu) .input2
+ * .output1
+ */
+HMPTestData completion_cases[] = {
+    TEST("migra\t",
+         "migrate",
+         "migrate migrate_cancel migrate_continue migrate_incoming "
+         "migrate_pause migrate_recover migrate_set_capability "
+         "migrate_set_parameter migrate_start_postcopy"),
+
+    /*
+     * Note QEMU doesn't keep 'info' when offering the completions
+     * suggestions.
+     */
+    TEST("info migra\t",
+         "migrate",
+         "migrate migrate_capabilities migrate_parameters"),
+
+    TEST("migrate_se\t",
+         "migrate_set_",
+         "migrate_set_capability migrate_set_parameter"),
+
+    /*
+     * parameters and capabilities are not listed here to avoid having
+     * to enumerate them all, see test_hmp_completion().
+     */
+    TEST("migrate_set_parameter \t", "migrate_set_parameter ", "@params@"),
+    TEST("migrate_set_capability \t", "migrate_set_capability ", "@caps@"),
+};
+
 /*
  * Find a contiguous run of tokens in @larger that match the sequence
  * of tokens in @smaller, ignoring mismatches due to sequences of
@@ -141,6 +182,22 @@ static bool token_list_is_substr(char **smaller, char **larger, int *last)
     return match;
 }
 
+static void assert_hmp_match_text(const char *str, const char *text)
+{
+    g_auto(GStrv) tok_str = g_strsplit_set(str, " ", -1);
+    g_auto(GStrv) tok_txt = g_strsplit_set(text, " \r\n", -1);
+    int idx;
+
+    if (token_list_is_substr(tok_str, tok_txt, &idx)) {
+        return;
+    }
+
+    g_test_message("HMP output mismatch for entry at line %d:", test_case_line);
+    g_test_message("expected vs. found (whitespace ignored):\n\n%s\n---\n%s",
+                   str, text);
+    g_assert_not_reached();
+}
+
 static void assert_hmp_match_line(const char *str, const char *text)
 {
     g_auto(GStrv) tok_str = g_strsplit_set(str, " ", -1);
@@ -217,6 +274,139 @@ static void test_hmp_migration_parameters(char *name, MigrateCommon *args)
 
     qtest_quit(qts);
 }
+
+static void hmp_sock_write(int fd, const char *buf)
+{
+    size_t sz = strlen(buf);
+
+    assert(fd > 0);
+    assert(write(fd, buf, sz) == sz);
+}
+
+static void hmp_sock_read(int fd, char *buf, size_t buf_sz)
+{
+    char *p = buf;
+    size_t sz = buf_sz - 1;
+
+    assert(fd >= 0);
+    memset(buf, 0, buf_sz);
+
+    while (sz > 0) {
+        ssize_t r = read(fd, p, sz);
+        char *prompt;
+
+        if (!r) {
+            break;
+        } else if (r < 0) {
+            if (errno == EINTR) {
+                continue;
+            }
+            g_assert_not_reached();
+        }
+
+        p += r;
+        sz -= r;
+
+        prompt = strstr(buf, "(qemu) ");
+        if (prompt) {
+            *prompt = '\0';
+            break;
+        }
+    }
+}
+
+static int comp(const void *a, const void *b)
+{
+    return strcmp(*(const char **) a, *(const char **) b);
+}
+
+static void get_migration_opts_sorted(GString *exp, const char * const *lookup, int n)
+{
+    g_autofree char **opts_array = g_new0(char *, n);
+    uint8_t i;
+
+    for (i = 0; i < n; i++) {
+        opts_array[i] = g_strdup(lookup[i]);
+    }
+
+    qsort(opts_array, n, sizeof(char *), comp);
+
+    for (i = 0; i < n; i++) {
+        g_string_append(exp, opts_array[i]);
+        if (i + 1 != n) {
+            g_string_append(exp, " ");
+        }
+    }
+
+    for (i = 0; i < n; i++) {
+        g_free(opts_array[i]);
+    }
+}
+
+static void hmp_completion_single(int fd, const struct HMPTestData *t)
+{
+    g_autoptr(GString) exp = g_string_new("");
+    char buf[8192];
+    char *output;
+
+    test_case_line = t->line;
+
+    if (g_str_equal(t->output1, "@caps@")) {
+        g_string_append(exp, "migrate_set_capability ");
+        get_migration_opts_sorted(exp, MigrationCapability_lookup.array,
+                                  MIGRATION_CAPABILITY__MAX);
+    } else if (g_str_equal(t->output1, "@params@")) {
+        g_string_append(exp, "migrate_set_parameter ");
+        get_migration_opts_sorted(exp, MigrationParameter_lookup.array,
+                                  MIGRATION_PARAMETER__MAX);
+    } else {
+        g_string_append(exp, t->output1);
+    }
+
+    hmp_sock_write(fd, t->input1);
+    hmp_sock_read(fd, buf, sizeof(buf));
+
+    /*
+     * readline first rewrites the input to the common root of the
+     * completions, then outputs the completion suggestions:
+     *
+     * (qemu) info migr<TAB>
+     * (qemu) migrate migrate_parameters
+     * migrate_capabilities ...
+     */
+    output = strstr(buf, t->input2);
+    assert_hmp_match_text(exp->str, output);
+
+    /* ^U backward kill line */
+    hmp_sock_write(fd, "\x15");
+}
+
+static void test_hmp_completion(char *name, MigrateCommon *args)
+{
+    g_autofree char *cmdline;
+    char buf[1024];
+    QTestState *qts;
+    int sockfds[2];
+
+    assert(!qemu_socketpair(AF_UNIX, SOCK_STREAM, 0, sockfds));
+    qemu_clear_cloexec(sockfds[1]);
+
+    cmdline = g_strdup_printf("-chardev socket,id=mon0,fd=%d "
+                              "-mon chardev=mon0,mode=readline -S",
+                              sockfds[1]);
+    qts = qtest_init(cmdline);
+    close(sockfds[1]);
+
+    /* read HMP banner */
+    hmp_sock_read(sockfds[0], buf, sizeof(buf));
+
+    for (int i = 0; i < G_N_ELEMENTS(completion_cases); i++) {
+        hmp_completion_single(sockfds[0], &completion_cases[i]);
+    }
+
+    close(sockfds[0]);
+    qtest_quit(qts);
+}
 #endif /* CONFIG_HMP */
 
 static void test_baddest(char *name, MigrateCommon *args)
@@ -460,5 +650,7 @@ void migration_test_add_misc(MigrationTestEnv *env)
 #ifdef CONFIG_HMP
     migration_test_add("/migration/hmp/parameters",
                        test_hmp_migration_parameters);
+    migration_test_add("/migration/hmp/completion",
+                       test_hmp_completion);
 #endif
 }
-- 
2.53.0

Re: [PATCH v2 10/18] tests/qtest/migration: Add a test for HMP completion

[Peter Xu]

On Wed, Sep 09, 2026 at 06:45:00PM -0300, Fabiano Rosas wrote:
> The following patches will touch HMP readline completion for migration
> parameters, add a test case.
> 
> The test puts the monitor on a chardev via socket and bypasses qtest
> facilities because it needs to emit raw codes to readline. It
> therefore requires a couple of new helpers to read/write to the
> monitor socket.
> 
> Usage:
> QTEST_QEMU_BINARY=./qemu-system-x86_64 \
> ./tests/qtest/migration-test --full -p /x86_64/migration/hmp/completion
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

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

-- 
Peter Xu

Re: [PATCH v2 10/18] tests/qtest/migration: Add a test for HMP completion

[Dr. David Alan Gilbert]

* Fabiano Rosas (farosas@suse.de) wrote:
> The following patches will touch HMP readline completion for migration
> parameters, add a test case.
> 
> The test puts the monitor on a chardev via socket and bypasses qtest
> facilities because it needs to emit raw codes to readline. It
> therefore requires a couple of new helpers to read/write to the
> monitor socket.
> 
> Usage:
> QTEST_QEMU_BINARY=./qemu-system-x86_64 \
> ./tests/qtest/migration-test --full -p /x86_64/migration/hmp/completion
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---

> +static void hmp_sock_read(int fd, char *buf, size_t buf_sz)
> +{
> +    char *p = buf;
> +    size_t sz = buf_sz - 1;
> +
> +    assert(fd >= 0);
> +    memset(buf, 0, buf_sz);
> +
> +    while (sz > 0) {
> +        ssize_t r = read(fd, p, sz);
> +        char *prompt;
> +
> +        if (!r) {
> +            break;
> +        } else if (r < 0) {
> +            if (errno == EINTR) {
> +                continue;
> +            }
> +            g_assert_not_reached();
> +        }
> +
> +        p += r;
> +        sz -= r;
> +
> +        prompt = strstr(buf, "(qemu) ");
> +        if (prompt) {
> +            *prompt = '\0';

I'm a bit confused by what you want the output to look like after this,
are you assuming this is the end of the string you're reading so truncation
is OK?

> +            break;
> +        }
> +    }
> +}
> +
> +static int comp(const void *a, const void *b)
> +{
> +    return strcmp(*(const char **) a, *(const char **) b);
> +}

Can you use qemu_pstrcmp0 there? (Maybe with g_ptr_array and g_ptr_array_sort is
what adds to some of the other questions below?)

> +static void get_migration_opts_sorted(GString *exp, const char * const *lookup, int n)
> +{
> +    g_autofree char **opts_array = g_new0(char *, n);
> +    uint8_t i;
> +
> +    for (i = 0; i < n; i++) {
> +        opts_array[i] = g_strdup(lookup[i]);
> +    }

If you made opts_array one element larger, and added a NULL at the end could you...

> +    qsort(opts_array, n, sizeof(char *), comp);
> +
> +    for (i = 0; i < n; i++) {
> +        g_string_append(exp, opts_array[i]);
> +        if (i + 1 != n) {
> +            g_string_append(exp, " ");
> +        }
> +    }

Use g_strjoinv here?

> +    for (i = 0; i < n; i++) {
> +        g_free(opts_array[i]);
> +    }
> +}

Dave

> +static void hmp_completion_single(int fd, const struct HMPTestData *t)
> +{
> +    g_autoptr(GString) exp = g_string_new("");
> +    char buf[8192];
> +    char *output;
> +
> +    test_case_line = t->line;
> +
> +    if (g_str_equal(t->output1, "@caps@")) {
> +        g_string_append(exp, "migrate_set_capability ");
> +        get_migration_opts_sorted(exp, MigrationCapability_lookup.array,
> +                                  MIGRATION_CAPABILITY__MAX);
> +    } else if (g_str_equal(t->output1, "@params@")) {
> +        g_string_append(exp, "migrate_set_parameter ");
> +        get_migration_opts_sorted(exp, MigrationParameter_lookup.array,
> +                                  MIGRATION_PARAMETER__MAX);
> +    } else {
> +        g_string_append(exp, t->output1);
> +    }
> +
> +    hmp_sock_write(fd, t->input1);
> +    hmp_sock_read(fd, buf, sizeof(buf));
> +
> +    /*
> +     * readline first rewrites the input to the common root of the
> +     * completions, then outputs the completion suggestions:
> +     *
> +     * (qemu) info migr<TAB>
> +     * (qemu) migrate migrate_parameters
> +     * migrate_capabilities ...
> +     */
> +    output = strstr(buf, t->input2);
> +    assert_hmp_match_text(exp->str, output);
> +
> +    /* ^U backward kill line */
> +    hmp_sock_write(fd, "\x15");
> +}
> +
> +static void test_hmp_completion(char *name, MigrateCommon *args)
> +{
> +    g_autofree char *cmdline;
> +    char buf[1024];
> +    QTestState *qts;
> +    int sockfds[2];
> +
> +    assert(!qemu_socketpair(AF_UNIX, SOCK_STREAM, 0, sockfds));
> +    qemu_clear_cloexec(sockfds[1]);
> +
> +    cmdline = g_strdup_printf("-chardev socket,id=mon0,fd=%d "
> +                              "-mon chardev=mon0,mode=readline -S",
> +                              sockfds[1]);
> +    qts = qtest_init(cmdline);
> +    close(sockfds[1]);
> +
> +    /* read HMP banner */
> +    hmp_sock_read(sockfds[0], buf, sizeof(buf));
> +
> +    for (int i = 0; i < G_N_ELEMENTS(completion_cases); i++) {
> +        hmp_completion_single(sockfds[0], &completion_cases[i]);
> +    }
> +
> +    close(sockfds[0]);
> +    qtest_quit(qts);
> +}
>  #endif /* CONFIG_HMP */
>  
>  static void test_baddest(char *name, MigrateCommon *args)
> @@ -460,5 +650,7 @@ void migration_test_add_misc(MigrationTestEnv *env)
>  #ifdef CONFIG_HMP
>      migration_test_add("/migration/hmp/parameters",
>                         test_hmp_migration_parameters);
> +    migration_test_add("/migration/hmp/completion",
> +                       test_hmp_completion);
>  #endif
>  }
> -- 
> 2.53.0
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

[PATCH v2 11/18] migration: HMP: Fix bandwidth parameters

[Fabiano Rosas]

The max-bandwidth and avail-switchover-bandwidth parameters are
declared as bytes/second in migration.json and interpreted as such
throughout the code, except in HMP where they're expected to be
provided as mebibytes.

Fix the HMP code to parse the values as bytes.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration-hmp-cmds.c     | 23 +++--------------------
 tests/qtest/migration/misc-tests.c |  4 ++--
 2 files changed, 5 insertions(+), 22 deletions(-)

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 27b38d0e5f..67ea2eea15 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -596,10 +596,9 @@ void hmp_migrate_set_parameter(MonitorHMP *hmp, const QDict *qdict)
     const char *valuestr = qdict_get_str(qdict, "value");
     Visitor *v = string_input_visitor_new(valuestr);
     MigrationParameters *p = g_new0(MigrationParameters, 1);
-    uint64_t valuebw = 0;
     uint64_t cache_size;
     Error *err = NULL;
-    int val, ret;
+    int val;
 
     val = qapi_enum_parse(&MigrationParameter_lookup, param, -1, &err);
     if (val < 0) {
@@ -644,27 +643,11 @@ void hmp_migrate_set_parameter(MonitorHMP *hmp, const QDict *qdict)
         break;
     case MIGRATION_PARAMETER_MAX_BANDWIDTH:
         p->has_max_bandwidth = true;
-        /*
-         * Can't use visit_type_size() here, because it
-         * defaults to Bytes rather than Mebibytes.
-         */
-        ret = qemu_strtosz_MiB(valuestr, NULL, &valuebw);
-        if (ret < 0 || valuebw > INT64_MAX
-            || (size_t)valuebw != valuebw) {
-            error_setg(&err, "Invalid size %s", valuestr);
-            break;
-        }
-        p->max_bandwidth = valuebw;
+        visit_type_size(v, param, &p->max_bandwidth, &err);
         break;
     case MIGRATION_PARAMETER_AVAIL_SWITCHOVER_BANDWIDTH:
         p->has_avail_switchover_bandwidth = true;
-        ret = qemu_strtosz_MiB(valuestr, NULL, &valuebw);
-        if (ret < 0 || valuebw > INT64_MAX
-            || (size_t)valuebw != valuebw) {
-            error_setg(&err, "Invalid size %s", valuestr);
-            break;
-        }
-        p->avail_switchover_bandwidth = valuebw;
+        visit_type_size(v, param, &p->avail_switchover_bandwidth, &err);
         break;
     case MIGRATION_PARAMETER_DOWNTIME_LIMIT:
         p->has_downtime_limit = true;
diff --git a/tests/qtest/migration/misc-tests.c b/tests/qtest/migration/misc-tests.c
index 04f46d5e81..447fa5e232 100644
--- a/tests/qtest/migration/misc-tests.c
+++ b/tests/qtest/migration/misc-tests.c
@@ -62,8 +62,8 @@ HMPTestData test_cases[] = {
     TEST("announce-rounds", "6", "6"),
     TEST("announce-step", "15", "15 ms"),
     TEST("downtime-limit", "400", "400 ms"),
-    TEST("avail-switchover-bandwidth", "2097152", "2199023255552 bytes/second"),
-    TEST("max-bandwidth", "9876543", "10356305952768 bytes/second"),
+    TEST("avail-switchover-bandwidth", "2097152", "2097152 bytes/second"),
+    TEST("max-bandwidth", "9876543", "9876543 bytes/second"),
     TEST("max-postcopy-bandwidth", "1048576", "1048576 bytes/second"),
     TEST("vcpu-dirty-limit", "20", "20 MB/s"),
     TEST("x-rdma-chunk-size", "1048576", "1048576 bytes"),
-- 
2.53.0

Re: [PATCH v2 11/18] migration: HMP: Fix bandwidth parameters

[Markus Armbruster]

Fabiano Rosas <farosas@suse.de> writes:

> The max-bandwidth and avail-switchover-bandwidth parameters are
> declared as bytes/second in migration.json and interpreted as such
> throughout the code, except in HMP where they're expected to be
> provided as mebibytes.
>
> Fix the HMP code to parse the values as bytes.

I wouldn't call this a fix.  The choice of unit was clearly intentional.
Doesn't mean we must not change it now.  David, what do you think?

> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH v2 11/18] migration: HMP: Fix bandwidth parameters

[Fabiano Rosas]

Markus Armbruster <armbru@redhat.com> writes:

> Fabiano Rosas <farosas@suse.de> writes:
>
>> The max-bandwidth and avail-switchover-bandwidth parameters are
>> declared as bytes/second in migration.json and interpreted as such
>> throughout the code, except in HMP where they're expected to be
>> provided as mebibytes.
>>
>> Fix the HMP code to parse the values as bytes.
>
> I wouldn't call this a fix.  The choice of unit was clearly intentional.
> Doesn't mean we must not change it now.  David, what do you think?
>

Hm, but if the code uses bytes in arithmetic throughout, there must be
bugs lingering somewhere. Although I haven't verified that. Nonetheless,
I can use a different terminology, no worries.

Re: [PATCH v2 11/18] migration: HMP: Fix bandwidth parameters

[Markus Armbruster]

Fabiano Rosas <farosas@suse.de> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Fabiano Rosas <farosas@suse.de> writes:
>>
>>> The max-bandwidth and avail-switchover-bandwidth parameters are
>>> declared as bytes/second in migration.json and interpreted as such
>>> throughout the code, except in HMP where they're expected to be
>>> provided as mebibytes.
>>>
>>> Fix the HMP code to parse the values as bytes.
>>
>> I wouldn't call this a fix.  The choice of unit was clearly intentional.
>> Doesn't mean we must not change it now.  David, what do you think?
>>
>
> Hm, but if the code uses bytes in arithmetic throughout, there must be
> bugs lingering somewhere. Although I haven't verified that.

I don't think so; the old code parses the argument string into a byte
count.

>                                                             Nonetheless,
> I can use a different terminology, no worries.

Thanks!

Re: [PATCH v2 11/18] migration: HMP: Fix bandwidth parameters

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Fabiano Rosas <farosas@suse.de> writes:
> 
> > The max-bandwidth and avail-switchover-bandwidth parameters are
> > declared as bytes/second in migration.json and interpreted as such
> > throughout the code, except in HMP where they're expected to be
> > provided as mebibytes.
> >
> > Fix the HMP code to parse the values as bytes.
> 
> I wouldn't call this a fix.  The choice of unit was clearly intentional.
> Doesn't mean we must not change it now.  David, what do you think?

Meh I wouldn't worry about whether we call it a 'fix' or not!
The real benefit here is that by making this number consistent
the units in the next patch aren't needed since everyone uses
the same unit.

Dave

> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH v2 11/18] migration: HMP: Fix bandwidth parameters

[Dr. David Alan Gilbert]

* Fabiano Rosas (farosas@suse.de) wrote:
> The max-bandwidth and avail-switchover-bandwidth parameters are
> declared as bytes/second in migration.json and interpreted as such
> throughout the code, except in HMP where they're expected to be
> provided as mebibytes.
> 
> Fix the HMP code to parse the values as bytes.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Makes sense, and shockingly, it means that 'max-bandwidth' and 'max-postcopy-bandwidth'
now have the same units!

Please point this out in the release changelog.

Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>

> ---
>  migration/migration-hmp-cmds.c     | 23 +++--------------------
>  tests/qtest/migration/misc-tests.c |  4 ++--
>  2 files changed, 5 insertions(+), 22 deletions(-)
> 
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index 27b38d0e5f..67ea2eea15 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -596,10 +596,9 @@ void hmp_migrate_set_parameter(MonitorHMP *hmp, const QDict *qdict)
>      const char *valuestr = qdict_get_str(qdict, "value");
>      Visitor *v = string_input_visitor_new(valuestr);
>      MigrationParameters *p = g_new0(MigrationParameters, 1);
> -    uint64_t valuebw = 0;
>      uint64_t cache_size;
>      Error *err = NULL;
> -    int val, ret;
> +    int val;
>  
>      val = qapi_enum_parse(&MigrationParameter_lookup, param, -1, &err);
>      if (val < 0) {
> @@ -644,27 +643,11 @@ void hmp_migrate_set_parameter(MonitorHMP *hmp, const QDict *qdict)
>          break;
>      case MIGRATION_PARAMETER_MAX_BANDWIDTH:
>          p->has_max_bandwidth = true;
> -        /*
> -         * Can't use visit_type_size() here, because it
> -         * defaults to Bytes rather than Mebibytes.
> -         */
> -        ret = qemu_strtosz_MiB(valuestr, NULL, &valuebw);
> -        if (ret < 0 || valuebw > INT64_MAX
> -            || (size_t)valuebw != valuebw) {
> -            error_setg(&err, "Invalid size %s", valuestr);
> -            break;
> -        }
> -        p->max_bandwidth = valuebw;
> +        visit_type_size(v, param, &p->max_bandwidth, &err);
>          break;
>      case MIGRATION_PARAMETER_AVAIL_SWITCHOVER_BANDWIDTH:
>          p->has_avail_switchover_bandwidth = true;
> -        ret = qemu_strtosz_MiB(valuestr, NULL, &valuebw);
> -        if (ret < 0 || valuebw > INT64_MAX
> -            || (size_t)valuebw != valuebw) {
> -            error_setg(&err, "Invalid size %s", valuestr);
> -            break;
> -        }
> -        p->avail_switchover_bandwidth = valuebw;
> +        visit_type_size(v, param, &p->avail_switchover_bandwidth, &err);
>          break;
>      case MIGRATION_PARAMETER_DOWNTIME_LIMIT:
>          p->has_downtime_limit = true;
> diff --git a/tests/qtest/migration/misc-tests.c b/tests/qtest/migration/misc-tests.c
> index 04f46d5e81..447fa5e232 100644
> --- a/tests/qtest/migration/misc-tests.c
> +++ b/tests/qtest/migration/misc-tests.c
> @@ -62,8 +62,8 @@ HMPTestData test_cases[] = {
>      TEST("announce-rounds", "6", "6"),
>      TEST("announce-step", "15", "15 ms"),
>      TEST("downtime-limit", "400", "400 ms"),
> -    TEST("avail-switchover-bandwidth", "2097152", "2199023255552 bytes/second"),
> -    TEST("max-bandwidth", "9876543", "10356305952768 bytes/second"),
> +    TEST("avail-switchover-bandwidth", "2097152", "2097152 bytes/second"),
> +    TEST("max-bandwidth", "9876543", "9876543 bytes/second"),
>      TEST("max-postcopy-bandwidth", "1048576", "1048576 bytes/second"),
>      TEST("vcpu-dirty-limit", "20", "20 MB/s"),
>      TEST("x-rdma-chunk-size", "1048576", "1048576 bytes"),
> -- 
> 2.53.0
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH v2 11/18] migration: HMP: Fix bandwidth parameters

[Peter Xu]

On Wed, Sep 09, 2026 at 06:45:01PM -0300, Fabiano Rosas wrote:
> The max-bandwidth and avail-switchover-bandwidth parameters are
> declared as bytes/second in migration.json and interpreted as such
> throughout the code, except in HMP where they're expected to be
> provided as mebibytes.
> 
> Fix the HMP code to parse the values as bytes.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

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

-- 
Peter Xu

[PATCH v2 12/18] migration: Change HMP 'info migrate_parameters' output

[Fabiano Rosas]

The output of 'info migrate_parameters' includes units of measurement
for a few parameters. This is convenient for a user. It also requires
every parameter to be individually listed in the
hmp_migrate_set_parameter() function, which in turn requires the
MigrationParameter (singular) enum to exist. While the latter is not
bothersome at all, the former is.

From a development and maintenance perspective, having a list of
parameters explicitly written in several parts of the code brings
several annoyances: conflicts during rebase, multiple extra hits when
grepping, requires contributors to search for every location a change
needs to be mirrored to, etc.

Remove the units from the output so we can write this code in a more
convenient way. The HMP output is not part of any ABI.

Also remove quotes from around the TLS options strings as this is
inconsistent with all the other strings.

Change block-bitmap-mapping format to a single line. This requires
updating one of the iotests to match.

Before:                                      After:
                     (unchanged entries omitted)
announce-initial: 50 ms                      announce-initial: 50
announce-max: 550 ms                         announce-max: 550
announce-rounds: 5                           announce-rounds: 5
announce-step: 100 ms                        announce-step: 100
tls-creds: ''                                tls-creds:
tls-hostname: ''                             tls-hostname:
tls-authz: ''                                tls-authz:
max-bandwidth: 134217728 bytes/second        max-bandwidth: 134217728
avail-switchover-bandwidth: 0 bytes/second   avail-switchover-bandwidth: 0
max-postcopy-bandwidth: 0 bytes/second       max-postcopy-bandwidth: 0
downtime-limit: 300 ms                       downtime-limit: 300
x-checkpoint-delay: 20000 ms                 x-checkpoint-delay: 20000
xbzrle-cache-size: 67108864 bytes            xbzrle-cache-size: 67108864
x-vcpu-dirty-limit-period: 1000 ms           x-vcpu-dirty-limit-period: 1000
vcpu-dirty-limit: 1 MB/s                     vcpu-dirty-limit: 1
x-rdma-chunk-size: 1048576 bytes             x-rdma-chunk-size: 1048576
block-bitmap-mapping:                        block-bitmap-mapping: bitmaps: name: bmap0 alias: bmap0 node-name: node-src alias: node-dst
  'node-src' -> 'node-dst'
    'bmap0' -> 'bmap0'

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration-hmp-cmds.c     | 80 +++++++++++++++++-------------
 tests/qemu-iotests/300             | 20 +++++---
 tests/qtest/migration/misc-tests.c | 30 +++++------
 3 files changed, 74 insertions(+), 56 deletions(-)

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 67ea2eea15..5d754414c4 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -335,16 +335,16 @@ void hmp_info_migrate_parameters(MonitorHMP *hmp, const QDict *qdict)
     params = qmp_query_migrate_parameters(NULL);
 
     if (params) {
-        monitor_hmp_printf(hmp, "%s: %" PRIu64 " ms\n",
+        monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_INITIAL),
             params->announce_initial);
-        monitor_hmp_printf(hmp, "%s: %" PRIu64 " ms\n",
+        monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_MAX),
             params->announce_max);
         monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_ROUNDS),
             params->announce_rounds);
-        monitor_hmp_printf(hmp, "%s: %" PRIu64 " ms\n",
+        monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_STEP),
             params->announce_step);
         assert(params->has_throttle_trigger_threshold);
@@ -368,35 +368,35 @@ void hmp_info_migrate_parameters(MonitorHMP *hmp, const QDict *qdict)
             MigrationParameter_str(MIGRATION_PARAMETER_MAX_CPU_THROTTLE),
             params->max_cpu_throttle);
         assert(params->tls_creds);
-        monitor_hmp_printf(hmp, "%s: '%s'\n",
+        monitor_hmp_printf(hmp, "%s: %s\n",
             MigrationParameter_str(MIGRATION_PARAMETER_TLS_CREDS),
                        params->tls_creds->u.s);
         assert(params->tls_hostname);
-        monitor_hmp_printf(hmp, "%s: '%s'\n",
+        monitor_hmp_printf(hmp, "%s: %s\n",
             MigrationParameter_str(MIGRATION_PARAMETER_TLS_HOSTNAME),
                        params->tls_hostname->u.s);
         assert(params->tls_authz);
-        monitor_hmp_printf(hmp, "%s: '%s'\n",
+        monitor_hmp_printf(hmp, "%s: %s\n",
             MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
                        params->tls_authz->u.s);
         assert(params->has_max_bandwidth);
-        monitor_hmp_printf(hmp, "%s: %" PRIu64 " bytes/second\n",
+        monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_MAX_BANDWIDTH),
             params->max_bandwidth);
         assert(params->has_avail_switchover_bandwidth);
-        monitor_hmp_printf(hmp, "%s: %" PRIu64 " bytes/second\n",
+        monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_AVAIL_SWITCHOVER_BANDWIDTH),
             params->avail_switchover_bandwidth);
         assert(params->has_max_postcopy_bandwidth);
-        monitor_hmp_printf(hmp, "%s: %" PRIu64 " bytes/second\n",
+        monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_MAX_POSTCOPY_BANDWIDTH),
             params->max_postcopy_bandwidth);
         assert(params->has_downtime_limit);
-        monitor_hmp_printf(hmp, "%s: %" PRIu64 " ms\n",
+        monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_DOWNTIME_LIMIT),
             params->downtime_limit);
         assert(params->has_x_checkpoint_delay);
-        monitor_hmp_printf(hmp, "%s: %u ms\n",
+        monitor_hmp_printf(hmp, "%s: %u\n",
             MigrationParameter_str(MIGRATION_PARAMETER_X_CHECKPOINT_DELAY),
             params->x_checkpoint_delay);
         monitor_hmp_printf(hmp, "%s: %u\n",
@@ -410,41 +410,51 @@ void hmp_info_migrate_parameters(MonitorHMP *hmp, const QDict *qdict)
             MigrationParameter_str(MIGRATION_PARAMETER_ZERO_PAGE_DETECTION),
             qapi_enum_lookup(&ZeroPageDetection_lookup,
                 params->zero_page_detection));
-        monitor_hmp_printf(hmp, "%s: %" PRIu64 " bytes\n",
+        monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE),
             params->xbzrle_cache_size);
 
         if (s->has_block_bitmap_mapping) {
-            const BitmapMigrationNodeAliasList *bmnal;
+            BitmapMigrationNodeAliasList *nal;
+            BitmapMigrationNodeAlias *na;
+            BitmapMigrationBitmapAliasList *bal;
+            BitmapMigrationBitmapAlias *ba;
+            BitmapMigrationBitmapAliasTransform *bat;
 
-            monitor_hmp_printf(hmp, "%s:\n",
-                               MigrationParameter_str(
-                                   MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING));
+            monitor_hmp_printf(hmp, "%s:",
+                           MigrationParameter_str(
+                               MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING));
 
-            for (bmnal = params->block_bitmap_mapping;
-                 bmnal;
-                 bmnal = bmnal->next)
+            for (nal = params->block_bitmap_mapping; nal; nal = nal->next)
             {
-                const BitmapMigrationNodeAlias *bmna = bmnal->value;
-                const BitmapMigrationBitmapAliasList *bmbal;
+                na = nal->value;
+                monitor_hmp_printf(hmp, " bitmaps:");
+                for (bal = na->bitmaps; bal; bal = bal->next) {
+                    ba = bal->value;
+                    bat = ba->transform;
 
-                monitor_hmp_printf(hmp, "  '%s' -> '%s'\n",
-                                   bmna->node_name, bmna->alias);
-
-                for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
-                    const BitmapMigrationBitmapAlias *bmba = bmbal->value;
-
-                    monitor_hmp_printf(hmp, "    '%s' -> '%s'\n",
-                                       bmba->name, bmba->alias);
+                    monitor_hmp_printf(hmp, " name: %s", ba->name);
+                    if (bat && bat->has_persistent) {
+                        if (bat->persistent) {
+                            monitor_hmp_printf(hmp, " persistent: on");
+                        } else {
+                            monitor_hmp_printf(hmp, " persistent: off");
+                        }
+                    }
+                    monitor_hmp_printf(hmp, " alias: %s", ba->alias);
                 }
+                monitor_hmp_printf(hmp, " node-name: %s alias: %s",
+                               na->node_name, na->alias);
             }
+
+            monitor_hmp_printf(hmp, "\n");
         }
 
-        monitor_hmp_printf(hmp, "%s: %" PRIu64 " ms\n",
+        monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
         MigrationParameter_str(MIGRATION_PARAMETER_X_VCPU_DIRTY_LIMIT_PERIOD),
         params->x_vcpu_dirty_limit_period);
 
-        monitor_hmp_printf(hmp, "%s: %" PRIu64 " MB/s\n",
+        monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_VCPU_DIRTY_LIMIT),
             params->vcpu_dirty_limit);
 
@@ -461,10 +471,10 @@ void hmp_info_migrate_parameters(MonitorHMP *hmp, const QDict *qdict)
         }
 
         if (params->has_x_rdma_chunk_size) {
-            monitor_hmp_printf(hmp, "%s: %" PRIu64 " bytes\n",
-                               MigrationParameter_str(
-                                   MIGRATION_PARAMETER_X_RDMA_CHUNK_SIZE),
-                               params->x_rdma_chunk_size);
+            monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
+                           MigrationParameter_str(
+                               MIGRATION_PARAMETER_X_RDMA_CHUNK_SIZE),
+                           params->x_rdma_chunk_size);
         }
 
         assert(params->has_cpr_exec_command);
diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index df2ba32632..7a5006b3bc 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -147,8 +147,7 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
 
             result = vm.qmp('human-monitor-command',
                             command_line='info migrate_parameters')
-
-            m = re.search(r'^block-bitmap-mapping:\r?(\n  .*)*\n',
+            m = re.search(r'^block-bitmap-mapping:(.*)\r\n',
                           result['return'], flags=re.MULTILINE)
             hmp_mapping = m.group(0).replace('\r', '') if m else None
 
@@ -158,15 +157,24 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
 
     @staticmethod
     def to_hmp_mapping(mapping: BlockBitmapMapping) -> str:
-        result = 'block-bitmap-mapping:\n'
+        result = 'block-bitmap-mapping:'
 
         for node in mapping:
-            result += f"  '{node['node-name']}' -> '{node['alias']}'\n"
-
             assert isinstance(node['bitmaps'], list)
+            result += ' bitmaps:'
             for bitmap in node['bitmaps']:
-                result += f"    '{bitmap['name']}' -> '{bitmap['alias']}'\n"
+                result += f" name: {bitmap['name']}"
+                if 'transform' in bitmap:
+                    if 'persistent' in bitmap['transform']:
+                        if bitmap['transform']['persistent']:
+                            result += " persistent: on"
+                        else:
+                            result += " persistent: off"
+                result += f" alias: {bitmap['alias']}"
 
+            result += f" node-name: {node['node-name']} alias: {node['alias']}"
+
+        result += '\n'
         return result
 
 
diff --git a/tests/qtest/migration/misc-tests.c b/tests/qtest/migration/misc-tests.c
index 447fa5e232..2261ae7c89 100644
--- a/tests/qtest/migration/misc-tests.c
+++ b/tests/qtest/migration/misc-tests.c
@@ -57,21 +57,21 @@ HMPTestData test_cases[] = {
     TEST("direct-io", "on", "on"),
 
     /* uint64_t */
-    TEST("announce-initial", "60", "60 ms"),
-    TEST("announce-max", "600", "600 ms"),
+    TEST("announce-initial", "60", "60"),
+    TEST("announce-max", "600", "600"),
     TEST("announce-rounds", "6", "6"),
-    TEST("announce-step", "15", "15 ms"),
-    TEST("downtime-limit", "400", "400 ms"),
-    TEST("avail-switchover-bandwidth", "2097152", "2097152 bytes/second"),
-    TEST("max-bandwidth", "9876543", "9876543 bytes/second"),
-    TEST("max-postcopy-bandwidth", "1048576", "1048576 bytes/second"),
-    TEST("vcpu-dirty-limit", "20", "20 MB/s"),
-    TEST("x-rdma-chunk-size", "1048576", "1048576 bytes"),
-    TEST("x-vcpu-dirty-limit-period", "750", "750 ms"),
-    TEST("xbzrle-cache-size", "67108864", "67108864 bytes"),
+    TEST("announce-step", "15", "15"),
+    TEST("downtime-limit", "400", "400"),
+    TEST("avail-switchover-bandwidth", "2097152", "2097152"),
+    TEST("max-bandwidth", "9876543", "9876543"),
+    TEST("max-postcopy-bandwidth", "1048576", "1048576"),
+    TEST("vcpu-dirty-limit", "20", "20"),
+    TEST("x-rdma-chunk-size", "1048576", "1048576"),
+    TEST("x-vcpu-dirty-limit-period", "750", "750"),
+    TEST("xbzrle-cache-size", "67108864", "67108864"),
 
     /* uint32_t */
-    TEST("x-checkpoint-delay", "5000", "5000 ms"),
+    TEST("x-checkpoint-delay", "5000", "5000"),
 
     /* uint8_t */
     TEST("cpu-throttle-increment", "15", "15"),
@@ -84,9 +84,9 @@ HMPTestData test_cases[] = {
     TEST("mode", "cpr-exec", "cpr-exec"),
     TEST("multifd-compression", "zlib", "zlib"),
     TEST("zero-page-detection", "none", "none"),
-    TEST("tls-authz", "my_authz", "'my_authz'"),
-    TEST("tls-creds", "null", "'null'"),
-    TEST("tls-hostname", "localhost", "'localhost'"),
+    TEST("tls-authz", "my_authz", "my_authz"),
+    TEST("tls-creds", "null", "null"),
+    TEST("tls-hostname", "localhost", "localhost"),
     TEST("cpr-exec-command", "/bin/true foobar", "/bin/true foobar"),
 
     /* can be set but are currently missing in the query output */
-- 
2.53.0

Re: [PATCH v2 12/18] migration: Change HMP 'info migrate_parameters' output

[Markus Armbruster]

Fabiano Rosas <farosas@suse.de> writes:

> The output of 'info migrate_parameters' includes units of measurement
> for a few parameters. This is convenient for a user.

Yes.

>                                                      It also requires
> every parameter to be individually listed in the
> hmp_migrate_set_parameter() function, which in turn requires the
> MigrationParameter (singular) enum to exist.

I think I understand what you mean, but your text doesn't express it
clearly.

How does having in "info migrate" imply the need for parameter-specific
code in "migrate_set_parameter"?  Perhaps with the (unstated)
requirement that "migrate_set_parameter" must take values in the units
shown by "info migrate_parameters"?

Let me try to explain the why differently.

hmp_info_migrate_parameters() and hmp_migrate_set_parameter() both have
code for each parameter, and they both use enum MigrationParameter.

You want to replace this parameter-specific code by code that works for
any parameter, in both functions.

Your new code really doesn't want to do special per-parameter stuff.
That's why you want to get rid of all that.

You split the work as follows:

* Get rid of special units in migrate_set_parameter [previous patch].
  Interface change, simple patch.

* Don't show units in info migrate_parameters [this patch].
  Interface change, simple patch.

* Replace parameter-specific code [next patches].  More interesting, but
  no interface change.

I like this split, it helps reviewers.

>                                              While the latter is not
> bothersome at all, the former is.

The values of enum MigrationParameter duplicate the members of struct
MigrationParameters.  That's plenty bothersome, isn't it?

As far as I can tell, the only remaining uses of enum MigrationParameter
at the end of the series are an assertion in
migrate_mark_all_params_present(), which we discussed in review of v1,
and hmp_completion_single() in qtest/migration/misc-tests.c.  Any chance
we can get rid of it entirely?

> From a development and maintenance perspective, having a list of
> parameters explicitly written in several parts of the code brings
> several annoyances: conflicts during rebase, multiple extra hits when
> grepping, requires contributors to search for every location a change
> needs to be mirrored to, etc.
>
> Remove the units from the output so we can write this code in a more
> convenient way. The HMP output is not part of any ABI.
>
> Also remove quotes from around the TLS options strings as this is
> inconsistent with all the other strings.
>
> Change block-bitmap-mapping format to a single line. This requires
> updating one of the iotests to match.
>
> Before:                                      After:
>                      (unchanged entries omitted)
> announce-initial: 50 ms                      announce-initial: 50
> announce-max: 550 ms                         announce-max: 550
> announce-rounds: 5                           announce-rounds: 5
> announce-step: 100 ms                        announce-step: 100
> tls-creds: ''                                tls-creds:
> tls-hostname: ''                             tls-hostname:
> tls-authz: ''                                tls-authz:
> max-bandwidth: 134217728 bytes/second        max-bandwidth: 134217728
> avail-switchover-bandwidth: 0 bytes/second   avail-switchover-bandwidth: 0
> max-postcopy-bandwidth: 0 bytes/second       max-postcopy-bandwidth: 0
> downtime-limit: 300 ms                       downtime-limit: 300
> x-checkpoint-delay: 20000 ms                 x-checkpoint-delay: 20000
> xbzrle-cache-size: 67108864 bytes            xbzrle-cache-size: 67108864
> x-vcpu-dirty-limit-period: 1000 ms           x-vcpu-dirty-limit-period: 1000
> vcpu-dirty-limit: 1 MB/s                     vcpu-dirty-limit: 1
> x-rdma-chunk-size: 1048576 bytes             x-rdma-chunk-size: 1048576
> block-bitmap-mapping:                        block-bitmap-mapping: bitmaps: name: bmap0 alias: bmap0 node-name: node-src alias: node-dst
>   'node-src' -> 'node-dst'
>     'bmap0' -> 'bmap0'

Uh, the value of block-bitmap-mapping can become really long.  Its QAPI
type is array of BitmapMigrationNodeAlias, and each array element
contains another array.

Why is this change useful?

> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH v2 12/18] migration: Change HMP 'info migrate_parameters' output

[Fabiano Rosas]

Markus Armbruster <armbru@redhat.com> writes:

> Fabiano Rosas <farosas@suse.de> writes:
>
>> The output of 'info migrate_parameters' includes units of measurement
>> for a few parameters. This is convenient for a user.
>
> Yes.
>
>>                                                      It also requires
>> every parameter to be individually listed in the
>> hmp_migrate_set_parameter() function, which in turn requires the
>> MigrationParameter (singular) enum to exist.
>
> I think I understand what you mean, but your text doesn't express it
> clearly.
>
> How does having in "info migrate" imply the need for parameter-specific
> code in "migrate_set_parameter"?  Perhaps with the (unstated)
> requirement that "migrate_set_parameter" must take values in the units
> shown by "info migrate_parameters"?
>
> Let me try to explain the why differently.
>
> hmp_info_migrate_parameters() and hmp_migrate_set_parameter() both have
> code for each parameter, and they both use enum MigrationParameter.
>
> You want to replace this parameter-specific code by code that works for
> any parameter, in both functions.
>
> Your new code really doesn't want to do special per-parameter stuff.
> That's why you want to get rid of all that.
>

Right, you got the point, it's that having the units requires custom
formatting which needs to be per-parameter and therefore requires the
parameters to be enumerated in the code which increases the maintenance
burden. I'll update the commit message with something more clear.

> You split the work as follows:
>
> * Get rid of special units in migrate_set_parameter [previous patch].
>   Interface change, simple patch.
>

This one is separate because it affects how the user _sets_ those
values, while the rest of the changes only affects how the user _sees_
the values.

> * Don't show units in info migrate_parameters [this patch].
>   Interface change, simple patch.
>
> * Replace parameter-specific code [next patches].  More interesting, but
>   no interface change.
>
> I like this split, it helps reviewers.
>
>>                                              While the latter is not
>> bothersome at all, the former is.
>
> The values of enum MigrationParameter duplicate the members of struct
> MigrationParameters.  That's plenty bothersome, isn't it?
>

As Peter pointed out in v1, this is now less bothersome than it used to
be, since we stopped requiring documentation for MigrationParameter in
commit 2220c2b9ef (qapi/migration: Don't document MigrationParameter,
2025-12-15).

> As far as I can tell, the only remaining uses of enum MigrationParameter
> at the end of the series are an assertion in
> migrate_mark_all_params_present(), which we discussed in review of v1,
> and hmp_completion_single() in qtest/migration/misc-tests.c.  Any chance
> we can get rid of it entirely?
>

We need something that ensures migrate_mark_all_params_present() will be
updated when a new parameter is added. My attempt in v1 of converting to
MigrationParameters to QDict didn't work because the has_ fields are not
carried over to the QDict. At the moment, the best I can do is to use
the enum.

As for the test, we could simply not test the completion of
parameters. It's not the end of the world, QEMU's implementation of
readline is already wonky.

There might be other situations in the future where we'd like to have a
mapping of parameter to user-friendly parameter name, in which case I
fear we'd probably just reintroduce the enum.

>> From a development and maintenance perspective, having a list of
>> parameters explicitly written in several parts of the code brings
>> several annoyances: conflicts during rebase, multiple extra hits when
>> grepping, requires contributors to search for every location a change
>> needs to be mirrored to, etc.
>>
>> Remove the units from the output so we can write this code in a more
>> convenient way. The HMP output is not part of any ABI.
>>
>> Also remove quotes from around the TLS options strings as this is
>> inconsistent with all the other strings.
>>
>> Change block-bitmap-mapping format to a single line. This requires
>> updating one of the iotests to match.
>>
>> Before:                                      After:
>>                      (unchanged entries omitted)
>> announce-initial: 50 ms                      announce-initial: 50
>> announce-max: 550 ms                         announce-max: 550
>> announce-rounds: 5                           announce-rounds: 5
>> announce-step: 100 ms                        announce-step: 100
>> tls-creds: ''                                tls-creds:
>> tls-hostname: ''                             tls-hostname:
>> tls-authz: ''                                tls-authz:
>> max-bandwidth: 134217728 bytes/second        max-bandwidth: 134217728
>> avail-switchover-bandwidth: 0 bytes/second   avail-switchover-bandwidth: 0
>> max-postcopy-bandwidth: 0 bytes/second       max-postcopy-bandwidth: 0
>> downtime-limit: 300 ms                       downtime-limit: 300
>> x-checkpoint-delay: 20000 ms                 x-checkpoint-delay: 20000
>> xbzrle-cache-size: 67108864 bytes            xbzrle-cache-size: 67108864
>> x-vcpu-dirty-limit-period: 1000 ms           x-vcpu-dirty-limit-period: 1000
>> vcpu-dirty-limit: 1 MB/s                     vcpu-dirty-limit: 1
>> x-rdma-chunk-size: 1048576 bytes             x-rdma-chunk-size: 1048576
>> block-bitmap-mapping:                        block-bitmap-mapping: bitmaps: name: bmap0 alias: bmap0 node-name: node-src alias: node-dst
>>   'node-src' -> 'node-dst'
>>     'bmap0' -> 'bmap0'
>
> Uh, the value of block-bitmap-mapping can become really long.  Its QAPI
> type is array of BitmapMigrationNodeAlias, and each array element
> contains another array.
>
> Why is this change useful?
>

Not so much useful as required, we can't have generic printing of QDict
and QList if block-bitmap-mapping needs custom arrows and spacing.

I can try to tweak hmp_migrate_print_qobject() in patch 14 to add some
newlines where appropriate.

>> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH v2 12/18] migration: Change HMP 'info migrate_parameters' output

[Markus Armbruster]

Fabiano Rosas <farosas@suse.de> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Fabiano Rosas <farosas@suse.de> writes:
>>
>>> The output of 'info migrate_parameters' includes units of measurement
>>> for a few parameters. This is convenient for a user.
>>
>> Yes.
>>
>>>                                                      It also requires
>>> every parameter to be individually listed in the
>>> hmp_migrate_set_parameter() function, which in turn requires the
>>> MigrationParameter (singular) enum to exist.
>>
>> I think I understand what you mean, but your text doesn't express it
>> clearly.
>>
>> How does having in "info migrate" imply the need for parameter-specific
>> code in "migrate_set_parameter"?  Perhaps with the (unstated)
>> requirement that "migrate_set_parameter" must take values in the units
>> shown by "info migrate_parameters"?
>>
>> Let me try to explain the why differently.
>>
>> hmp_info_migrate_parameters() and hmp_migrate_set_parameter() both have
>> code for each parameter, and they both use enum MigrationParameter.
>>
>> You want to replace this parameter-specific code by code that works for
>> any parameter, in both functions.
>>
>> Your new code really doesn't want to do special per-parameter stuff.
>> That's why you want to get rid of all that.
>>
>
> Right, you got the point, it's that having the units requires custom
> formatting which needs to be per-parameter and therefore requires the
> parameters to be enumerated in the code which increases the maintenance
> burden. I'll update the commit message with something more clear.
>
>> You split the work as follows:
>>
>> * Get rid of special units in migrate_set_parameter [previous patch].
>>   Interface change, simple patch.
>>
>
> This one is separate because it affects how the user _sets_ those
> values, while the rest of the changes only affects how the user _sees_
> the values.
>
>> * Don't show units in info migrate_parameters [this patch].
>>   Interface change, simple patch.
>>
>> * Replace parameter-specific code [next patches].  More interesting, but
>>   no interface change.
>>
>> I like this split, it helps reviewers.
>>
>>>                                              While the latter is not
>>> bothersome at all, the former is.
>>
>> The values of enum MigrationParameter duplicate the members of struct
>> MigrationParameters.  That's plenty bothersome, isn't it?
>>
>
> As Peter pointed out in v1, this is now less bothersome than it used to
> be, since we stopped requiring documentation for MigrationParameter in
> commit 2220c2b9ef (qapi/migration: Don't document MigrationParameter,
> 2025-12-15).

"Less bothersome than it was" I can buy, "not at all bothersome" I
won't.

>> As far as I can tell, the only remaining uses of enum MigrationParameter
>> at the end of the series are an assertion in
>> migrate_mark_all_params_present(), which we discussed in review of v1,
>> and hmp_completion_single() in qtest/migration/misc-tests.c.  Any chance
>> we can get rid of it entirely?
>>
>
> We need something that ensures migrate_mark_all_params_present() will be
> updated when a new parameter is added. My attempt in v1 of converting to
> MigrationParameters to QDict didn't work because the has_ fields are not
> carried over to the QDict. At the moment, the best I can do is to use
> the enum.

Avoids "must keep has_fields[] in migrate_mark_all_params_present()
consistent with MigrationParameters" by "must keep MigrationParameter
consistent with MigrationParameters".  Not quite moving around the deck
chairs, because the latter is within the same file, but close.

Here's an idea for a clean solution: have a special input visitor with a
visit_optional() that always says yes.  This sets all the has_FOO.
Whether this can be made to work and is less of a maintenance burden
than MigrationParameter is left as an exercise to the reader :)

> As for the test, we could simply not test the completion of
> parameters. It's not the end of the world, QEMU's implementation of
> readline is already wonky.
>
> There might be other situations in the future where we'd like to have a
> mapping of parameter to user-friendly parameter name, in which case I
> fear we'd probably just reintroduce the enum.

It was a mistake the first time, and it will likely be a mistake the
second time.

>>> From a development and maintenance perspective, having a list of
>>> parameters explicitly written in several parts of the code brings
>>> several annoyances: conflicts during rebase, multiple extra hits when
>>> grepping, requires contributors to search for every location a change
>>> needs to be mirrored to, etc.
>>>
>>> Remove the units from the output so we can write this code in a more
>>> convenient way. The HMP output is not part of any ABI.
>>>
>>> Also remove quotes from around the TLS options strings as this is
>>> inconsistent with all the other strings.
>>>
>>> Change block-bitmap-mapping format to a single line. This requires
>>> updating one of the iotests to match.
>>>
>>> Before:                                      After:
>>>                      (unchanged entries omitted)
>>> announce-initial: 50 ms                      announce-initial: 50
>>> announce-max: 550 ms                         announce-max: 550
>>> announce-rounds: 5                           announce-rounds: 5
>>> announce-step: 100 ms                        announce-step: 100
>>> tls-creds: ''                                tls-creds:
>>> tls-hostname: ''                             tls-hostname:
>>> tls-authz: ''                                tls-authz:
>>> max-bandwidth: 134217728 bytes/second        max-bandwidth: 134217728
>>> avail-switchover-bandwidth: 0 bytes/second   avail-switchover-bandwidth: 0
>>> max-postcopy-bandwidth: 0 bytes/second       max-postcopy-bandwidth: 0
>>> downtime-limit: 300 ms                       downtime-limit: 300
>>> x-checkpoint-delay: 20000 ms                 x-checkpoint-delay: 20000
>>> xbzrle-cache-size: 67108864 bytes            xbzrle-cache-size: 67108864
>>> x-vcpu-dirty-limit-period: 1000 ms           x-vcpu-dirty-limit-period: 1000
>>> vcpu-dirty-limit: 1 MB/s                     vcpu-dirty-limit: 1
>>> x-rdma-chunk-size: 1048576 bytes             x-rdma-chunk-size: 1048576
>>> block-bitmap-mapping:                        block-bitmap-mapping: bitmaps: name: bmap0 alias: bmap0 node-name: node-src alias: node-dst
>>>   'node-src' -> 'node-dst'
>>>     'bmap0' -> 'bmap0'
>>
>> Uh, the value of block-bitmap-mapping can become really long.  Its QAPI
>> type is array of BitmapMigrationNodeAlias, and each array element
>> contains another array.
>>
>> Why is this change useful?
>>
>
> Not so much useful as required, we can't have generic printing of QDict
> and QList if block-bitmap-mapping needs custom arrows and spacing.
>
> I can try to tweak hmp_migrate_print_qobject() in patch 14 to add some
> newlines where appropriate.

JSON pretty-printers can do this, and I'm confident you can do this,
too.

>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>

[PATCH v2 13/18] migration: Use keyval input visitor in HMP set command

[Fabiano Rosas]

Change the hmp_migrate_set_parameter command to use a keyval input
visitor.

Currently a string visitor is used and due to limitations of that
particular visitor's implementation it's necessary to consult the QAPI
type enum (MigrationParameter_lookup) and call each visit_type_*
function individually. Which makes using a visitor pointless.

Since there are other visitors implemented properly and generated code
to iterate the QAPI object, prefer using one of those. The keyval
input visitor is adequate because HMP provides basically one key and
one value for each migrate_set_parameter command.

To switch from string_input_visitor to keyval_input_visitor simply put
the parameter name and value into a dict and invoke
visit_type_MigrationParameters().

Note that it's not necessary to go through any of the keyval_* code
because due to the nature of HMP, there's no parsing to do (no '=', no
',', etc).

With this the migrate_set_parameters HMP commands will be
automatically updated anytime a new migration parameter is added.

One parameter, "cpr-exec-command", takes the strList type which needs
to be built manually. This moves to a "legacy" suffixed function.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration-hmp-cmds.c     | 195 +++++++----------------------
 tests/qtest/migration/misc-tests.c |   2 +-
 2 files changed, 49 insertions(+), 148 deletions(-)

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 5d754414c4..0d93d38b05 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -25,7 +25,9 @@
 #include "qapi/error.h"
 #include "qapi/qapi-commands-migration.h"
 #include "qapi/qapi-visit-migration.h"
+#include "qapi/qobject-input-visitor.h"
 #include "qobject/qdict.h"
+#include "qobject/qstring.h"
 #include "qapi/string-input-visitor.h"
 #include "qapi/string-output-visitor.h"
 #include "qemu/cutils.h"
@@ -600,154 +602,14 @@ end:
     hmp_handle_error(hmp, err);
 }
 
-void hmp_migrate_set_parameter(MonitorHMP *hmp, const QDict *qdict)
+static void hmp_migrate_set_parameter_legacy(MonitorHMP *hmp, const QDict *qdict)
 {
     const char *param = qdict_get_str(qdict, "parameter");
     const char *valuestr = qdict_get_str(qdict, "value");
-    Visitor *v = string_input_visitor_new(valuestr);
     MigrationParameters *p = g_new0(MigrationParameters, 1);
-    uint64_t cache_size;
     Error *err = NULL;
-    int val;
 
-    val = qapi_enum_parse(&MigrationParameter_lookup, param, -1, &err);
-    if (val < 0) {
-        goto cleanup;
-    }
-
-    switch (val) {
-    case MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD:
-        p->has_throttle_trigger_threshold = true;
-        visit_type_uint8(v, param, &p->throttle_trigger_threshold, &err);
-        break;
-    case MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL:
-        p->has_cpu_throttle_initial = true;
-        visit_type_uint8(v, param, &p->cpu_throttle_initial, &err);
-        break;
-    case MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT:
-        p->has_cpu_throttle_increment = true;
-        visit_type_uint8(v, param, &p->cpu_throttle_increment, &err);
-        break;
-    case MIGRATION_PARAMETER_CPU_THROTTLE_TAILSLOW:
-        p->has_cpu_throttle_tailslow = true;
-        visit_type_bool(v, param, &p->cpu_throttle_tailslow, &err);
-        break;
-    case MIGRATION_PARAMETER_MAX_CPU_THROTTLE:
-        p->has_max_cpu_throttle = true;
-        visit_type_uint8(v, param, &p->max_cpu_throttle, &err);
-        break;
-    case MIGRATION_PARAMETER_TLS_CREDS:
-        p->tls_creds = g_new0(StrOrNull, 1);
-        p->tls_creds->type = QTYPE_QSTRING;
-        visit_type_str(v, param, &p->tls_creds->u.s, &err);
-        break;
-    case MIGRATION_PARAMETER_TLS_HOSTNAME:
-        p->tls_hostname = g_new0(StrOrNull, 1);
-        p->tls_hostname->type = QTYPE_QSTRING;
-        visit_type_str(v, param, &p->tls_hostname->u.s, &err);
-        break;
-    case MIGRATION_PARAMETER_TLS_AUTHZ:
-        p->tls_authz = g_new0(StrOrNull, 1);
-        p->tls_authz->type = QTYPE_QSTRING;
-        visit_type_str(v, param, &p->tls_authz->u.s, &err);
-        break;
-    case MIGRATION_PARAMETER_MAX_BANDWIDTH:
-        p->has_max_bandwidth = true;
-        visit_type_size(v, param, &p->max_bandwidth, &err);
-        break;
-    case MIGRATION_PARAMETER_AVAIL_SWITCHOVER_BANDWIDTH:
-        p->has_avail_switchover_bandwidth = true;
-        visit_type_size(v, param, &p->avail_switchover_bandwidth, &err);
-        break;
-    case MIGRATION_PARAMETER_DOWNTIME_LIMIT:
-        p->has_downtime_limit = true;
-        visit_type_size(v, param, &p->downtime_limit, &err);
-        break;
-    case MIGRATION_PARAMETER_X_CHECKPOINT_DELAY:
-        p->has_x_checkpoint_delay = true;
-        visit_type_uint32(v, param, &p->x_checkpoint_delay, &err);
-        break;
-    case MIGRATION_PARAMETER_MULTIFD_CHANNELS:
-        p->has_multifd_channels = true;
-        visit_type_uint8(v, param, &p->multifd_channels, &err);
-        break;
-    case MIGRATION_PARAMETER_MULTIFD_COMPRESSION:
-        p->has_multifd_compression = true;
-        visit_type_MultiFDCompression(v, param, &p->multifd_compression,
-                                      &err);
-        break;
-    case MIGRATION_PARAMETER_MULTIFD_ZLIB_LEVEL:
-        p->has_multifd_zlib_level = true;
-        visit_type_uint8(v, param, &p->multifd_zlib_level, &err);
-        break;
-    case MIGRATION_PARAMETER_MULTIFD_QATZIP_LEVEL:
-        p->has_multifd_qatzip_level = true;
-        visit_type_uint8(v, param, &p->multifd_qatzip_level, &err);
-        break;
-    case MIGRATION_PARAMETER_MULTIFD_ZSTD_LEVEL:
-        p->has_multifd_zstd_level = true;
-        visit_type_uint8(v, param, &p->multifd_zstd_level, &err);
-        break;
-    case MIGRATION_PARAMETER_ZERO_PAGE_DETECTION:
-        p->has_zero_page_detection = true;
-        visit_type_ZeroPageDetection(v, param, &p->zero_page_detection, &err);
-        break;
-    case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE:
-        p->has_xbzrle_cache_size = true;
-        if (!visit_type_size(v, param, &cache_size, &err)) {
-            break;
-        }
-        if (cache_size > INT64_MAX || (size_t)cache_size != cache_size) {
-            error_setg(&err, "Invalid size %s", valuestr);
-            break;
-        }
-        p->xbzrle_cache_size = cache_size;
-        break;
-    case MIGRATION_PARAMETER_MAX_POSTCOPY_BANDWIDTH:
-        p->has_max_postcopy_bandwidth = true;
-        visit_type_size(v, param, &p->max_postcopy_bandwidth, &err);
-        break;
-    case MIGRATION_PARAMETER_ANNOUNCE_INITIAL:
-        p->has_announce_initial = true;
-        visit_type_size(v, param, &p->announce_initial, &err);
-        break;
-    case MIGRATION_PARAMETER_ANNOUNCE_MAX:
-        p->has_announce_max = true;
-        visit_type_size(v, param, &p->announce_max, &err);
-        break;
-    case MIGRATION_PARAMETER_ANNOUNCE_ROUNDS:
-        p->has_announce_rounds = true;
-        visit_type_size(v, param, &p->announce_rounds, &err);
-        break;
-    case MIGRATION_PARAMETER_ANNOUNCE_STEP:
-        p->has_announce_step = true;
-        visit_type_size(v, param, &p->announce_step, &err);
-        break;
-    case MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING:
-        error_setg(&err, "The block-bitmap-mapping parameter can only be set "
-                   "through QMP");
-        break;
-    case MIGRATION_PARAMETER_X_VCPU_DIRTY_LIMIT_PERIOD:
-        p->has_x_vcpu_dirty_limit_period = true;
-        visit_type_size(v, param, &p->x_vcpu_dirty_limit_period, &err);
-        break;
-    case MIGRATION_PARAMETER_VCPU_DIRTY_LIMIT:
-        p->has_vcpu_dirty_limit = true;
-        visit_type_size(v, param, &p->vcpu_dirty_limit, &err);
-        break;
-    case MIGRATION_PARAMETER_MODE:
-        p->has_mode = true;
-        visit_type_MigMode(v, param, &p->mode, &err);
-        break;
-    case MIGRATION_PARAMETER_DIRECT_IO:
-        p->has_direct_io = true;
-        visit_type_bool(v, param, &p->direct_io, &err);
-        break;
-    case MIGRATION_PARAMETER_X_RDMA_CHUNK_SIZE:
-        p->has_x_rdma_chunk_size = true;
-        visit_type_size(v, param, &p->x_rdma_chunk_size, &err);
-        break;
-    case MIGRATION_PARAMETER_CPR_EXEC_COMMAND: {
+    if (g_str_equal(param, "cpr-exec-command")) {
         /*
          * NOTE: g_autofree will only auto g_free() the strv array when
          * needed, it will not free the strings within the array. It's
@@ -760,15 +622,14 @@ void hmp_migrate_set_parameter(MonitorHMP *hmp, const QDict *qdict)
 
         if (!g_shell_parse_argv(valuestr, NULL, &strv, &gerr)) {
             error_setg(&err, "%s", gerr->message);
-            break;
+            goto cleanup;
         }
         for (int i = 0; strv[i]; i++) {
             QAPI_LIST_APPEND(tail, strv[i]);
         }
         p->has_cpr_exec_command = true;
-        break;
-    }
-    default:
+
+    } else {
         g_assert_not_reached();
     }
 
@@ -778,12 +639,52 @@ void hmp_migrate_set_parameter(MonitorHMP *hmp, const QDict *qdict)
 
     qmp_migrate_set_parameters(p, &err);
 
- cleanup:
+cleanup:
     qapi_free_MigrationParameters(p);
+    hmp_handle_error(hmp, err);
+}
+
+static void hmp_migrate_set_parameter_qapi(MonitorHMP *hmp, const QDict *qdict)
+{
+    const char *param = qdict_get_str(qdict, "parameter");
+    const char *valuestr = qdict_get_str(qdict, "value");
+    g_autoptr(QDict) input = qdict_new();
+    g_autoptr(MigrationParameters) p = NULL;
+    Visitor *v;
+    Error *err = NULL;
+
+    /* the same as keyval_parse(), but here there's no need to parse */
+    qdict_put_obj(input, param, QOBJECT(qstring_from_str(valuestr)));
+
+    v = qobject_input_visitor_new_keyval(QOBJECT(input));
+    if (visit_type_MigrationParameters(v, NULL, &p, &err)) {
+        qmp_migrate_set_parameters(p, &err);
+    }
+
     visit_free(v);
     hmp_handle_error(hmp, err);
 }
 
+void hmp_migrate_set_parameter(MonitorHMP *hmp, const QDict *qdict)
+{
+    const char *param = qdict_get_str(qdict, "parameter");
+
+    if (g_str_equal(param, "block-bitmap-mapping")) {
+        Error *err = NULL;
+
+        error_setg(&err, "The %s parameter can only be set through QMP", param);
+        hmp_handle_error(hmp, err);
+        return;
+    }
+
+    /* this has a non-standard setter */
+    if (g_str_equal(param, "cpr-exec-command")) {
+        return hmp_migrate_set_parameter_legacy(hmp, qdict);
+    }
+
+    hmp_migrate_set_parameter_qapi(hmp, qdict);
+}
+
 void hmp_migrate_start_postcopy(MonitorHMP *hmp, const QDict *qdict)
 {
     Error *err = NULL;
diff --git a/tests/qtest/migration/misc-tests.c b/tests/qtest/migration/misc-tests.c
index 2261ae7c89..4ac2f42a5a 100644
--- a/tests/qtest/migration/misc-tests.c
+++ b/tests/qtest/migration/misc-tests.c
@@ -50,7 +50,7 @@ typedef struct HMPTestData {
 HMPTestData test_cases[] = {
     TEST("", "", "migrate_set_parameter: string expected"),
     TEST("foo", "", "migrate_set_parameter: string expected"),
-    TEST("foo", "on", "Error: invalid parameter value: foo"),
+    TEST("foo", "on", "Error: Parameter 'foo' is unexpected"),
 
     /* bool */
     TEST("cpu-throttle-tailslow", "on", "on"),
-- 
2.53.0

Re: [PATCH v2 13/18] migration: Use keyval input visitor in HMP set command

[Markus Armbruster]

Fabiano Rosas <farosas@suse.de> writes:

> Change the hmp_migrate_set_parameter command to use a keyval input
> visitor.
>
> Currently a string visitor is used and due to limitations of that
> particular visitor's implementation it's necessary to consult the QAPI
> type enum (MigrationParameter_lookup) and call each visit_type_*
> function individually. Which makes using a visitor pointless.

Also, the less the string visitors are used, the happier I am.

> Since there are other visitors implemented properly and generated code
> to iterate the QAPI object, prefer using one of those. The keyval
> input visitor is adequate because HMP provides basically one key and
> one value for each migrate_set_parameter command.
>
> To switch from string_input_visitor to keyval_input_visitor simply put
> the parameter name and value into a dict and invoke
> visit_type_MigrationParameters().

This works for scalar types: the value is a QString, and the QObject
keyval input visitor automatically converts to the C type the visitor
expects.  It doesn't work for non-scalar types; see cpr-exec-command
below.

> Note that it's not necessary to go through any of the keyval_* code
> because due to the nature of HMP, there's no parsing to do (no '=', no
> ',', etc).

migrate_set_parameter syntax isn't keyval, only val is, i.e. its value
argument is in keyval value syntax (more or less).

"More or less" is my hedge against differences between the string input
visitor and the QObject keyval input visitor.  Did you check?

Aside: we could create migrate_set_parameters with keyval syntax if we
cared.

> With this the migrate_set_parameters HMP commands will be
> automatically updated anytime a new migration parameter is added.
>
> One parameter, "cpr-exec-command", takes the strList type which needs
> to be built manually. This moves to a "legacy" suffixed function.

When visit_type_MigrationParameters() visits "cpr-exec-command", it
calls visit_type_strList().  With the QObject keyval input visitor, this
expects a QList, not a QString.

Thus, hmp_migrate_set_parameter() needs to parse the value argument into
a list at least.  That's why it needs to be a special case.

It parses it with g_shell_parse_argv().  GLib docs "specify" this to
parse "a command line [...] in much the same way the shell would, but
without many of the expansions the shell would perform (variable
expansion, globs, operators, filename expansion, etc. are not
supported)."  Ugh!  But I digress.

> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/migration-hmp-cmds.c     | 195 +++++++----------------------
>  tests/qtest/migration/misc-tests.c |   2 +-
>  2 files changed, 49 insertions(+), 148 deletions(-)
>
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index 5d754414c4..0d93d38b05 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -25,7 +25,9 @@
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-migration.h"
>  #include "qapi/qapi-visit-migration.h"
> +#include "qapi/qobject-input-visitor.h"
>  #include "qobject/qdict.h"
> +#include "qobject/qstring.h"
>  #include "qapi/string-input-visitor.h"
>  #include "qapi/string-output-visitor.h"
>  #include "qemu/cutils.h"
> @@ -600,154 +602,14 @@ end:
>      hmp_handle_error(hmp, err);
>  }
>  
> -void hmp_migrate_set_parameter(MonitorHMP *hmp, const QDict *qdict)
> +static void hmp_migrate_set_parameter_legacy(MonitorHMP *hmp, const QDict *qdict)
>  {
>      const char *param = qdict_get_str(qdict, "parameter");
>      const char *valuestr = qdict_get_str(qdict, "value");
> -    Visitor *v = string_input_visitor_new(valuestr);
>      MigrationParameters *p = g_new0(MigrationParameters, 1);
> -    uint64_t cache_size;
>      Error *err = NULL;
> -    int val;
>  
> -    val = qapi_enum_parse(&MigrationParameter_lookup, param, -1, &err);
> -    if (val < 0) {
> -        goto cleanup;
> -    }
> -
> -    switch (val) {
> -    case MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD:
> -        p->has_throttle_trigger_threshold = true;
> -        visit_type_uint8(v, param, &p->throttle_trigger_threshold, &err);
> -        break;
> -    case MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL:
> -        p->has_cpu_throttle_initial = true;
> -        visit_type_uint8(v, param, &p->cpu_throttle_initial, &err);
> -        break;
> -    case MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT:
> -        p->has_cpu_throttle_increment = true;
> -        visit_type_uint8(v, param, &p->cpu_throttle_increment, &err);
> -        break;
> -    case MIGRATION_PARAMETER_CPU_THROTTLE_TAILSLOW:
> -        p->has_cpu_throttle_tailslow = true;
> -        visit_type_bool(v, param, &p->cpu_throttle_tailslow, &err);
> -        break;
> -    case MIGRATION_PARAMETER_MAX_CPU_THROTTLE:
> -        p->has_max_cpu_throttle = true;
> -        visit_type_uint8(v, param, &p->max_cpu_throttle, &err);
> -        break;
> -    case MIGRATION_PARAMETER_TLS_CREDS:
> -        p->tls_creds = g_new0(StrOrNull, 1);
> -        p->tls_creds->type = QTYPE_QSTRING;
> -        visit_type_str(v, param, &p->tls_creds->u.s, &err);
> -        break;
> -    case MIGRATION_PARAMETER_TLS_HOSTNAME:
> -        p->tls_hostname = g_new0(StrOrNull, 1);
> -        p->tls_hostname->type = QTYPE_QSTRING;
> -        visit_type_str(v, param, &p->tls_hostname->u.s, &err);
> -        break;
> -    case MIGRATION_PARAMETER_TLS_AUTHZ:
> -        p->tls_authz = g_new0(StrOrNull, 1);
> -        p->tls_authz->type = QTYPE_QSTRING;
> -        visit_type_str(v, param, &p->tls_authz->u.s, &err);
> -        break;
> -    case MIGRATION_PARAMETER_MAX_BANDWIDTH:
> -        p->has_max_bandwidth = true;
> -        visit_type_size(v, param, &p->max_bandwidth, &err);
> -        break;
> -    case MIGRATION_PARAMETER_AVAIL_SWITCHOVER_BANDWIDTH:
> -        p->has_avail_switchover_bandwidth = true;
> -        visit_type_size(v, param, &p->avail_switchover_bandwidth, &err);
> -        break;
> -    case MIGRATION_PARAMETER_DOWNTIME_LIMIT:
> -        p->has_downtime_limit = true;
> -        visit_type_size(v, param, &p->downtime_limit, &err);
> -        break;
> -    case MIGRATION_PARAMETER_X_CHECKPOINT_DELAY:
> -        p->has_x_checkpoint_delay = true;
> -        visit_type_uint32(v, param, &p->x_checkpoint_delay, &err);
> -        break;
> -    case MIGRATION_PARAMETER_MULTIFD_CHANNELS:
> -        p->has_multifd_channels = true;
> -        visit_type_uint8(v, param, &p->multifd_channels, &err);
> -        break;
> -    case MIGRATION_PARAMETER_MULTIFD_COMPRESSION:
> -        p->has_multifd_compression = true;
> -        visit_type_MultiFDCompression(v, param, &p->multifd_compression,
> -                                      &err);
> -        break;
> -    case MIGRATION_PARAMETER_MULTIFD_ZLIB_LEVEL:
> -        p->has_multifd_zlib_level = true;
> -        visit_type_uint8(v, param, &p->multifd_zlib_level, &err);
> -        break;
> -    case MIGRATION_PARAMETER_MULTIFD_QATZIP_LEVEL:
> -        p->has_multifd_qatzip_level = true;
> -        visit_type_uint8(v, param, &p->multifd_qatzip_level, &err);
> -        break;
> -    case MIGRATION_PARAMETER_MULTIFD_ZSTD_LEVEL:
> -        p->has_multifd_zstd_level = true;
> -        visit_type_uint8(v, param, &p->multifd_zstd_level, &err);
> -        break;
> -    case MIGRATION_PARAMETER_ZERO_PAGE_DETECTION:
> -        p->has_zero_page_detection = true;
> -        visit_type_ZeroPageDetection(v, param, &p->zero_page_detection, &err);
> -        break;
> -    case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE:
> -        p->has_xbzrle_cache_size = true;
> -        if (!visit_type_size(v, param, &cache_size, &err)) {
> -            break;
> -        }
> -        if (cache_size > INT64_MAX || (size_t)cache_size != cache_size) {
> -            error_setg(&err, "Invalid size %s", valuestr);
> -            break;
> -        }
> -        p->xbzrle_cache_size = cache_size;
> -        break;
> -    case MIGRATION_PARAMETER_MAX_POSTCOPY_BANDWIDTH:
> -        p->has_max_postcopy_bandwidth = true;
> -        visit_type_size(v, param, &p->max_postcopy_bandwidth, &err);
> -        break;
> -    case MIGRATION_PARAMETER_ANNOUNCE_INITIAL:
> -        p->has_announce_initial = true;
> -        visit_type_size(v, param, &p->announce_initial, &err);
> -        break;
> -    case MIGRATION_PARAMETER_ANNOUNCE_MAX:
> -        p->has_announce_max = true;
> -        visit_type_size(v, param, &p->announce_max, &err);
> -        break;
> -    case MIGRATION_PARAMETER_ANNOUNCE_ROUNDS:
> -        p->has_announce_rounds = true;
> -        visit_type_size(v, param, &p->announce_rounds, &err);
> -        break;
> -    case MIGRATION_PARAMETER_ANNOUNCE_STEP:
> -        p->has_announce_step = true;
> -        visit_type_size(v, param, &p->announce_step, &err);
> -        break;
> -    case MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING:
> -        error_setg(&err, "The block-bitmap-mapping parameter can only be set "
> -                   "through QMP");
> -        break;
> -    case MIGRATION_PARAMETER_X_VCPU_DIRTY_LIMIT_PERIOD:
> -        p->has_x_vcpu_dirty_limit_period = true;
> -        visit_type_size(v, param, &p->x_vcpu_dirty_limit_period, &err);
> -        break;
> -    case MIGRATION_PARAMETER_VCPU_DIRTY_LIMIT:
> -        p->has_vcpu_dirty_limit = true;
> -        visit_type_size(v, param, &p->vcpu_dirty_limit, &err);
> -        break;
> -    case MIGRATION_PARAMETER_MODE:
> -        p->has_mode = true;
> -        visit_type_MigMode(v, param, &p->mode, &err);
> -        break;
> -    case MIGRATION_PARAMETER_DIRECT_IO:
> -        p->has_direct_io = true;
> -        visit_type_bool(v, param, &p->direct_io, &err);
> -        break;
> -    case MIGRATION_PARAMETER_X_RDMA_CHUNK_SIZE:
> -        p->has_x_rdma_chunk_size = true;
> -        visit_type_size(v, param, &p->x_rdma_chunk_size, &err);
> -        break;
> -    case MIGRATION_PARAMETER_CPR_EXEC_COMMAND: {
> +    if (g_str_equal(param, "cpr-exec-command")) {
>          /*
>           * NOTE: g_autofree will only auto g_free() the strv array when
>           * needed, it will not free the strings within the array. It's
> @@ -760,15 +622,14 @@ void hmp_migrate_set_parameter(MonitorHMP *hmp, const QDict *qdict)
>  
>          if (!g_shell_parse_argv(valuestr, NULL, &strv, &gerr)) {
>              error_setg(&err, "%s", gerr->message);
> -            break;
> +            goto cleanup;
>          }
>          for (int i = 0; strv[i]; i++) {
>              QAPI_LIST_APPEND(tail, strv[i]);
>          }
>          p->has_cpr_exec_command = true;
> -        break;
> -    }
> -    default:
> +
> +    } else {
>          g_assert_not_reached();
>      }
>  
> @@ -778,12 +639,52 @@ void hmp_migrate_set_parameter(MonitorHMP *hmp, const QDict *qdict)
>  
>      qmp_migrate_set_parameters(p, &err);
>  
> - cleanup:
> +cleanup:
>      qapi_free_MigrationParameters(p);
> +    hmp_handle_error(hmp, err);
> +}
> +
> +static void hmp_migrate_set_parameter_qapi(MonitorHMP *hmp, const QDict *qdict)
> +{
> +    const char *param = qdict_get_str(qdict, "parameter");
> +    const char *valuestr = qdict_get_str(qdict, "value");
> +    g_autoptr(QDict) input = qdict_new();
> +    g_autoptr(MigrationParameters) p = NULL;
> +    Visitor *v;
> +    Error *err = NULL;
> +
> +    /* the same as keyval_parse(), but here there's no need to parse */
> +    qdict_put_obj(input, param, QOBJECT(qstring_from_str(valuestr)));
> +
> +    v = qobject_input_visitor_new_keyval(QOBJECT(input));
> +    if (visit_type_MigrationParameters(v, NULL, &p, &err)) {
> +        qmp_migrate_set_parameters(p, &err);
> +    }
> +
>      visit_free(v);
>      hmp_handle_error(hmp, err);
>  }
>  
> +void hmp_migrate_set_parameter(MonitorHMP *hmp, const QDict *qdict)
> +{
> +    const char *param = qdict_get_str(qdict, "parameter");
> +
> +    if (g_str_equal(param, "block-bitmap-mapping")) {
> +        Error *err = NULL;
> +
> +        error_setg(&err, "The %s parameter can only be set through QMP", param);
> +        hmp_handle_error(hmp, err);

I think a hmp_report_error(MonitorHMP *hmp, const char *fmt, ...) would
be nice to have.

> +        return;
> +    }
> +
> +    /* this has a non-standard setter */
> +    if (g_str_equal(param, "cpr-exec-command")) {

We check for "cpr-exec-command" in two places, here and in
hmp_migrate_set_parameter().  A bit awkward.

> +        return hmp_migrate_set_parameter_legacy(hmp, qdict);
> +    }
> +
> +    hmp_migrate_set_parameter_qapi(hmp, qdict);
> +}

I think I'd do this differently.

Recall how we normally parse text into a C object:

1. Parse the string into a QObject.

2. Wrap it in the appropriate visitor.

3. Convert it into a C object with visit_type_TYPE().

For QMP, step 1 uses json_message_parser_init() & friends, and step 2
is qobject_input_visitor_new().

For the command line, step 1 and 2 are commonly
qobject_input_visitor_new_str(), which either uses qobject_from_json()
and qobject_input_visitor_new(), or keyval_parse() and
qobject_input_visitor_new_keyval().

hmp_migrate_set_parameter() could do step 1 and 2 like this:

    // step 1
    if param == "block-bitmap-mapping"
        report error and return
    if param == "cpr-exec-command"
        value = string parsed into QList
    else // scalars
        value = string
    input = {param: value}
    // step 2
    v = qobject_input_visitor_new_keyval(input)

> +
>  void hmp_migrate_start_postcopy(MonitorHMP *hmp, const QDict *qdict)
>  {
>      Error *err = NULL;
> diff --git a/tests/qtest/migration/misc-tests.c b/tests/qtest/migration/misc-tests.c
> index 2261ae7c89..4ac2f42a5a 100644
> --- a/tests/qtest/migration/misc-tests.c
> +++ b/tests/qtest/migration/misc-tests.c
> @@ -50,7 +50,7 @@ typedef struct HMPTestData {
>  HMPTestData test_cases[] = {
>      TEST("", "", "migrate_set_parameter: string expected"),
>      TEST("foo", "", "migrate_set_parameter: string expected"),
> -    TEST("foo", "on", "Error: invalid parameter value: foo"),
> +    TEST("foo", "on", "Error: Parameter 'foo' is unexpected"),

This appears to be an improvement.  Where does it come from?

>  
>      /* bool */
>      TEST("cpu-throttle-tailslow", "on", "on"),

Re: [PATCH v2 13/18] migration: Use keyval input visitor in HMP set command

[Fabiano Rosas]

Markus Armbruster <armbru@redhat.com> writes:

> Fabiano Rosas <farosas@suse.de> writes:
>
>> Change the hmp_migrate_set_parameter command to use a keyval input
>> visitor.
>>
>> Currently a string visitor is used and due to limitations of that
>> particular visitor's implementation it's necessary to consult the QAPI
>> type enum (MigrationParameter_lookup) and call each visit_type_*
>> function individually. Which makes using a visitor pointless.
>
> Also, the less the string visitors are used, the happier I am.
>
>> Since there are other visitors implemented properly and generated code
>> to iterate the QAPI object, prefer using one of those. The keyval
>> input visitor is adequate because HMP provides basically one key and
>> one value for each migrate_set_parameter command.
>>
>> To switch from string_input_visitor to keyval_input_visitor simply put
>> the parameter name and value into a dict and invoke
>> visit_type_MigrationParameters().
>
> This works for scalar types: the value is a QString, and the QObject
> keyval input visitor automatically converts to the C type the visitor
> expects.  It doesn't work for non-scalar types; see cpr-exec-command
> below.
>
>> Note that it's not necessary to go through any of the keyval_* code
>> because due to the nature of HMP, there's no parsing to do (no '=', no
>> ',', etc).
>
> migrate_set_parameter syntax isn't keyval, only val is, i.e. its value
> argument is in keyval value syntax (more or less).
>
> "More or less" is my hedge against differences between the string input
> visitor and the QObject keyval input visitor.  Did you check?
>

They ultimately use the same functions to do the conversion
(qemu_strtou64, qemu_strtod_finite, etc). The differences are in
slightly different wording of error messages and the fact that the
string input visitor has a different message for -ERANGE while the
qobject input visitor has a TODO for that case.

I think those differences are of no consequence. The error messages that
we really care about are the ones resulting from the migration code
validation (because they're informative to the user). The API level
messages are too generic anyway.

> Aside: we could create migrate_set_parameters with keyval syntax if we
> cared.
>
>> With this the migrate_set_parameters HMP commands will be
>> automatically updated anytime a new migration parameter is added.
>>
>> One parameter, "cpr-exec-command", takes the strList type which needs
>> to be built manually. This moves to a "legacy" suffixed function.
>
> When visit_type_MigrationParameters() visits "cpr-exec-command", it
> calls visit_type_strList().  With the QObject keyval input visitor, this
> expects a QList, not a QString.
>
> Thus, hmp_migrate_set_parameter() needs to parse the value argument into
> a list at least.  That's why it needs to be a special case.
>

I'm having some difficulty understanding why isn't that the job of the
visit function.

I think what you're saying is that we can convert QAPI's strList into
QObject's QList of QString, but cannot convert the HMP string directly
into QList even though we know that string represents a strList. I feel
like there's code missing somewhere...

... although it wouldn't help with cpr-exec-command because of the
"shell parsing" semantics. But that's another issue.

> It parses it with g_shell_parse_argv().  GLib docs "specify" this to
> parse "a command line [...] in much the same way the shell would, but
> without many of the expansions the shell would perform (variable
> expansion, globs, operators, filename expansion, etc. are not
> supported)."  Ugh!  But I digress.
>

One side-effect of having code that handles parameters genericly is that
there is less room for inventing custom parsing when new parameters are
introduced.

>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/migration-hmp-cmds.c     | 195 +++++++----------------------
>>  tests/qtest/migration/misc-tests.c |   2 +-
>>  2 files changed, 49 insertions(+), 148 deletions(-)
>>
>> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
>> index 5d754414c4..0d93d38b05 100644
>> --- a/migration/migration-hmp-cmds.c
>> +++ b/migration/migration-hmp-cmds.c
>> @@ -25,7 +25,9 @@
>>  #include "qapi/error.h"
>>  #include "qapi/qapi-commands-migration.h"
>>  #include "qapi/qapi-visit-migration.h"
>> +#include "qapi/qobject-input-visitor.h"
>>  #include "qobject/qdict.h"
>> +#include "qobject/qstring.h"
>>  #include "qapi/string-input-visitor.h"
>>  #include "qapi/string-output-visitor.h"
>>  #include "qemu/cutils.h"
>> @@ -600,154 +602,14 @@ end:
>>      hmp_handle_error(hmp, err);
>>  }
>>  
>> -void hmp_migrate_set_parameter(MonitorHMP *hmp, const QDict *qdict)
>> +static void hmp_migrate_set_parameter_legacy(MonitorHMP *hmp, const QDict *qdict)
>>  {
>>      const char *param = qdict_get_str(qdict, "parameter");
>>      const char *valuestr = qdict_get_str(qdict, "value");
>> -    Visitor *v = string_input_visitor_new(valuestr);
>>      MigrationParameters *p = g_new0(MigrationParameters, 1);
>> -    uint64_t cache_size;
>>      Error *err = NULL;
>> -    int val;
>>  
>> -    val = qapi_enum_parse(&MigrationParameter_lookup, param, -1, &err);
>> -    if (val < 0) {
>> -        goto cleanup;
>> -    }
>> -
>> -    switch (val) {
>> -    case MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD:
>> -        p->has_throttle_trigger_threshold = true;
>> -        visit_type_uint8(v, param, &p->throttle_trigger_threshold, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL:
>> -        p->has_cpu_throttle_initial = true;
>> -        visit_type_uint8(v, param, &p->cpu_throttle_initial, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT:
>> -        p->has_cpu_throttle_increment = true;
>> -        visit_type_uint8(v, param, &p->cpu_throttle_increment, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_CPU_THROTTLE_TAILSLOW:
>> -        p->has_cpu_throttle_tailslow = true;
>> -        visit_type_bool(v, param, &p->cpu_throttle_tailslow, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_MAX_CPU_THROTTLE:
>> -        p->has_max_cpu_throttle = true;
>> -        visit_type_uint8(v, param, &p->max_cpu_throttle, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_TLS_CREDS:
>> -        p->tls_creds = g_new0(StrOrNull, 1);
>> -        p->tls_creds->type = QTYPE_QSTRING;
>> -        visit_type_str(v, param, &p->tls_creds->u.s, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_TLS_HOSTNAME:
>> -        p->tls_hostname = g_new0(StrOrNull, 1);
>> -        p->tls_hostname->type = QTYPE_QSTRING;
>> -        visit_type_str(v, param, &p->tls_hostname->u.s, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_TLS_AUTHZ:
>> -        p->tls_authz = g_new0(StrOrNull, 1);
>> -        p->tls_authz->type = QTYPE_QSTRING;
>> -        visit_type_str(v, param, &p->tls_authz->u.s, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_MAX_BANDWIDTH:
>> -        p->has_max_bandwidth = true;
>> -        visit_type_size(v, param, &p->max_bandwidth, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_AVAIL_SWITCHOVER_BANDWIDTH:
>> -        p->has_avail_switchover_bandwidth = true;
>> -        visit_type_size(v, param, &p->avail_switchover_bandwidth, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_DOWNTIME_LIMIT:
>> -        p->has_downtime_limit = true;
>> -        visit_type_size(v, param, &p->downtime_limit, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_X_CHECKPOINT_DELAY:
>> -        p->has_x_checkpoint_delay = true;
>> -        visit_type_uint32(v, param, &p->x_checkpoint_delay, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_MULTIFD_CHANNELS:
>> -        p->has_multifd_channels = true;
>> -        visit_type_uint8(v, param, &p->multifd_channels, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_MULTIFD_COMPRESSION:
>> -        p->has_multifd_compression = true;
>> -        visit_type_MultiFDCompression(v, param, &p->multifd_compression,
>> -                                      &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_MULTIFD_ZLIB_LEVEL:
>> -        p->has_multifd_zlib_level = true;
>> -        visit_type_uint8(v, param, &p->multifd_zlib_level, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_MULTIFD_QATZIP_LEVEL:
>> -        p->has_multifd_qatzip_level = true;
>> -        visit_type_uint8(v, param, &p->multifd_qatzip_level, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_MULTIFD_ZSTD_LEVEL:
>> -        p->has_multifd_zstd_level = true;
>> -        visit_type_uint8(v, param, &p->multifd_zstd_level, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_ZERO_PAGE_DETECTION:
>> -        p->has_zero_page_detection = true;
>> -        visit_type_ZeroPageDetection(v, param, &p->zero_page_detection, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE:
>> -        p->has_xbzrle_cache_size = true;
>> -        if (!visit_type_size(v, param, &cache_size, &err)) {
>> -            break;
>> -        }
>> -        if (cache_size > INT64_MAX || (size_t)cache_size != cache_size) {
>> -            error_setg(&err, "Invalid size %s", valuestr);
>> -            break;
>> -        }
>> -        p->xbzrle_cache_size = cache_size;
>> -        break;
>> -    case MIGRATION_PARAMETER_MAX_POSTCOPY_BANDWIDTH:
>> -        p->has_max_postcopy_bandwidth = true;
>> -        visit_type_size(v, param, &p->max_postcopy_bandwidth, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_ANNOUNCE_INITIAL:
>> -        p->has_announce_initial = true;
>> -        visit_type_size(v, param, &p->announce_initial, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_ANNOUNCE_MAX:
>> -        p->has_announce_max = true;
>> -        visit_type_size(v, param, &p->announce_max, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_ANNOUNCE_ROUNDS:
>> -        p->has_announce_rounds = true;
>> -        visit_type_size(v, param, &p->announce_rounds, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_ANNOUNCE_STEP:
>> -        p->has_announce_step = true;
>> -        visit_type_size(v, param, &p->announce_step, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING:
>> -        error_setg(&err, "The block-bitmap-mapping parameter can only be set "
>> -                   "through QMP");
>> -        break;
>> -    case MIGRATION_PARAMETER_X_VCPU_DIRTY_LIMIT_PERIOD:
>> -        p->has_x_vcpu_dirty_limit_period = true;
>> -        visit_type_size(v, param, &p->x_vcpu_dirty_limit_period, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_VCPU_DIRTY_LIMIT:
>> -        p->has_vcpu_dirty_limit = true;
>> -        visit_type_size(v, param, &p->vcpu_dirty_limit, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_MODE:
>> -        p->has_mode = true;
>> -        visit_type_MigMode(v, param, &p->mode, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_DIRECT_IO:
>> -        p->has_direct_io = true;
>> -        visit_type_bool(v, param, &p->direct_io, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_X_RDMA_CHUNK_SIZE:
>> -        p->has_x_rdma_chunk_size = true;
>> -        visit_type_size(v, param, &p->x_rdma_chunk_size, &err);
>> -        break;
>> -    case MIGRATION_PARAMETER_CPR_EXEC_COMMAND: {
>> +    if (g_str_equal(param, "cpr-exec-command")) {
>>          /*
>>           * NOTE: g_autofree will only auto g_free() the strv array when
>>           * needed, it will not free the strings within the array. It's
>> @@ -760,15 +622,14 @@ void hmp_migrate_set_parameter(MonitorHMP *hmp, const QDict *qdict)
>>  
>>          if (!g_shell_parse_argv(valuestr, NULL, &strv, &gerr)) {
>>              error_setg(&err, "%s", gerr->message);
>> -            break;
>> +            goto cleanup;
>>          }
>>          for (int i = 0; strv[i]; i++) {
>>              QAPI_LIST_APPEND(tail, strv[i]);
>>          }
>>          p->has_cpr_exec_command = true;
>> -        break;
>> -    }
>> -    default:
>> +
>> +    } else {
>>          g_assert_not_reached();
>>      }
>>  
>> @@ -778,12 +639,52 @@ void hmp_migrate_set_parameter(MonitorHMP *hmp, const QDict *qdict)
>>  
>>      qmp_migrate_set_parameters(p, &err);
>>  
>> - cleanup:
>> +cleanup:
>>      qapi_free_MigrationParameters(p);
>> +    hmp_handle_error(hmp, err);
>> +}
>> +
>> +static void hmp_migrate_set_parameter_qapi(MonitorHMP *hmp, const QDict *qdict)
>> +{
>> +    const char *param = qdict_get_str(qdict, "parameter");
>> +    const char *valuestr = qdict_get_str(qdict, "value");
>> +    g_autoptr(QDict) input = qdict_new();
>> +    g_autoptr(MigrationParameters) p = NULL;
>> +    Visitor *v;
>> +    Error *err = NULL;
>> +
>> +    /* the same as keyval_parse(), but here there's no need to parse */
>> +    qdict_put_obj(input, param, QOBJECT(qstring_from_str(valuestr)));
>> +
>> +    v = qobject_input_visitor_new_keyval(QOBJECT(input));
>> +    if (visit_type_MigrationParameters(v, NULL, &p, &err)) {
>> +        qmp_migrate_set_parameters(p, &err);
>> +    }
>> +
>>      visit_free(v);
>>      hmp_handle_error(hmp, err);
>>  }
>>  
>> +void hmp_migrate_set_parameter(MonitorHMP *hmp, const QDict *qdict)
>> +{
>> +    const char *param = qdict_get_str(qdict, "parameter");
>> +
>> +    if (g_str_equal(param, "block-bitmap-mapping")) {
>> +        Error *err = NULL;
>> +
>> +        error_setg(&err, "The %s parameter can only be set through QMP", param);
>> +        hmp_handle_error(hmp, err);
>
> I think a hmp_report_error(MonitorHMP *hmp, const char *fmt, ...) would
> be nice to have.
>

I'll look into it.

>> +        return;
>> +    }
>> +
>> +    /* this has a non-standard setter */
>> +    if (g_str_equal(param, "cpr-exec-command")) {
>
> We check for "cpr-exec-command" in two places, here and in
> hmp_migrate_set_parameter().  A bit awkward.
>
>> +        return hmp_migrate_set_parameter_legacy(hmp, qdict);
>> +    }
>> +
>> +    hmp_migrate_set_parameter_qapi(hmp, qdict);
>> +}
>
> I think I'd do this differently.
>
> Recall how we normally parse text into a C object:
>
> 1. Parse the string into a QObject.
>
> 2. Wrap it in the appropriate visitor.
>
> 3. Convert it into a C object with visit_type_TYPE().
>
> For QMP, step 1 uses json_message_parser_init() & friends, and step 2
> is qobject_input_visitor_new().
>
> For the command line, step 1 and 2 are commonly
> qobject_input_visitor_new_str(), which either uses qobject_from_json()
> and qobject_input_visitor_new(), or keyval_parse() and
> qobject_input_visitor_new_keyval().
>
> hmp_migrate_set_parameter() could do step 1 and 2 like this:
>
>     // step 1
>     if param == "block-bitmap-mapping"
>         report error and return
>     if param == "cpr-exec-command"
>         value = string parsed into QList
>     else // scalars
>         value = string
>     input = {param: value}
>     // step 2
>     v = qobject_input_visitor_new_keyval(input)
>

Makes sense, let me try it.

>> +
>>  void hmp_migrate_start_postcopy(MonitorHMP *hmp, const QDict *qdict)
>>  {
>>      Error *err = NULL;
>> diff --git a/tests/qtest/migration/misc-tests.c b/tests/qtest/migration/misc-tests.c
>> index 2261ae7c89..4ac2f42a5a 100644
>> --- a/tests/qtest/migration/misc-tests.c
>> +++ b/tests/qtest/migration/misc-tests.c
>> @@ -50,7 +50,7 @@ typedef struct HMPTestData {
>>  HMPTestData test_cases[] = {
>>      TEST("", "", "migrate_set_parameter: string expected"),
>>      TEST("foo", "", "migrate_set_parameter: string expected"),
>> -    TEST("foo", "on", "Error: invalid parameter value: foo"),
>> +    TEST("foo", "on", "Error: Parameter 'foo' is unexpected"),
>
> This appears to be an improvement.  Where does it come from?
>

Old code went through the enum lookup whereas now the QObject visitor's
qobject_input_check_struct flags the struct member name.

>>  
>>      /* bool */
>>      TEST("cpu-throttle-tailslow", "on", "on"),

Re: [PATCH v2 13/18] migration: Use keyval input visitor in HMP set command

[Fabiano Rosas]

Fabiano Rosas <farosas@suse.de> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Fabiano Rosas <farosas@suse.de> writes:
>>
>>> Change the hmp_migrate_set_parameter command to use a keyval input
>>> visitor.
>>>
>>> Currently a string visitor is used and due to limitations of that
>>> particular visitor's implementation it's necessary to consult the QAPI
>>> type enum (MigrationParameter_lookup) and call each visit_type_*
>>> function individually. Which makes using a visitor pointless.
>>
>> Also, the less the string visitors are used, the happier I am.
>>
>>> Since there are other visitors implemented properly and generated code
>>> to iterate the QAPI object, prefer using one of those. The keyval
>>> input visitor is adequate because HMP provides basically one key and
>>> one value for each migrate_set_parameter command.
>>>
>>> To switch from string_input_visitor to keyval_input_visitor simply put
>>> the parameter name and value into a dict and invoke
>>> visit_type_MigrationParameters().
>>
>> This works for scalar types: the value is a QString, and the QObject
>> keyval input visitor automatically converts to the C type the visitor
>> expects.  It doesn't work for non-scalar types; see cpr-exec-command
>> below.
>>
>>> Note that it's not necessary to go through any of the keyval_* code
>>> because due to the nature of HMP, there's no parsing to do (no '=', no
>>> ',', etc).
>>
>> migrate_set_parameter syntax isn't keyval, only val is, i.e. its value
>> argument is in keyval value syntax (more or less).
>>
>> "More or less" is my hedge against differences between the string input
>> visitor and the QObject keyval input visitor.  Did you check?
>>
>
> They ultimately use the same functions to do the conversion
> (qemu_strtou64, qemu_strtod_finite, etc). The differences are in
> slightly different wording of error messages and the fact that the
> string input visitor has a different message for -ERANGE while the
> qobject input visitor has a TODO for that case.
>
> I think those differences are of no consequence. The error messages that
> we really care about are the ones resulting from the migration code
> validation (because they're informative to the user). The API level
> messages are too generic anyway.
>
>> Aside: we could create migrate_set_parameters with keyval syntax if we
>> cared.
>>
>>> With this the migrate_set_parameters HMP commands will be
>>> automatically updated anytime a new migration parameter is added.
>>>
>>> One parameter, "cpr-exec-command", takes the strList type which needs
>>> to be built manually. This moves to a "legacy" suffixed function.
>>
>> When visit_type_MigrationParameters() visits "cpr-exec-command", it
>> calls visit_type_strList().  With the QObject keyval input visitor, this
>> expects a QList, not a QString.
>>
>> Thus, hmp_migrate_set_parameter() needs to parse the value argument into
>> a list at least.  That's why it needs to be a special case.
>>
>
> I'm having some difficulty understanding why isn't that the job of the
> visit function.
>
> I think what you're saying is that we can convert QAPI's strList into
> QObject's QList of QString, but cannot convert the HMP string directly
> into QList even though we know that string represents a strList. I feel
> like there's code missing somewhere...
>
> ... although it wouldn't help with cpr-exec-command because of the
> "shell parsing" semantics. But that's another issue.
>
>> It parses it with g_shell_parse_argv().  GLib docs "specify" this to
>> parse "a command line [...] in much the same way the shell would, but
>> without many of the expansions the shell would perform (variable
>> expansion, globs, operators, filename expansion, etc. are not
>> supported)."  Ugh!  But I digress.
>>
>
> One side-effect of having code that handles parameters genericly is that
> there is less room for inventing custom parsing when new parameters are
> introduced.
>
>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>> ---
>>>  migration/migration-hmp-cmds.c     | 195 +++++++----------------------
>>>  tests/qtest/migration/misc-tests.c |   2 +-
>>>  2 files changed, 49 insertions(+), 148 deletions(-)
>>>
>>> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
>>> index 5d754414c4..0d93d38b05 100644
>>> --- a/migration/migration-hmp-cmds.c
>>> +++ b/migration/migration-hmp-cmds.c
>>> @@ -25,7 +25,9 @@
>>>  #include "qapi/error.h"
>>>  #include "qapi/qapi-commands-migration.h"
>>>  #include "qapi/qapi-visit-migration.h"
>>> +#include "qapi/qobject-input-visitor.h"
>>>  #include "qobject/qdict.h"
>>> +#include "qobject/qstring.h"
>>>  #include "qapi/string-input-visitor.h"
>>>  #include "qapi/string-output-visitor.h"
>>>  #include "qemu/cutils.h"
>>> @@ -600,154 +602,14 @@ end:
>>>      hmp_handle_error(hmp, err);
>>>  }
>>>  
>>> -void hmp_migrate_set_parameter(MonitorHMP *hmp, const QDict *qdict)
>>> +static void hmp_migrate_set_parameter_legacy(MonitorHMP *hmp, const QDict *qdict)
>>>  {
>>>      const char *param = qdict_get_str(qdict, "parameter");
>>>      const char *valuestr = qdict_get_str(qdict, "value");
>>> -    Visitor *v = string_input_visitor_new(valuestr);
>>>      MigrationParameters *p = g_new0(MigrationParameters, 1);
>>> -    uint64_t cache_size;
>>>      Error *err = NULL;
>>> -    int val;
>>>  
>>> -    val = qapi_enum_parse(&MigrationParameter_lookup, param, -1, &err);
>>> -    if (val < 0) {
>>> -        goto cleanup;
>>> -    }
>>> -
>>> -    switch (val) {
>>> -    case MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD:
>>> -        p->has_throttle_trigger_threshold = true;
>>> -        visit_type_uint8(v, param, &p->throttle_trigger_threshold, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL:
>>> -        p->has_cpu_throttle_initial = true;
>>> -        visit_type_uint8(v, param, &p->cpu_throttle_initial, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT:
>>> -        p->has_cpu_throttle_increment = true;
>>> -        visit_type_uint8(v, param, &p->cpu_throttle_increment, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_CPU_THROTTLE_TAILSLOW:
>>> -        p->has_cpu_throttle_tailslow = true;
>>> -        visit_type_bool(v, param, &p->cpu_throttle_tailslow, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_MAX_CPU_THROTTLE:
>>> -        p->has_max_cpu_throttle = true;
>>> -        visit_type_uint8(v, param, &p->max_cpu_throttle, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_TLS_CREDS:
>>> -        p->tls_creds = g_new0(StrOrNull, 1);
>>> -        p->tls_creds->type = QTYPE_QSTRING;
>>> -        visit_type_str(v, param, &p->tls_creds->u.s, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_TLS_HOSTNAME:
>>> -        p->tls_hostname = g_new0(StrOrNull, 1);
>>> -        p->tls_hostname->type = QTYPE_QSTRING;
>>> -        visit_type_str(v, param, &p->tls_hostname->u.s, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_TLS_AUTHZ:
>>> -        p->tls_authz = g_new0(StrOrNull, 1);
>>> -        p->tls_authz->type = QTYPE_QSTRING;
>>> -        visit_type_str(v, param, &p->tls_authz->u.s, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_MAX_BANDWIDTH:
>>> -        p->has_max_bandwidth = true;
>>> -        visit_type_size(v, param, &p->max_bandwidth, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_AVAIL_SWITCHOVER_BANDWIDTH:
>>> -        p->has_avail_switchover_bandwidth = true;
>>> -        visit_type_size(v, param, &p->avail_switchover_bandwidth, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_DOWNTIME_LIMIT:
>>> -        p->has_downtime_limit = true;
>>> -        visit_type_size(v, param, &p->downtime_limit, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_X_CHECKPOINT_DELAY:
>>> -        p->has_x_checkpoint_delay = true;
>>> -        visit_type_uint32(v, param, &p->x_checkpoint_delay, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_MULTIFD_CHANNELS:
>>> -        p->has_multifd_channels = true;
>>> -        visit_type_uint8(v, param, &p->multifd_channels, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_MULTIFD_COMPRESSION:
>>> -        p->has_multifd_compression = true;
>>> -        visit_type_MultiFDCompression(v, param, &p->multifd_compression,
>>> -                                      &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_MULTIFD_ZLIB_LEVEL:
>>> -        p->has_multifd_zlib_level = true;
>>> -        visit_type_uint8(v, param, &p->multifd_zlib_level, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_MULTIFD_QATZIP_LEVEL:
>>> -        p->has_multifd_qatzip_level = true;
>>> -        visit_type_uint8(v, param, &p->multifd_qatzip_level, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_MULTIFD_ZSTD_LEVEL:
>>> -        p->has_multifd_zstd_level = true;
>>> -        visit_type_uint8(v, param, &p->multifd_zstd_level, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_ZERO_PAGE_DETECTION:
>>> -        p->has_zero_page_detection = true;
>>> -        visit_type_ZeroPageDetection(v, param, &p->zero_page_detection, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE:
>>> -        p->has_xbzrle_cache_size = true;
>>> -        if (!visit_type_size(v, param, &cache_size, &err)) {
>>> -            break;
>>> -        }
>>> -        if (cache_size > INT64_MAX || (size_t)cache_size != cache_size) {
>>> -            error_setg(&err, "Invalid size %s", valuestr);
>>> -            break;
>>> -        }
>>> -        p->xbzrle_cache_size = cache_size;
>>> -        break;
>>> -    case MIGRATION_PARAMETER_MAX_POSTCOPY_BANDWIDTH:
>>> -        p->has_max_postcopy_bandwidth = true;
>>> -        visit_type_size(v, param, &p->max_postcopy_bandwidth, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_ANNOUNCE_INITIAL:
>>> -        p->has_announce_initial = true;
>>> -        visit_type_size(v, param, &p->announce_initial, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_ANNOUNCE_MAX:
>>> -        p->has_announce_max = true;
>>> -        visit_type_size(v, param, &p->announce_max, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_ANNOUNCE_ROUNDS:
>>> -        p->has_announce_rounds = true;
>>> -        visit_type_size(v, param, &p->announce_rounds, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_ANNOUNCE_STEP:
>>> -        p->has_announce_step = true;
>>> -        visit_type_size(v, param, &p->announce_step, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING:
>>> -        error_setg(&err, "The block-bitmap-mapping parameter can only be set "
>>> -                   "through QMP");
>>> -        break;
>>> -    case MIGRATION_PARAMETER_X_VCPU_DIRTY_LIMIT_PERIOD:
>>> -        p->has_x_vcpu_dirty_limit_period = true;
>>> -        visit_type_size(v, param, &p->x_vcpu_dirty_limit_period, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_VCPU_DIRTY_LIMIT:
>>> -        p->has_vcpu_dirty_limit = true;
>>> -        visit_type_size(v, param, &p->vcpu_dirty_limit, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_MODE:
>>> -        p->has_mode = true;
>>> -        visit_type_MigMode(v, param, &p->mode, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_DIRECT_IO:
>>> -        p->has_direct_io = true;
>>> -        visit_type_bool(v, param, &p->direct_io, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_X_RDMA_CHUNK_SIZE:
>>> -        p->has_x_rdma_chunk_size = true;
>>> -        visit_type_size(v, param, &p->x_rdma_chunk_size, &err);
>>> -        break;
>>> -    case MIGRATION_PARAMETER_CPR_EXEC_COMMAND: {
>>> +    if (g_str_equal(param, "cpr-exec-command")) {
>>>          /*
>>>           * NOTE: g_autofree will only auto g_free() the strv array when
>>>           * needed, it will not free the strings within the array. It's
>>> @@ -760,15 +622,14 @@ void hmp_migrate_set_parameter(MonitorHMP *hmp, const QDict *qdict)
>>>  
>>>          if (!g_shell_parse_argv(valuestr, NULL, &strv, &gerr)) {
>>>              error_setg(&err, "%s", gerr->message);
>>> -            break;
>>> +            goto cleanup;
>>>          }
>>>          for (int i = 0; strv[i]; i++) {
>>>              QAPI_LIST_APPEND(tail, strv[i]);
>>>          }
>>>          p->has_cpr_exec_command = true;
>>> -        break;
>>> -    }
>>> -    default:
>>> +
>>> +    } else {
>>>          g_assert_not_reached();
>>>      }
>>>  
>>> @@ -778,12 +639,52 @@ void hmp_migrate_set_parameter(MonitorHMP *hmp, const QDict *qdict)
>>>  
>>>      qmp_migrate_set_parameters(p, &err);
>>>  
>>> - cleanup:
>>> +cleanup:
>>>      qapi_free_MigrationParameters(p);
>>> +    hmp_handle_error(hmp, err);
>>> +}
>>> +
>>> +static void hmp_migrate_set_parameter_qapi(MonitorHMP *hmp, const QDict *qdict)
>>> +{
>>> +    const char *param = qdict_get_str(qdict, "parameter");
>>> +    const char *valuestr = qdict_get_str(qdict, "value");
>>> +    g_autoptr(QDict) input = qdict_new();
>>> +    g_autoptr(MigrationParameters) p = NULL;
>>> +    Visitor *v;
>>> +    Error *err = NULL;
>>> +
>>> +    /* the same as keyval_parse(), but here there's no need to parse */
>>> +    qdict_put_obj(input, param, QOBJECT(qstring_from_str(valuestr)));
>>> +
>>> +    v = qobject_input_visitor_new_keyval(QOBJECT(input));
>>> +    if (visit_type_MigrationParameters(v, NULL, &p, &err)) {
>>> +        qmp_migrate_set_parameters(p, &err);
>>> +    }
>>> +
>>>      visit_free(v);
>>>      hmp_handle_error(hmp, err);
>>>  }
>>>  
>>> +void hmp_migrate_set_parameter(MonitorHMP *hmp, const QDict *qdict)
>>> +{
>>> +    const char *param = qdict_get_str(qdict, "parameter");
>>> +
>>> +    if (g_str_equal(param, "block-bitmap-mapping")) {
>>> +        Error *err = NULL;
>>> +
>>> +        error_setg(&err, "The %s parameter can only be set through QMP", param);
>>> +        hmp_handle_error(hmp, err);
>>
>> I think a hmp_report_error(MonitorHMP *hmp, const char *fmt, ...) would
>> be nice to have.
>>
>
> I'll look into it.
>

I see that hmp_handle_error() takes an hmp argument that's unused and
also that callers often do the redundant:

   if (err) {
      hmp_handle_error(err);
   }

But aside from those I don't see other improvements to be made, many
callsites of hmp_handle_error() re-use the Error for other calls,
e.g. block/monitor/block-hmp-cmds.c:

    Error *err = NULL;
    ...
    if (!qdict_get_try_str(qdict, "node-name")) {
        qobject_unref(qdict);
        error_setg(&err, "'node-name' needs to be specified");
        goto out;
    }

    BlockDriverState *bs = bds_tree_init(qdict, &err);
    if (!bs) {
        goto out;
    }

    bdrv_set_monitor_owned(bs);
 out:
    qemu_opts_del(opts);
    hmp_handle_error(err);

So having another helper I think would just create confusion as
error_setg() is already a standard way of creating an error.

>>> +        return;
>>> +    }
>>> +
>>> +    /* this has a non-standard setter */
>>> +    if (g_str_equal(param, "cpr-exec-command")) {
>>
>> We check for "cpr-exec-command" in two places, here and in
>> hmp_migrate_set_parameter().  A bit awkward.
>>
>>> +        return hmp_migrate_set_parameter_legacy(hmp, qdict);
>>> +    }
>>> +
>>> +    hmp_migrate_set_parameter_qapi(hmp, qdict);
>>> +}
>>
>> I think I'd do this differently.
>>
>> Recall how we normally parse text into a C object:
>>
>> 1. Parse the string into a QObject.
>>
>> 2. Wrap it in the appropriate visitor.
>>
>> 3. Convert it into a C object with visit_type_TYPE().
>>
>> For QMP, step 1 uses json_message_parser_init() & friends, and step 2
>> is qobject_input_visitor_new().
>>
>> For the command line, step 1 and 2 are commonly
>> qobject_input_visitor_new_str(), which either uses qobject_from_json()
>> and qobject_input_visitor_new(), or keyval_parse() and
>> qobject_input_visitor_new_keyval().
>>
>> hmp_migrate_set_parameter() could do step 1 and 2 like this:
>>
>>     // step 1
>>     if param == "block-bitmap-mapping"
>>         report error and return
>>     if param == "cpr-exec-command"
>>         value = string parsed into QList
>>     else // scalars
>>         value = string
>>     input = {param: value}
>>     // step 2
>>     v = qobject_input_visitor_new_keyval(input)
>>
>
> Makes sense, let me try it.
>

Works like a charm, thanks!

>>> +
>>>  void hmp_migrate_start_postcopy(MonitorHMP *hmp, const QDict *qdict)
>>>  {
>>>      Error *err = NULL;
>>> diff --git a/tests/qtest/migration/misc-tests.c b/tests/qtest/migration/misc-tests.c
>>> index 2261ae7c89..4ac2f42a5a 100644
>>> --- a/tests/qtest/migration/misc-tests.c
>>> +++ b/tests/qtest/migration/misc-tests.c
>>> @@ -50,7 +50,7 @@ typedef struct HMPTestData {
>>>  HMPTestData test_cases[] = {
>>>      TEST("", "", "migrate_set_parameter: string expected"),
>>>      TEST("foo", "", "migrate_set_parameter: string expected"),
>>> -    TEST("foo", "on", "Error: invalid parameter value: foo"),
>>> +    TEST("foo", "on", "Error: Parameter 'foo' is unexpected"),
>>
>> This appears to be an improvement.  Where does it come from?
>>
>
> Old code went through the enum lookup whereas now the QObject visitor's
> qobject_input_check_struct flags the struct member name.
>
>>>  
>>>      /* bool */
>>>      TEST("cpu-throttle-tailslow", "on", "on"),

Re: [PATCH v2 13/18] migration: Use keyval input visitor in HMP set command

[Markus Armbruster]

Fabiano Rosas <farosas@suse.de> writes:

> Fabiano Rosas <farosas@suse.de> writes:
>
>> Markus Armbruster <armbru@redhat.com> writes:

[...]

>>> I think a hmp_report_error(MonitorHMP *hmp, const char *fmt, ...) would
>>> be nice to have.
>>>
>>
>> I'll look into it.
>>
>
> I see that hmp_handle_error() takes an hmp argument that's unused and

Unused since commit 193227f9e5 (error: Use error_report_err() instead of
monitor_printf(), 2015-12-18).

I doubt it's worth removing.

> also that callers often do the redundant:
>
>    if (err) {
>       hmp_handle_error(err);
>    }

Would be nice to clean this up.

> But aside from those I don't see other improvements to be made, many
> callsites of hmp_handle_error() re-use the Error for other calls,
> e.g. block/monitor/block-hmp-cmds.c:
>
>     Error *err = NULL;
>     ...
>     if (!qdict_get_try_str(qdict, "node-name")) {
>         qobject_unref(qdict);
>         error_setg(&err, "'node-name' needs to be specified");
>         goto out;
>     }
>
>     BlockDriverState *bs = bds_tree_init(qdict, &err);
>     if (!bs) {
>         goto out;
>     }
>
>     bdrv_set_monitor_owned(bs);
>  out:
>     qemu_opts_del(opts);
>     hmp_handle_error(err);
>
> So having another helper I think would just create confusion as
> error_setg() is already a standard way of creating an error.

We have both error_report() and error_report_err(), and it doesn't lead
to confusion as far as I can tell.

Whether doing the same for HMP error reporting would be an improvement
is not obvious.  Let's move on.

[...]

Re: [PATCH v2 13/18] migration: Use keyval input visitor in HMP set command

[Fabiano Rosas]

Markus Armbruster <armbru@redhat.com> writes:

> Fabiano Rosas <farosas@suse.de> writes:
>
>> Fabiano Rosas <farosas@suse.de> writes:
>>
>>> Markus Armbruster <armbru@redhat.com> writes:
>
> [...]
>
>>>> I think a hmp_report_error(MonitorHMP *hmp, const char *fmt, ...) would
>>>> be nice to have.
>>>>
>>>
>>> I'll look into it.
>>>
>>
>> I see that hmp_handle_error() takes an hmp argument that's unused and
>
> Unused since commit 193227f9e5 (error: Use error_report_err() instead of
> monitor_printf(), 2015-12-18).
>
> I doubt it's worth removing.
>
>> also that callers often do the redundant:
>>
>>    if (err) {
>>       hmp_handle_error(err);
>>    }
>
> Would be nice to clean this up.
>

Yep, I've a patch for it.

>> But aside from those I don't see other improvements to be made, many
>> callsites of hmp_handle_error() re-use the Error for other calls,
>> e.g. block/monitor/block-hmp-cmds.c:
>>
>>     Error *err = NULL;
>>     ...
>>     if (!qdict_get_try_str(qdict, "node-name")) {
>>         qobject_unref(qdict);
>>         error_setg(&err, "'node-name' needs to be specified");
>>         goto out;
>>     }
>>
>>     BlockDriverState *bs = bds_tree_init(qdict, &err);
>>     if (!bs) {
>>         goto out;
>>     }
>>
>>     bdrv_set_monitor_owned(bs);
>>  out:
>>     qemu_opts_del(opts);
>>     hmp_handle_error(err);
>>
>> So having another helper I think would just create confusion as
>> error_setg() is already a standard way of creating an error.
>
> We have both error_report() and error_report_err(), and it doesn't lead
> to confusion as far as I can tell.
>

They're not analogous, hmp_handle_error() it's more versatile than
error_report_err() because it can cope with err=NULL:

// this uses only one helper for the error
pseudo1() {
    err = NULL;
    
    ret = afunc();
    if (ret < 0) {
        error_setg(&err, "fail");
        goto out;
    }
    
    another_func(&err);
    if (hmp_handle_error(err)) {
        return;
    }
    
    yet_another_func(&err);
out:
    hmp_handle_error(err);
}

vs.

// this needs two different helpers
pseudo2() {
    err = NULL;
    
    ret = afunc();
    if (ret < 0) {
        hmp_error_report("fail");
        return;
    }
    
    another_func(&err);
    if (hmp_handle_error(err)) {
        return;
    }
    
    yet_another_func(&err);
    hmp_handle_error(err);
}

> Whether doing the same for HMP error reporting would be an improvement
> is not obvious.  Let's move on.
>

Sure

> [...]

Re: [PATCH v2 13/18] migration: Use keyval input visitor in HMP set command

[Markus Armbruster]

Fabiano Rosas <farosas@suse.de> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Fabiano Rosas <farosas@suse.de> writes:
>>
>>> Change the hmp_migrate_set_parameter command to use a keyval input
>>> visitor.
>>>
>>> Currently a string visitor is used and due to limitations of that
>>> particular visitor's implementation it's necessary to consult the QAPI
>>> type enum (MigrationParameter_lookup) and call each visit_type_*
>>> function individually. Which makes using a visitor pointless.
>>
>> Also, the less the string visitors are used, the happier I am.
>>
>>> Since there are other visitors implemented properly and generated code
>>> to iterate the QAPI object, prefer using one of those. The keyval
>>> input visitor is adequate because HMP provides basically one key and
>>> one value for each migrate_set_parameter command.
>>>
>>> To switch from string_input_visitor to keyval_input_visitor simply put
>>> the parameter name and value into a dict and invoke
>>> visit_type_MigrationParameters().
>>
>> This works for scalar types: the value is a QString, and the QObject
>> keyval input visitor automatically converts to the C type the visitor
>> expects.  It doesn't work for non-scalar types; see cpr-exec-command
>> below.
>>
>>> Note that it's not necessary to go through any of the keyval_* code
>>> because due to the nature of HMP, there's no parsing to do (no '=', no
>>> ',', etc).
>>
>> migrate_set_parameter syntax isn't keyval, only val is, i.e. its value
>> argument is in keyval value syntax (more or less).
>>
>> "More or less" is my hedge against differences between the string input
>> visitor and the QObject keyval input visitor.  Did you check?
>>
>
> They ultimately use the same functions to do the conversion
> (qemu_strtou64, qemu_strtod_finite, etc). The differences are in
> slightly different wording of error messages and the fact that the
> string input visitor has a different message for -ERANGE while the
> qobject input visitor has a TODO for that case.
>
> I think those differences are of no consequence. The error messages that
> we really care about are the ones resulting from the migration code
> validation (because they're informative to the user). The API level
> messages are too generic anyway.

Should the commit message make this argument?

>> Aside: we could create migrate_set_parameters with keyval syntax if we
>> cared.
>>
>>> With this the migrate_set_parameters HMP commands will be
>>> automatically updated anytime a new migration parameter is added.
>>>
>>> One parameter, "cpr-exec-command", takes the strList type which needs
>>> to be built manually. This moves to a "legacy" suffixed function.
>>
>> When visit_type_MigrationParameters() visits "cpr-exec-command", it
>> calls visit_type_strList().  With the QObject keyval input visitor, this
>> expects a QList, not a QString.
>>
>> Thus, hmp_migrate_set_parameter() needs to parse the value argument into
>> a list at least.  That's why it needs to be a special case.
>>
>
> I'm having some difficulty understanding why isn't that the job of the
> visit function.
>
> I think what you're saying is that we can convert QAPI's strList into
> QObject's QList of QString, but cannot convert the HMP string directly
> into QList even though we know that string represents a strList. I feel
> like there's code missing somewhere...

This is due to separation of concerns in the design of the visitor
pipelines.  Example:

         JSON         input
        parser       visitor
    JSON ---> QObject ---> QAPI-generated C type

         JSON         output
       formatter     visitor
    JSON <--- QObject <--- QAPI-generated C type

Parser/formatter deal with JSON.  All they know about QAPI is QObject.

Visitors deal with QAPI-generated C types.  All they know about JSON is
QObject.

Keyval came much later, and we had to accept a weaker separation for it.
In JSON, the type of scalars is explicit in the syntax.  In keyval, it
is not.  So scalar values are always strings in the parser's output, and
the QObject keyval visitor needs to parse these strings.  This leads to
restrictions discussed in keyval.c's big comment.

But this applies *only* to scalars!  The separation still holds for
objects and arrays.

> ... although it wouldn't help with cpr-exec-command because of the
> "shell parsing" semantics. But that's another issue.

Yes.

>> It parses it with g_shell_parse_argv().  GLib docs "specify" this to
>> parse "a command line [...] in much the same way the shell would, but
>> without many of the expansions the shell would perform (variable
>> expansion, globs, operators, filename expansion, etc. are not
>> supported)."  Ugh!  But I digress.
>
> One side-effect of having code that handles parameters genericly is that
> there is less room for inventing custom parsing when new parameters are
> introduced.

We use g_shell_parse_argv() for cpr-exec-command and nothing else, which
effectively makes it custom syntax.  At least it's custom syntax we
didn't design and implement, but still.  Too.  Much.  Syntax.  Too.
Many.  Parsers.

>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>

[...]

>>> diff --git a/tests/qtest/migration/misc-tests.c b/tests/qtest/migration/misc-tests.c
>>> index 2261ae7c89..4ac2f42a5a 100644
>>> --- a/tests/qtest/migration/misc-tests.c
>>> +++ b/tests/qtest/migration/misc-tests.c
>>> @@ -50,7 +50,7 @@ typedef struct HMPTestData {
>>>  HMPTestData test_cases[] = {
>>>      TEST("", "", "migrate_set_parameter: string expected"),
>>>      TEST("foo", "", "migrate_set_parameter: string expected"),
>>> -    TEST("foo", "on", "Error: invalid parameter value: foo"),
>>> +    TEST("foo", "on", "Error: Parameter 'foo' is unexpected"),
>>
>> This appears to be an improvement.  Where does it come from?
>>
>
> Old code went through the enum lookup whereas now the QObject visitor's
> qobject_input_check_struct flags the struct member name.

hmp_migrate_set_parameter() takes a parameter argument that names a
member of MigrationParameters.  Trick: it parses it as a member of
related type MigrationParameter, with qapi_enum_parse().  Since the
members of MigrationParameter are *values*, qapi_enum_parse() reasonably
reports "invalid parameter value".

Trickery breed bad error messages, film at eleven.

Suggest to mention the improvement in the commit message.

>>>  
>>>      /* bool */
>>>      TEST("cpu-throttle-tailslow", "on", "on"),

[PATCH v2 14/18] migration: Use output visitor in info command

[Fabiano Rosas]

The hmp_info_migrate_parameters function currently open-codes the
mon_printf calls for each migration parameter. As with the set command
in the last patch, this should not be necessary as the QAPI
infrastructure already has generated code that takes type and struct
member names into account, including converting _ from C into the '-'
character as part of parameter names strings.

The current code is also quite painful to rebase if a series has been
carried for a long time while parameters have been added in master.

Replace all of this with a conversion from MigrationParameters to
QDict using an output visitor and a loop over the QDict that prints
per-QAPI-type formatted strings.

Modelled after block/qapi.c:dump_qobject, but with some changes to
keep the migration command output formatting.

Note that this was not a for-free improvement, the HMP command format
output was changed incompatibly in a previous patch. It doesn't output
units anymore.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration-hmp-cmds.c | 238 ++++++++++++---------------------
 migration/options.c            |   2 +-
 migration/options.h            |   1 +
 3 files changed, 86 insertions(+), 155 deletions(-)

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 0d93d38b05..5dfbb69390 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -26,7 +26,12 @@
 #include "qapi/qapi-commands-migration.h"
 #include "qapi/qapi-visit-migration.h"
 #include "qapi/qobject-input-visitor.h"
+#include "qapi/qobject-output-visitor.h"
+#include "qobject/qbool.h"
 #include "qobject/qdict.h"
+#include "qobject/qjson.h"
+#include "qobject/qlist.h"
+#include "qobject/qnum.h"
 #include "qobject/qstring.h"
 #include "qapi/string-input-visitor.h"
 #include "qapi/string-output-visitor.h"
@@ -317,170 +322,95 @@ void hmp_info_migrate_capabilities(MonitorHMP *hmp, const QDict *qdict)
     qapi_free_MigrationCapabilityStatusList(caps);
 }
 
-static void monitor_print_cpr_exec_command(MonitorHMP *hmp, strList *args)
+static void hmp_migrate_print_qobject(MonitorHMP *hmp, const char *label,
+                                      QObject *obj)
 {
-    monitor_hmp_printf(hmp, "%s:",
-        MigrationParameter_str(MIGRATION_PARAMETER_CPR_EXEC_COMMAND));
+    const char *sep;
 
-    while (args) {
-        monitor_hmp_printf(hmp, " %s", args->value);
-        args = args->next;
+    if (!obj) {
+        return;
+    }
+
+    /*
+     * Put a space after labels
+     * foo: bar
+     *     ^
+     */
+    if (label && label[0] && label[strlen(label) - 1] == ':') {
+        sep = " ";
+    } else {
+        sep = "";
+    }
+
+    switch (qobject_type(obj)) {
+    case QTYPE_QNUM: {
+        g_autofree char *str = qnum_to_string(qobject_to(QNum, obj));
+
+        monitor_hmp_printf(hmp, "%s%s%s", label, sep, str);
+        break;
+    }
+    case QTYPE_QSTRING:
+        monitor_hmp_printf(hmp, "%s%s%s", label, sep,
+                           qstring_get_str(qobject_to(QString, obj)));
+        break;
+    case QTYPE_QDICT: {
+        QDict *d = qobject_to(QDict, obj);
+        const QDictEntry *e;
+        int i = 0;
+
+        for (e = qdict_first(d); e; e = qdict_next(d, e), i++) {
+            g_autofree char *l = g_strdup_printf("%s:", qdict_entry_key(e));
+            if (i) {
+                monitor_hmp_printf(hmp, " ");
+            }
+            hmp_migrate_print_qobject(hmp, l, qdict_entry_value(e));
+        }
+        break;
+    }
+    case QTYPE_QLIST: {
+        const QListEntry *e;
+
+        monitor_hmp_printf(hmp, "%s", label);
+
+        for (e = qlist_first(qobject_to(QList, obj)); e; e = qlist_next(e)) {
+            /*
+             * In the first iteration, this is the space after the
+             * colon, otherwise it's the space between list
+             * elements.
+             */
+            monitor_hmp_printf(hmp, " ");
+            hmp_migrate_print_qobject(hmp, "", e->value);
+        }
+
+        break;
+    }
+    case QTYPE_QBOOL:
+        monitor_hmp_printf(hmp, "%s%s%s", label, sep,
+                           qbool_get_bool(qobject_to(QBool, obj)) ?
+                           "on" : "off");
+        break;
+    case QTYPE_NONE:
+    case QTYPE_QNULL:
+    default:
+        g_assert_not_reached();
+        break;
     }
-    monitor_hmp_printf(hmp, "\n");
 }
 
 void hmp_info_migrate_parameters(MonitorHMP *hmp, const QDict *qdict)
 {
-    MigrationParameters *params;
-    MigrationState *s = migrate_get_current();
+    MigrationParameters *params = qmp_query_migrate_parameters(NULL);
+    g_autoptr(QDict) d;
+    const QDictEntry *e;
 
-    params = qmp_query_migrate_parameters(NULL);
+    assert(params);
 
-    if (params) {
-        monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
-            MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_INITIAL),
-            params->announce_initial);
-        monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
-            MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_MAX),
-            params->announce_max);
-        monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
-            MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_ROUNDS),
-            params->announce_rounds);
-        monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
-            MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_STEP),
-            params->announce_step);
-        assert(params->has_throttle_trigger_threshold);
-        monitor_hmp_printf(hmp, "%s: %u\n",
-            MigrationParameter_str(MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD),
-            params->throttle_trigger_threshold);
-        assert(params->has_cpu_throttle_initial);
-        monitor_hmp_printf(hmp, "%s: %u\n",
-            MigrationParameter_str(MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL),
-            params->cpu_throttle_initial);
-        assert(params->has_cpu_throttle_increment);
-        monitor_hmp_printf(hmp, "%s: %u\n",
-            MigrationParameter_str(MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT),
-            params->cpu_throttle_increment);
-        assert(params->has_cpu_throttle_tailslow);
-        monitor_hmp_printf(hmp, "%s: %s\n",
-            MigrationParameter_str(MIGRATION_PARAMETER_CPU_THROTTLE_TAILSLOW),
-            params->cpu_throttle_tailslow ? "on" : "off");
-        assert(params->has_max_cpu_throttle);
-        monitor_hmp_printf(hmp, "%s: %u\n",
-            MigrationParameter_str(MIGRATION_PARAMETER_MAX_CPU_THROTTLE),
-            params->max_cpu_throttle);
-        assert(params->tls_creds);
-        monitor_hmp_printf(hmp, "%s: %s\n",
-            MigrationParameter_str(MIGRATION_PARAMETER_TLS_CREDS),
-                       params->tls_creds->u.s);
-        assert(params->tls_hostname);
-        monitor_hmp_printf(hmp, "%s: %s\n",
-            MigrationParameter_str(MIGRATION_PARAMETER_TLS_HOSTNAME),
-                       params->tls_hostname->u.s);
-        assert(params->tls_authz);
-        monitor_hmp_printf(hmp, "%s: %s\n",
-            MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
-                       params->tls_authz->u.s);
-        assert(params->has_max_bandwidth);
-        monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
-            MigrationParameter_str(MIGRATION_PARAMETER_MAX_BANDWIDTH),
-            params->max_bandwidth);
-        assert(params->has_avail_switchover_bandwidth);
-        monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
-            MigrationParameter_str(MIGRATION_PARAMETER_AVAIL_SWITCHOVER_BANDWIDTH),
-            params->avail_switchover_bandwidth);
-        assert(params->has_max_postcopy_bandwidth);
-        monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
-            MigrationParameter_str(MIGRATION_PARAMETER_MAX_POSTCOPY_BANDWIDTH),
-            params->max_postcopy_bandwidth);
-        assert(params->has_downtime_limit);
-        monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
-            MigrationParameter_str(MIGRATION_PARAMETER_DOWNTIME_LIMIT),
-            params->downtime_limit);
-        assert(params->has_x_checkpoint_delay);
-        monitor_hmp_printf(hmp, "%s: %u\n",
-            MigrationParameter_str(MIGRATION_PARAMETER_X_CHECKPOINT_DELAY),
-            params->x_checkpoint_delay);
-        monitor_hmp_printf(hmp, "%s: %u\n",
-            MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_CHANNELS),
-            params->multifd_channels);
-        monitor_hmp_printf(hmp, "%s: %s\n",
-            MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_COMPRESSION),
-            MultiFDCompression_str(params->multifd_compression));
-        assert(params->has_zero_page_detection);
-        monitor_hmp_printf(hmp, "%s: %s\n",
-            MigrationParameter_str(MIGRATION_PARAMETER_ZERO_PAGE_DETECTION),
-            qapi_enum_lookup(&ZeroPageDetection_lookup,
-                params->zero_page_detection));
-        monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
-            MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE),
-            params->xbzrle_cache_size);
+    d = migrate_params_to_dict(params, NULL);
+    for (e = qdict_first(d); e; e = qdict_next(d, e)) {
+        g_autofree char *label = g_strdup_printf("%s:", qdict_entry_key(e));
 
-        if (s->has_block_bitmap_mapping) {
-            BitmapMigrationNodeAliasList *nal;
-            BitmapMigrationNodeAlias *na;
-            BitmapMigrationBitmapAliasList *bal;
-            BitmapMigrationBitmapAlias *ba;
-            BitmapMigrationBitmapAliasTransform *bat;
-
-            monitor_hmp_printf(hmp, "%s:",
-                           MigrationParameter_str(
-                               MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING));
-
-            for (nal = params->block_bitmap_mapping; nal; nal = nal->next)
-            {
-                na = nal->value;
-                monitor_hmp_printf(hmp, " bitmaps:");
-                for (bal = na->bitmaps; bal; bal = bal->next) {
-                    ba = bal->value;
-                    bat = ba->transform;
-
-                    monitor_hmp_printf(hmp, " name: %s", ba->name);
-                    if (bat && bat->has_persistent) {
-                        if (bat->persistent) {
-                            monitor_hmp_printf(hmp, " persistent: on");
-                        } else {
-                            monitor_hmp_printf(hmp, " persistent: off");
-                        }
-                    }
-                    monitor_hmp_printf(hmp, " alias: %s", ba->alias);
-                }
-                monitor_hmp_printf(hmp, " node-name: %s alias: %s",
-                               na->node_name, na->alias);
-            }
-
-            monitor_hmp_printf(hmp, "\n");
-        }
-
-        monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
-        MigrationParameter_str(MIGRATION_PARAMETER_X_VCPU_DIRTY_LIMIT_PERIOD),
-        params->x_vcpu_dirty_limit_period);
-
-        monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
-            MigrationParameter_str(MIGRATION_PARAMETER_VCPU_DIRTY_LIMIT),
-            params->vcpu_dirty_limit);
-
-        assert(params->has_mode);
-        monitor_hmp_printf(hmp, "%s: %s\n",
-            MigrationParameter_str(MIGRATION_PARAMETER_MODE),
-            qapi_enum_lookup(&MigMode_lookup, params->mode));
-
-        if (params->has_direct_io) {
-            monitor_hmp_printf(hmp, "%s: %s\n",
-                               MigrationParameter_str(
-                                   MIGRATION_PARAMETER_DIRECT_IO),
-                               params->direct_io ? "on" : "off");
-        }
-
-        if (params->has_x_rdma_chunk_size) {
-            monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
-                           MigrationParameter_str(
-                               MIGRATION_PARAMETER_X_RDMA_CHUNK_SIZE),
-                           params->x_rdma_chunk_size);
-        }
-
-        assert(params->has_cpr_exec_command);
-        monitor_print_cpr_exec_command(hmp, params->cpr_exec_command);
+        hmp_migrate_print_qobject(hmp, label, qdict_entry_value(e));
+        monitor_hmp_printf(hmp, "\n");
     }
 
     qapi_free_MigrationParameters(params);
diff --git a/migration/options.c b/migration/options.c
index 990fcd6867..188c4ad20b 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -1089,7 +1089,7 @@ static void tls_opt_to_str(StrOrNull *opt)
     opt->u.s = g_strdup("");
 }
 
-static QDict *migrate_params_to_dict(MigrationParameters *p, Error **errp)
+QDict *migrate_params_to_dict(MigrationParameters *p, Error **errp)
 {
     QObject *obj = NULL;
     Visitor *v = qobject_output_visitor_new(&obj);
diff --git a/migration/options.h b/migration/options.h
index c7da2d0b5b..0d9f88c11e 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -94,4 +94,5 @@ uint64_t migrate_rdma_chunk_size(void);
 bool migrate_params_check(MigrationParameters *params, Error **errp);
 void migrate_params_init(MigrationParameters *params);
 bool migrate_params_free(MigrationParameters *params, Error **errp);
+QDict *migrate_params_to_dict(MigrationParameters *p, Error **errp);
 #endif
-- 
2.53.0

[PATCH v2 15/18] migration: Rewrite migrate_set_parameter_completion using QDict

[Fabiano Rosas]

The migrate_set_parameter_completion function is the last user of the
MigrationParameter enum. Write the code using an output visitor and
QDict instead so we can remove the enum in a future patch.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration-hmp-cmds.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 5dfbb69390..720913255f 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -736,14 +736,19 @@ void migrate_set_capability_completion(ReadLineState *rs, int nb_args,
 void migrate_set_parameter_completion(ReadLineState *rs, int nb_args,
                                       const char *str)
 {
+    g_autoptr(QDict) d = NULL;
+    const QDictEntry *e;
     size_t len;
 
+    /* Temporarily borrow the global parameters */
+    d = migrate_params_to_dict(&migrate_get_current()->parameters,
+                               &error_abort);
     len = strlen(str);
     readline_set_completion_index(rs, len);
     if (nb_args == 2) {
-        int i;
-        for (i = 0; i < MIGRATION_PARAMETER__MAX; i++) {
-            readline_add_completion_of(rs, str, MigrationParameter_str(i));
+        for (e = qdict_first(d); e; e = qdict_next(d, e)) {
+            const char *key = qdict_entry_key(e);
+            readline_add_completion_of(rs, str, key);
         }
     }
 }
-- 
2.53.0

Re: [PATCH v2 15/18] migration: Rewrite migrate_set_parameter_completion using QDict

[Markus Armbruster]

Fabiano Rosas <farosas@suse.de> writes:

> The migrate_set_parameter_completion function is the last user of the
> MigrationParameter enum.

Not true, there's still the assertion in
migrate_mark_all_params_present(), and hmp_completion_single() in
tests/qtest/migration/misc-tests.c.

The assertion is unclean: it uses MIGRATION_PARAMETER__MAX for number of
MigrationParameters members, which tacitly assumes MigrationParameter
matches MigrationParameters.

>                          Write the code using an output visitor and
> QDict instead so we can remove the enum in a future patch.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH v2 15/18] migration: Rewrite migrate_set_parameter_completion using QDict

[Fabiano Rosas]

Markus Armbruster <armbru@redhat.com> writes:

> Fabiano Rosas <farosas@suse.de> writes:
>
>> The migrate_set_parameter_completion function is the last user of the
>> MigrationParameter enum.
>
> Not true, there's still the assertion in
> migrate_mark_all_params_present(), and hmp_completion_single() in
> tests/qtest/migration/misc-tests.c.
>
> The assertion is unclean: it uses MIGRATION_PARAMETER__MAX for number of
> MigrationParameters members, which tacitly assumes MigrationParameter
> matches MigrationParameters.
>

I'm not sure I understand what you mean here. The usefulness of
MigrationParameter is quite diminished if it doesn't always match the
members of MigrationParameters.

In fact... could we code-generate the enum from MigrationParameters
without the need for explicitly declaring MigrationParameter? That way
we keep the usefulness of having a lookup function and a "max" and do
away with the task of updating MigrationParameter to match
MigrationParameters.

>>                          Write the code using an output visitor and
>> QDict instead so we can remove the enum in a future patch.
>>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH v2 15/18] migration: Rewrite migrate_set_parameter_completion using QDict

[Markus Armbruster]

Fabiano Rosas <farosas@suse.de> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Fabiano Rosas <farosas@suse.de> writes:
>>
>>> The migrate_set_parameter_completion function is the last user of the
>>> MigrationParameter enum.
>>
>> Not true, there's still the assertion in
>> migrate_mark_all_params_present(), and hmp_completion_single() in
>> tests/qtest/migration/misc-tests.c.
>>
>> The assertion is unclean: it uses MIGRATION_PARAMETER__MAX for number of
>> MigrationParameters members, which tacitly assumes MigrationParameter
>> matches MigrationParameters.
>>
>
> I'm not sure I understand what you mean here. The usefulness of
> MigrationParameter is quite diminished if it doesn't always match the
> members of MigrationParameters.

What we want to assert is "has_fields[] covers all the optional members
of MigrationParameters *p.

That's hard, so we settle for "has_fields[] covers the exact right
number of bool pointers".  Works as long as all elements of has_fields[]
point into @p, and there are no duplicates.

Note that "exact right number" is necessary but not sufficient for the
"covers all".  In other words, we're settling for a weaker assertion we
can actually check.  That's okay.

But that's still hard, so we assert "has_fields[] covers as many bool
pointers as there are MigrationParameter values" instead.  Works as long
as MigrationParameter matches MigrationParameters.

This isn't settling for a weaker assertion, it's asserting something
else!  It tacitly relies on "MigrationParameter matches
MigrationParameters", which is not locally obvious, and not ensured by
code, only by patch review.  I consider that unclean.  Doesn't mean it's
worthless, does mean it needs a comment.

> In fact... could we code-generate the enum from MigrationParameters
> without the need for explicitly declaring MigrationParameter? That way
> we keep the usefulness of having a lookup function and a "max" and do
> away with the task of updating MigrationParameter to match
> MigrationParameters.

Yes, we could, but it would be too much QAPI generator and documentation
complexity for just one use case.

The existing tool to introspect QAPI types from C is visitors.  Could it
work?  A bit of a bother, I'm afraid, but less complex than messing with
the QAPI generator.

>>>                          Write the code using an output visitor and
>>> QDict instead so we can remove the enum in a future patch.
>>>
>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>

[PATCH v2 16/18] migration: Add capabilities into MigrationParameters

[Fabiano Rosas]

Add capabilities to MigrationParameters. This structure will hold all
migration options. Capabilities will go away in the next patch.

From this point on, both QMP and HMP versions of
migrate-set-parameters and query-migrate-parameters gain the ability
to work with capabilities.

With MigrationParameters now having members for each capability, the
migration capabilities commands (query-migrate-capabilities,
migrate-set-capabilities) will soon be deprecated. Add a set of
helpers to convert between the old MigrationCapability representation
and the new representation as members of MigrationParameters.

Acked-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c |   8 +++
 migration/options.c   |  71 ++++++++++++++++++++++++
 migration/options.h   |   5 ++
 qapi/migration.json   | 124 ++++++++++++++++++++++++++++++++++++++++--
 4 files changed, 204 insertions(+), 4 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index dab282dd2f..c8e7e86ea0 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -4086,6 +4086,14 @@ static bool migration_object_check(MigrationState *ms, Error **errp)
         return false;
     }
 
+    /*
+     * FIXME: Temporarily while -global capabilties are still using
+     * s->capabilities. Will be gone by the end of the series.
+     */
+    for (int i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
+        migrate_capability_set_compat(&ms->parameters, i, ms->capabilities[i]);
+    }
+
     return migrate_caps_check(old_caps, ms->capabilities, errp);
 }
 
diff --git a/migration/options.c b/migration/options.c
index 188c4ad20b..aff5d8f550 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -775,6 +775,52 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
     return true;
 }
 
+static bool *migrate_capability_get_addr(MigrationParameters *params,
+                                         enum MigrationCapability i)
+{
+    bool *cap_addr[] = {
+        &params->xbzrle, &params->rdma_pin_all,
+        &params->auto_converge, &params->events, &params->postcopy_ram,
+        &params->x_colo, &params->release_ram, &params->return_path,
+        &params->pause_before_switchover, &params->multifd,
+        &params->dirty_bitmaps, &params->postcopy_blocktime,
+        &params->late_block_activate, &params->x_ignore_shared,
+        &params->validate_uuid, &params->background_snapshot,
+        &params->zero_copy_send, &params->postcopy_preempt,
+        &params->switchover_ack, &params->dirty_limit, &params->mapped_ram,
+    };
+
+    assert(i >= 0 && i < MIGRATION_CAPABILITY__MAX);
+    return cap_addr[i];
+}
+
+/* Compatibility for code that reads capabilities in a loop */
+bool migrate_capability_get_compat(MigrationParameters *params, int i)
+{
+    return *(migrate_capability_get_addr(params, i));
+}
+
+/* Compatibility for code that writes capabilities in a loop */
+void migrate_capability_set_compat(MigrationParameters *params, int i, bool val)
+{
+    *(migrate_capability_get_addr(params, i)) = val;
+}
+
+/*
+ * Set capabilities for compatibility with the old
+ * migrate-set-capabilities command.
+ */
+void migrate_capabilities_set_compat(MigrationParameters *params,
+                                     MigrationCapabilityStatusList *caps)
+{
+    MigrationCapabilityStatusList *cap;
+
+    for (cap = caps; cap; cap = cap->next) {
+        migrate_capability_set_compat(params, cap->value->capability,
+                                      cap->value->state);
+    }
+}
+
 MigrationCapabilityStatusList *qmp_query_migrate_capabilities(Error **errp)
 {
     MigrationCapabilityStatusList *head = NULL, **tail = &head;
@@ -816,6 +862,8 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
     for (cap = params; cap; cap = cap->next) {
         s->capabilities[cap->value->capability] = cap->value->state;
     }
+
+    migrate_capabilities_set_compat(&s->parameters, params);
 }
 
 /* parameters */
@@ -1139,6 +1187,15 @@ static void migrate_mark_all_params_present(MigrationParameters *p)
         &p->has_x_vcpu_dirty_limit_period, &p->has_vcpu_dirty_limit,
         &p->has_mode, &p->has_zero_page_detection, &p->has_direct_io,
         &p->has_x_rdma_chunk_size, &p->has_cpr_exec_command,
+        &p->has_xbzrle, &p->has_rdma_pin_all,
+        &p->has_auto_converge, &p->has_events,
+        &p->has_postcopy_ram, &p->has_x_colo, &p->has_release_ram,
+        &p->has_return_path, &p->has_pause_before_switchover, &p->has_multifd,
+        &p->has_dirty_bitmaps, &p->has_postcopy_blocktime,
+        &p->has_late_block_activate, &p->has_x_ignore_shared,
+        &p->has_validate_uuid, &p->has_background_snapshot,
+        &p->has_zero_copy_send, &p->has_postcopy_preempt,
+        &p->has_switchover_ack, &p->has_dirty_limit, &p->has_mapped_ram,
     };
 
     len = ARRAY_SIZE(has_fields);
@@ -1414,6 +1471,20 @@ void qmp_migrate_set_parameters(MigrationParameters *input, Error **errp)
     tls_opt_to_str(input->tls_hostname);
     tls_opt_to_str(input->tls_authz);
 
+    /*
+     * FIXME: Temporarily while migrate_caps_check is not
+     * converted to look at s->parameters. Will be gone the end of
+     * the series.
+     */
+    bool new_caps[MIGRATION_CAPABILITY__MAX] = { 0 };
+    for (int i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
+        new_caps[i] = migrate_capability_get_compat(cur, i);
+    }
+    if (!migrate_caps_check(migrate_get_current()->capabilities, new_caps,
+                            errp)) {
+        return;
+    }
+
     /* merge input on top of current */
     if (!migrate_params_merge(cur, input, &new, errp)) {
         return;
diff --git a/migration/options.h b/migration/options.h
index 0d9f88c11e..f7bf56e9ff 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -95,4 +95,9 @@ bool migrate_params_check(MigrationParameters *params, Error **errp);
 void migrate_params_init(MigrationParameters *params);
 bool migrate_params_free(MigrationParameters *params, Error **errp);
 QDict *migrate_params_to_dict(MigrationParameters *p, Error **errp);
+bool migrate_capability_get_compat(MigrationParameters *params, int i);
+void migrate_capability_set_compat(MigrationParameters *params, int i,
+                                   bool val);
+void migrate_capabilities_set_compat(MigrationParameters *params,
+                                     MigrationCapabilityStatusList *caps);
 #endif
diff --git a/qapi/migration.json b/qapi/migration.json
index b1eaf7b054..5f4ad965d6 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -830,7 +830,14 @@
            'zero-page-detection',
            'direct-io',
            { 'name': 'x-rdma-chunk-size', 'features': [ 'unstable' ] },
-           'cpr-exec-command'] }
+           'cpr-exec-command',
+           'xbzrle', 'rdma-pin-all', 'auto-converge', 'events',
+           'postcopy-ram', 'x-colo', 'release-ram', 'return-path',
+           'pause-before-switchover', 'multifd', 'dirty-bitmaps',
+           'postcopy-blocktime', 'late-block-activate', 'x-ignore-shared',
+           'validate-uuid', 'background-snapshot', 'zero-copy-send',
+           'postcopy-preempt', 'switchover-ack', 'dirty-limit',
+           'mapped-ram'] }
 
 ##
 # @migrate-set-parameters:
@@ -1012,10 +1019,98 @@
 #     Must be set to the same value on both source and destination
 #     before migration starts.  (Since 11.1)
 #
+# @xbzrle: Migration supports xbzrle (Xor Based Zero Run Length
+#     Encoding).  This feature allows us to minimize migration traffic
+#     for certain work loads, by sending compressed difference of the
+#     pages
+#
+# @rdma-pin-all: Controls whether or not the entire VM memory
+#     footprint is mlock()'d on demand or all at once.  Refer to
+#     docs/rdma.txt for usage.  Disabled by default.  (since 2.0)
+#
+# @events: generate events for each migration state change (since 2.4)
+#
+# @auto-converge: If enabled, QEMU will automatically throttle down
+#     the guest to speed up convergence of RAM migration.  (since 1.6)
+#
+# @postcopy-ram: Start executing on the migration target before all of
+#     RAM has been migrated, pulling the remaining pages along as
+#     needed.  The capacity must have the same setting on both source
+#     and target or migration will not even start.  **Note:** if the
+#     migration fails during postcopy the VM will fail.  (since 2.6)
+#
+# @x-colo: If enabled, migration will never end, and the state of the
+#     VM on the primary side will be migrated continuously to the VM
+#     on secondary side, this process is called COarse-Grain LOck
+#     Stepping (COLO) for Non-stop Service.  (since 2.8)
+#
+# @release-ram: if enabled, QEMU will free the migrated ram pages on
+#     the source during postcopy-ram migration.  (since 2.9)
+#
+# @return-path: If enabled, migration will use the return path even
+#     for precopy.  (since 2.10)
+#
+# @pause-before-switchover: Pause outgoing migration before
+#     serialising device state and before disabling block IO
+#     (since 2.11)
+#
+# @multifd: Use more than one fd for migration (since 4.0)
+#
+# @dirty-bitmaps: If enabled, QEMU will migrate named dirty bitmaps.
+#     (since 2.12)
+#
+# @postcopy-blocktime: Calculate downtime for postcopy live migration
+#     (since 3.0)
+#
+# @late-block-activate: If enabled, the destination will not activate
+#     block devices (and thus take locks) immediately at the end of
+#     migration.  (since 3.0)
+#
+# @x-ignore-shared: If enabled, QEMU will not migrate shared memory
+#     that is accessible on the destination machine.  (since 4.0)
+#
+# @validate-uuid: Send the UUID of the source to allow the destination
+#     to ensure it is the same.  (since 4.2)
+#
+# @background-snapshot: If enabled, the migration stream will be a
+#     snapshot of the VM exactly at the point when the migration
+#     procedure starts.  The VM RAM is saved with running VM.
+#     (since 6.0)
+#
+# @zero-copy-send: Controls behavior on sending memory pages on
+#     migration.  When true, enables a zero-copy mechanism for sending
+#     memory pages, if host supports it.  Requires that QEMU be
+#     permitted to use locked memory for guest RAM pages.  (since 7.1)
+#
+# @postcopy-preempt: If enabled, the migration process will allow
+#     postcopy requests to preempt precopy stream, so postcopy
+#     requests will be handled faster.  This is a performance feature
+#     and should not affect the correctness of postcopy migration.
+#     (since 7.1)
+#
+# @switchover-ack: If enabled, migration will not stop the source VM
+#     and complete the migration until the destination has
+#     acknowledged that it is OK to switch over.  The acknowledgement
+#     may depend, for example, on some device's data being loaded in
+#     the destination before doing switchover.  This can reduce
+#     downtime if devices that support this capability are present.
+#     Capability @return-path must be enabled to use it.  (since 8.1)
+#
+# @dirty-limit: If enabled, migration will throttle vCPUs as needed to
+#     keep their dirty page rate within @vcpu-dirty-limit.  This can
+#     improve responsiveness of large guests during live migration,
+#     and can result in more stable read performance.  Requires KVM
+#     with accelerator property "dirty-ring-size" set.  (Since 8.1)
+#
+# @mapped-ram: Migrate using fixed offsets in the migration file for
+#     each RAM page.  Requires a migration URI that supports seeking,
+#     such as a file.  (since 9.0)
+#
 # Features:
 #
-# @unstable: Members @x-checkpoint-delay, @x-rdma-chunk-size, and
-#     @x-vcpu-dirty-limit-period are experimental.
+# @unstable: Members @x-checkpoint-delay, @x-rdma-chunk-size,
+#     @x-vcpu-dirty-limit-period, @x-colo, @x-ignore-shared and are
+#     experimental.
 #
 # Since: 2.4
 ##
@@ -1053,7 +1148,28 @@
             '*direct-io': 'bool',
             '*x-rdma-chunk-size': { 'type': 'uint64',
                                     'features': [ 'unstable' ] },
-            '*cpr-exec-command': [ 'str' ]} }
+            '*cpr-exec-command': [ 'str' ],
+            '*xbzrle': 'bool',
+            '*rdma-pin-all': 'bool',
+            '*auto-converge': 'bool',
+            '*events': 'bool',
+            '*postcopy-ram': 'bool',
+            '*x-colo': { 'type': 'bool', 'features': [ 'unstable' ] },
+            '*release-ram': 'bool',
+            '*return-path': 'bool',
+            '*pause-before-switchover': 'bool',
+            '*multifd': 'bool',
+            '*dirty-bitmaps': 'bool',
+            '*postcopy-blocktime': 'bool',
+            '*late-block-activate': 'bool',
+            '*x-ignore-shared': { 'type': 'bool', 'features': [ 'unstable' ] },
+            '*validate-uuid': 'bool',
+            '*background-snapshot': 'bool',
+            '*zero-copy-send': 'bool',
+            '*postcopy-preempt': 'bool',
+            '*switchover-ack': 'bool',
+            '*dirty-limit': 'bool',
+            '*mapped-ram': 'bool' } }
 
 ##
 # @query-migrate-parameters:
-- 
2.53.0

Re: [PATCH v2 16/18] migration: Add capabilities into MigrationParameters

[Markus Armbruster]

Fabiano Rosas <farosas@suse.de> writes:

> Add capabilities to MigrationParameters. This structure will hold all
> migration options. Capabilities will go away in the next patch.
>
> From this point on, both QMP and HMP versions of
> migrate-set-parameters and query-migrate-parameters gain the ability
> to work with capabilities.
>
> With MigrationParameters now having members for each capability, the
> migration capabilities commands (query-migrate-capabilities,
> migrate-set-capabilities) will soon be deprecated. Add a set of
> helpers to convert between the old MigrationCapability representation
> and the new representation as members of MigrationParameters.
>
> Acked-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

QAPI schema
Acked-by: Markus Armbruster <armbru@redhat.com>

[PATCH v2 17/18] migration: Remove s->capabilities

[Fabiano Rosas]

Last patch added capabilities to s->parameters. Now we can replace all
instances of s->capabilities with s->parameters:

- The -global properties now get set directly in s->parameters.

- Accessors from options.c now read from s->parameters.

- migrate_caps_check() now takes a MigrationParameters object. The
  function is still kept around because migrate-set-capabilities will
  still use it.

- The machinery for background-snapshot compatibility check goes
  away. We can check each capability by name (if s->parameters.cap ...)

- savevm uses the helper functions introduced in the last patch to do
  validation of capabilities found on the migration stream.

Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c              |  22 +-
 migration/migration.h              |   2 +-
 migration/options.c                | 318 ++++++++++++-----------------
 migration/options.h                |  19 +-
 migration/savevm.c                 |   8 +-
 tests/qtest/migration/misc-tests.c | 141 +++++++++++++
 6 files changed, 285 insertions(+), 225 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index c8e7e86ea0..69dd4f3d05 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -255,9 +255,10 @@ static bool
 migration_capabilities_and_transport_compatible(MigrationAddress *addr,
                                                 Error **errp)
 {
+    MigrationState *s = migrate_get_current();
+
     if (addr->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
-        return migrate_rdma_caps_check(migrate_get_current()->capabilities,
-                                       errp);
+        return migrate_rdma_caps_check(&s->parameters, errp);
     }
 
     return true;
@@ -4079,22 +4080,7 @@ static void migration_instance_init(Object *obj)
  */
 static bool migration_object_check(MigrationState *ms, Error **errp)
 {
-    /* Assuming all off */
-    bool old_caps[MIGRATION_CAPABILITY__MAX] = { 0 };
-
-    if (!migrate_params_check(&ms->parameters, errp)) {
-        return false;
-    }
-
-    /*
-     * FIXME: Temporarily while -global capabilties are still using
-     * s->capabilities. Will be gone by the end of the series.
-     */
-    for (int i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
-        migrate_capability_set_compat(&ms->parameters, i, ms->capabilities[i]);
-    }
-
-    return migrate_caps_check(old_caps, ms->capabilities, errp);
+    return migrate_params_check(&ms->parameters, errp);
 }
 
 static const TypeInfo migration_type = {
diff --git a/migration/migration.h b/migration/migration.h
index e47ff4e3d1..e1204b7f0d 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -357,7 +357,7 @@ struct MigrationState {
     /* Timestamp when VM is down (ms) to migrate the last stuff */
     int64_t downtime_start;
     int64_t downtime;
-    bool capabilities[MIGRATION_CAPABILITY__MAX];
+    int64_t expected_downtime;
     int64_t setup_time;
 
     /*
diff --git a/migration/options.c b/migration/options.c
index aff5d8f550..a25c9c2523 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -85,9 +85,6 @@
 #define DEFAULT_MIGRATE_ANNOUNCE_ROUNDS    5
 #define DEFAULT_MIGRATE_ANNOUNCE_STEP    100
 
-#define DEFINE_PROP_MIG_CAP(name, x)             \
-    DEFINE_PROP_BOOL(name, MigrationState, capabilities[x], false)
-
 const PropertyInfo qdev_prop_StrOrNull;
 #define DEFINE_PROP_STR_OR_NULL(_name, _state, _field)                  \
     DEFINE_PROP(_name, _state, _field, qdev_prop_StrOrNull, StrOrNull *, \
@@ -195,32 +192,42 @@ const Property migration_properties[] = {
                       parameters.x_rdma_chunk_size,
                       DEFAULT_MIGRATE_X_RDMA_CHUNK_SIZE),
 
-    /* Migration capabilities */
-    DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
-    DEFINE_PROP_MIG_CAP("x-rdma-pin-all", MIGRATION_CAPABILITY_RDMA_PIN_ALL),
-    DEFINE_PROP_MIG_CAP("x-auto-converge", MIGRATION_CAPABILITY_AUTO_CONVERGE),
-    DEFINE_PROP_MIG_CAP("x-events", MIGRATION_CAPABILITY_EVENTS),
-    DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM),
-    DEFINE_PROP_MIG_CAP("x-postcopy-preempt",
-                        MIGRATION_CAPABILITY_POSTCOPY_PREEMPT),
-    DEFINE_PROP_MIG_CAP("postcopy-blocktime",
-                        MIGRATION_CAPABILITY_POSTCOPY_BLOCKTIME),
-    DEFINE_PROP_MIG_CAP("x-colo", MIGRATION_CAPABILITY_X_COLO),
-    DEFINE_PROP_MIG_CAP("x-release-ram", MIGRATION_CAPABILITY_RELEASE_RAM),
-    DEFINE_PROP_MIG_CAP("x-return-path", MIGRATION_CAPABILITY_RETURN_PATH),
-    DEFINE_PROP_MIG_CAP("x-multifd", MIGRATION_CAPABILITY_MULTIFD),
-    DEFINE_PROP_MIG_CAP("x-background-snapshot",
-            MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT),
+    DEFINE_PROP_BOOL("x-xbzrle",
+                     MigrationState, parameters.xbzrle, false),
+    DEFINE_PROP_BOOL("x-rdma-pin-all",
+                     MigrationState, parameters.rdma_pin_all, false),
+    DEFINE_PROP_BOOL("x-auto-converge",
+                     MigrationState, parameters.auto_converge, false),
+    DEFINE_PROP_BOOL("x-events",
+                     MigrationState, parameters.events, false),
+    DEFINE_PROP_BOOL("x-postcopy-ram",
+                     MigrationState, parameters.postcopy_ram, false),
+    DEFINE_PROP_BOOL("x-postcopy-preempt",
+                     MigrationState, parameters.postcopy_preempt, false),
+    DEFINE_PROP_BOOL("postcopy-blocktime",
+                     MigrationState, parameters.postcopy_blocktime, false),
+    DEFINE_PROP_BOOL("x-colo",
+                     MigrationState, parameters.x_colo, false),
+    DEFINE_PROP_BOOL("x-release-ram",
+                     MigrationState, parameters.release_ram, false),
+    DEFINE_PROP_BOOL("x-return-path",
+                     MigrationState, parameters.return_path, false),
+    DEFINE_PROP_BOOL("x-multifd",
+                     MigrationState, parameters.multifd, false),
+    DEFINE_PROP_BOOL("x-background-snapshot",
+                     MigrationState, parameters.background_snapshot, false),
 #ifdef CONFIG_LINUX
-    DEFINE_PROP_MIG_CAP("x-zero-copy-send",
-            MIGRATION_CAPABILITY_ZERO_COPY_SEND),
+    DEFINE_PROP_BOOL("x-zero-copy-send",
+                     MigrationState, parameters.zero_copy_send, false),
 #endif
-    DEFINE_PROP_MIG_CAP("x-switchover-ack",
-                        MIGRATION_CAPABILITY_SWITCHOVER_ACK),
-    DEFINE_PROP_MIG_CAP("x-dirty-limit", MIGRATION_CAPABILITY_DIRTY_LIMIT),
-    DEFINE_PROP_MIG_CAP("mapped-ram", MIGRATION_CAPABILITY_MAPPED_RAM),
-    DEFINE_PROP_MIG_CAP("x-ignore-shared",
-                        MIGRATION_CAPABILITY_X_IGNORE_SHARED),
+    DEFINE_PROP_BOOL("x-switchover-ack",
+                     MigrationState, parameters.switchover_ack, false),
+    DEFINE_PROP_BOOL("x-dirty-limit",
+                     MigrationState, parameters.dirty_limit, false),
+    DEFINE_PROP_BOOL("mapped-ram",
+                     MigrationState, parameters.mapped_ram, false),
+    DEFINE_PROP_BOOL("x-ignore-shared",
+                     MigrationState, parameters.x_ignore_shared, false),
 };
 const size_t migration_properties_count = ARRAY_SIZE(migration_properties);
 
@@ -303,7 +310,7 @@ bool migrate_auto_converge(void)
 {
     MigrationState *s = migrate_get_current();
 
-    return s->capabilities[MIGRATION_CAPABILITY_AUTO_CONVERGE];
+    return s->parameters.auto_converge;
 }
 
 bool migrate_send_switchover_start(void)
@@ -317,144 +324,142 @@ bool migrate_background_snapshot(void)
 {
     MigrationState *s = migrate_get_current();
 
-    return s->capabilities[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT];
+    return s->parameters.background_snapshot;
 }
 
 bool migrate_colo(void)
 {
     MigrationState *s = migrate_get_current();
 
-    return s->capabilities[MIGRATION_CAPABILITY_X_COLO];
+    return s->parameters.x_colo;
 }
 
 bool migrate_dirty_bitmaps(void)
 {
     MigrationState *s = migrate_get_current();
 
-    return s->capabilities[MIGRATION_CAPABILITY_DIRTY_BITMAPS];
+    return s->parameters.dirty_bitmaps;
 }
 
 bool migrate_dirty_limit(void)
 {
     MigrationState *s = migrate_get_current();
 
-    return s->capabilities[MIGRATION_CAPABILITY_DIRTY_LIMIT];
+    return s->parameters.dirty_limit;
 }
 
 bool migrate_events(void)
 {
     MigrationState *s = migrate_get_current();
 
-    return s->capabilities[MIGRATION_CAPABILITY_EVENTS];
+    return s->parameters.events;
 }
 
 bool migrate_mapped_ram(void)
 {
     MigrationState *s = migrate_get_current();
 
-    return s->capabilities[MIGRATION_CAPABILITY_MAPPED_RAM];
+    return s->parameters.mapped_ram;
 }
 
 bool migrate_ignore_shared(void)
 {
     MigrationState *s = migrate_get_current();
 
-    return s->capabilities[MIGRATION_CAPABILITY_X_IGNORE_SHARED];
+    return s->parameters.x_ignore_shared;
 }
 
 bool migrate_late_block_activate(void)
 {
     MigrationState *s = migrate_get_current();
 
-    return s->capabilities[MIGRATION_CAPABILITY_LATE_BLOCK_ACTIVATE];
+    return s->parameters.late_block_activate;
 }
 
 bool migrate_multifd(void)
 {
     MigrationState *s = migrate_get_current();
 
-    return s->capabilities[MIGRATION_CAPABILITY_MULTIFD];
+    return s->parameters.multifd;
 }
 
 bool migrate_pause_before_switchover(void)
 {
     MigrationState *s = migrate_get_current();
 
-    return s->capabilities[MIGRATION_CAPABILITY_PAUSE_BEFORE_SWITCHOVER];
+    return s->parameters.pause_before_switchover;
 }
 
 bool migrate_postcopy_blocktime(void)
 {
     MigrationState *s = migrate_get_current();
 
-    return s->capabilities[MIGRATION_CAPABILITY_POSTCOPY_BLOCKTIME];
+    return s->parameters.postcopy_blocktime;
 }
 
 bool migrate_postcopy_preempt(void)
 {
     MigrationState *s = migrate_get_current();
 
-    return s->capabilities[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT];
+    return s->parameters.postcopy_preempt;
 }
 
 bool migrate_postcopy_ram(void)
 {
     MigrationState *s = migrate_get_current();
 
-    return s->capabilities[MIGRATION_CAPABILITY_POSTCOPY_RAM];
+    return s->parameters.postcopy_ram;
 }
 
 bool migrate_rdma_pin_all(void)
 {
     MigrationState *s = migrate_get_current();
 
-    return s->capabilities[MIGRATION_CAPABILITY_RDMA_PIN_ALL];
+    return s->parameters.rdma_pin_all;
 }
 
 bool migrate_release_ram(void)
 {
     MigrationState *s = migrate_get_current();
 
-    return s->capabilities[MIGRATION_CAPABILITY_RELEASE_RAM];
+    return s->parameters.release_ram;
 }
 
 bool migrate_return_path(void)
 {
     MigrationState *s = migrate_get_current();
 
-    return s->capabilities[MIGRATION_CAPABILITY_RETURN_PATH];
+    return s->parameters.return_path;
 }
 
 bool migrate_switchover_ack(void)
 {
     MigrationState *s = migrate_get_current();
 
-    return s->capabilities[MIGRATION_CAPABILITY_SWITCHOVER_ACK];
+    return s->parameters.switchover_ack;
 }
 
 bool migrate_validate_uuid(void)
 {
     MigrationState *s = migrate_get_current();
 
-    return s->capabilities[MIGRATION_CAPABILITY_VALIDATE_UUID];
+    return s->parameters.validate_uuid;
 }
 
 bool migrate_xbzrle(void)
 {
     MigrationState *s = migrate_get_current();
 
-    return s->capabilities[MIGRATION_CAPABILITY_XBZRLE];
+    return s->parameters.xbzrle;
 }
 
 bool migrate_zero_copy_send(void)
 {
     MigrationState *s = migrate_get_current();
 
-    return s->capabilities[MIGRATION_CAPABILITY_ZERO_COPY_SEND];
+    return s->parameters.zero_copy_send;
 }
 
-/* pseudo capabilities */
-
 bool migrate_multifd_flush_after_each_section(void)
 {
     MigrationState *s = migrate_get_current();
@@ -506,44 +511,6 @@ WriteTrackingSupport migrate_query_write_tracking(void)
     return WT_SUPPORT_COMPATIBLE;
 }
 
-/* Migration capabilities set */
-struct MigrateCapsSet {
-    int size;                       /* Capability set size */
-    MigrationCapability caps[];     /* Variadic array of capabilities */
-};
-typedef struct MigrateCapsSet MigrateCapsSet;
-
-/* Define and initialize MigrateCapsSet */
-#define INITIALIZE_MIGRATE_CAPS_SET(_name, ...)   \
-    MigrateCapsSet _name = {    \
-        .size = sizeof((int []) { __VA_ARGS__ }) / sizeof(int), \
-        .caps = { __VA_ARGS__ } \
-    }
-
-/* Background-snapshot compatibility check list */
-static const
-INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot,
-    MIGRATION_CAPABILITY_POSTCOPY_RAM,
-    MIGRATION_CAPABILITY_DIRTY_BITMAPS,
-    MIGRATION_CAPABILITY_POSTCOPY_BLOCKTIME,
-    MIGRATION_CAPABILITY_LATE_BLOCK_ACTIVATE,
-    MIGRATION_CAPABILITY_RETURN_PATH,
-    MIGRATION_CAPABILITY_MULTIFD,
-    MIGRATION_CAPABILITY_PAUSE_BEFORE_SWITCHOVER,
-    MIGRATION_CAPABILITY_AUTO_CONVERGE,
-    MIGRATION_CAPABILITY_RELEASE_RAM,
-    MIGRATION_CAPABILITY_RDMA_PIN_ALL,
-    MIGRATION_CAPABILITY_XBZRLE,
-    MIGRATION_CAPABILITY_X_COLO,
-    MIGRATION_CAPABILITY_VALIDATE_UUID,
-    MIGRATION_CAPABILITY_ZERO_COPY_SEND);
-
-/* Snapshot compatibility check list */
-static const
-INITIALIZE_MIGRATE_CAPS_SET(check_caps_savevm,
-                            MIGRATION_CAPABILITY_MULTIFD,
-);
-
 static bool migrate_incoming_started(void)
 {
     return !!migration_incoming_get_current()->transport_data;
@@ -552,34 +519,28 @@ static bool migrate_incoming_started(void)
 bool migrate_can_snapshot(Error **errp)
 {
     MigrationState *s = migrate_get_current();
-    int i;
 
-    for (i = 0; i < check_caps_savevm.size; i++) {
-        int incomp_cap = check_caps_savevm.caps[i];
-
-        if (s->capabilities[incomp_cap]) {
-            error_setg(errp,
-                       "Snapshots are not compatible with %s",
-                       MigrationCapability_str(incomp_cap));
-            return false;
-        }
+    if (migrate_capability_get_compat(
+            &s->parameters, MIGRATION_CAPABILITY_MULTIFD)) {
+        error_setg(errp,
+                   "Snapshots are not compatible with multifd");
+        return false;
     }
 
     return true;
 }
 
-
-bool migrate_rdma_caps_check(bool *caps, Error **errp)
+bool migrate_rdma_caps_check(MigrationParameters *params, Error **errp)
 {
-    if (caps[MIGRATION_CAPABILITY_XBZRLE]) {
+    if (params->xbzrle) {
         error_setg(errp, "RDMA and XBZRLE can't be used together");
         return false;
     }
-    if (caps[MIGRATION_CAPABILITY_MULTIFD]) {
+    if (params->multifd) {
         error_setg(errp, "RDMA and multifd can't be used together");
         return false;
     }
-    if (caps[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
+    if (params->postcopy_ram) {
         error_setg(errp, "RDMA and postcopy-ram can't be used together");
         return false;
     }
@@ -587,30 +548,23 @@ bool migrate_rdma_caps_check(bool *caps, Error **errp)
     return true;
 }
 
-/**
- * @migration_caps_check - check capability compatibility
- *
- * @old_caps: old capability list
- * @new_caps: new capability list
- * @errp: set *errp if the check failed, with reason
- *
- * Returns true if check passed, otherwise false.
- */
-bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
+bool migrate_caps_check(MigrationParameters *new, Error **errp)
 {
-    ERRP_GUARD();
+    MigrationState *s = migrate_get_current();
     MigrationIncomingState *mis = migration_incoming_get_current();
+    bool postcopy_already_on = s->parameters.postcopy_ram;
+    ERRP_GUARD();
 
 #ifdef CONFIG_REPLICATION
-    if (new_caps[MIGRATION_CAPABILITY_X_COLO]) {
-        if (!new_caps[MIGRATION_CAPABILITY_RETURN_PATH]) {
+    if (new->x_colo) {
+        if (!new->return_path) {
             error_setg(errp, "Capability 'x-colo' requires capability "
                              "'return-path'");
             return false;
         }
     }
 #else
-    if (new_caps[MIGRATION_CAPABILITY_X_COLO]) {
+    if (new->x_colo) {
         error_setg(errp, "QEMU compiled without replication module"
                    " can't enable COLO");
         error_append_hint(errp, "Please enable replication before COLO.\n");
@@ -618,27 +572,27 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
     }
 #endif
 
-    if (new_caps[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
+    if (new->postcopy_ram) {
         /* This check is reasonably expensive, so only when it's being
          * set the first time, also it's only the destination that needs
          * special support.
          */
-        if (!old_caps[MIGRATION_CAPABILITY_POSTCOPY_RAM] &&
+        if (!postcopy_already_on &&
             runstate_check(RUN_STATE_INMIGRATE) &&
             !postcopy_ram_supported_by_host(mis, errp)) {
             error_prepend(errp, "Postcopy is not supported: ");
             return false;
         }
 
-        if (new_caps[MIGRATION_CAPABILITY_X_IGNORE_SHARED]) {
+        if (new->x_ignore_shared) {
             error_setg(errp, "Postcopy is not compatible with ignore-shared");
             return false;
         }
     }
 
-    if (new_caps[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT]) {
+    if (new->background_snapshot) {
         WriteTrackingSupport wt_support;
-        int idx;
+
         /*
          * Check if 'background-snapshot' capability is supported by
          * host kernel and compatible with guest memory configuration.
@@ -654,41 +608,45 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
             return false;
         }
 
-        /*
-         * Check if there are any migration capabilities
-         * incompatible with 'background-snapshot'.
-         */
-        for (idx = 0; idx < check_caps_background_snapshot.size; idx++) {
-            int incomp_cap = check_caps_background_snapshot.caps[idx];
-            if (new_caps[incomp_cap]) {
-                error_setg(errp,
-                        "Background-snapshot is not compatible with %s",
-                        MigrationCapability_str(incomp_cap));
-                return false;
-            }
+        if (new->postcopy_ram ||
+            new->dirty_bitmaps ||
+            new->postcopy_blocktime ||
+            new->late_block_activate ||
+            new->return_path ||
+            new->multifd ||
+            new->pause_before_switchover ||
+            new->auto_converge ||
+            new->release_ram ||
+            new->rdma_pin_all ||
+            new->xbzrle ||
+            new->x_colo ||
+            new->validate_uuid ||
+            new->zero_copy_send) {
+            error_setg(errp,
+                       "Background-snapshot is not compatible with "
+                       "currently set capabilities");
+            return false;
         }
     }
 
 #ifdef CONFIG_LINUX
-    if (new_caps[MIGRATION_CAPABILITY_ZERO_COPY_SEND] &&
-        (!new_caps[MIGRATION_CAPABILITY_MULTIFD] ||
-         new_caps[MIGRATION_CAPABILITY_XBZRLE] ||
-         migrate_multifd_compression() ||
-         migrate_tls())) {
+    if (new->zero_copy_send &&
+        (!new->multifd || new->xbzrle ||
+         migrate_multifd_compression() || migrate_tls())) {
         error_setg(errp,
                    "Zero copy only available for non-compressed non-TLS multifd migration");
         return false;
     }
 #else
-    if (new_caps[MIGRATION_CAPABILITY_ZERO_COPY_SEND]) {
+    if (new->zero_copy_send) {
         error_setg(errp,
                    "Zero copy currently only available on Linux");
         return false;
     }
 #endif
 
-    if (new_caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT]) {
-        if (!new_caps[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
+    if (new->postcopy_preempt) {
+        if (!new->postcopy_ram) {
             error_setg(errp, "Postcopy preempt requires postcopy-ram");
             return false;
         }
@@ -700,22 +658,22 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
         }
     }
 
-    if (new_caps[MIGRATION_CAPABILITY_MULTIFD]) {
+    if (new->multifd) {
         if (!migrate_multifd() && migrate_incoming_started()) {
             error_setg(errp, "Multifd must be set before incoming starts");
             return false;
         }
     }
 
-    if (new_caps[MIGRATION_CAPABILITY_SWITCHOVER_ACK]) {
-        if (!new_caps[MIGRATION_CAPABILITY_RETURN_PATH]) {
+    if (new->switchover_ack) {
+        if (!new->return_path) {
             error_setg(errp, "Capability 'switchover-ack' requires capability "
                              "'return-path'");
             return false;
         }
     }
-    if (new_caps[MIGRATION_CAPABILITY_DIRTY_LIMIT]) {
-        if (new_caps[MIGRATION_CAPABILITY_AUTO_CONVERGE]) {
+    if (new->dirty_limit) {
+        if (new->auto_converge) {
             error_setg(errp, "dirty-limit conflicts with auto-converge"
                        " either of then available currently");
             return false;
@@ -728,30 +686,29 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
         }
     }
 
-    if (new_caps[MIGRATION_CAPABILITY_MULTIFD]) {
-        if (new_caps[MIGRATION_CAPABILITY_XBZRLE]) {
+    if (new->multifd) {
+        if (new->xbzrle) {
             error_setg(errp, "Multifd is not compatible with xbzrle");
             return false;
         }
     }
 
-    if (new_caps[MIGRATION_CAPABILITY_MAPPED_RAM]) {
-        if (new_caps[MIGRATION_CAPABILITY_XBZRLE]) {
+    if (new->mapped_ram) {
+        if (new->xbzrle) {
             error_setg(errp,
                        "Mapped-ram migration is incompatible with xbzrle");
             return false;
         }
     }
 
-    if (new_caps[MIGRATION_CAPABILITY_MAPPED_RAM] &&
-        new_caps[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
-        if (new_caps[MIGRATION_CAPABILITY_MULTIFD]) {
+    if (new->mapped_ram && new->postcopy_ram) {
+        if (new->multifd) {
             error_setg(errp,
                        "Multifd is not supported with fast snapshot load");
             return false;
         }
 
-        if (new_caps[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT]) {
+        if (new->postcopy_preempt) {
             error_setg(
                 errp,
                 "Postcopy Preempt is incompatible with fast snapshot load");
@@ -769,7 +726,7 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
      * On destination side, check the cases that capability is being set
      * after incoming thread has started.
      */
-    if (migrate_rdma() && !migrate_rdma_caps_check(new_caps, errp)) {
+    if (migrate_rdma() && !migrate_rdma_caps_check(new, errp)) {
         return false;
     }
     return true;
@@ -831,39 +788,37 @@ MigrationCapabilityStatusList *qmp_query_migrate_capabilities(Error **errp)
     for (i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
         caps = g_malloc0(sizeof(*caps));
         caps->capability = i;
-        caps->state = s->capabilities[i];
+        caps->state = migrate_capability_get_compat(&s->parameters, i);
         QAPI_LIST_APPEND(tail, caps);
     }
 
     return head;
 }
 
-void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
+void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *capabilities,
                                   Error **errp)
 {
     MigrationState *s = migrate_get_current();
-    MigrationCapabilityStatusList *cap;
-    bool new_caps[MIGRATION_CAPABILITY__MAX];
+    g_autoptr(MigrationParameters) params = NULL;
 
     if (migration_is_running() || migration_in_colo_state()) {
         error_setg(errp, "There's a migration process in progress");
         return;
     }
 
-    memcpy(new_caps, s->capabilities, sizeof(new_caps));
-    for (cap = params; cap; cap = cap->next) {
-        new_caps[cap->value->capability] = cap->value->state;
-    }
+    /*
+     * Capabilities validation needs to first copy from s->parameters
+     * in case the incoming capabilities have a capability that
+     * conflicts with another that's already set.
+     */
+    params = QAPI_CLONE(MigrationParameters, &s->parameters);
+    migrate_capabilities_set_compat(params, capabilities);
 
-    if (!migrate_caps_check(s->capabilities, new_caps, errp)) {
+    if (!migrate_caps_check(params, errp)) {
         return;
     }
 
-    for (cap = params; cap; cap = cap->next) {
-        s->capabilities[cap->value->capability] = cap->value->state;
-    }
-
-    migrate_capabilities_set_compat(&s->parameters, params);
+    migrate_capabilities_set_compat(&s->parameters, capabilities);
 }
 
 /* parameters */
@@ -924,9 +879,8 @@ bool migrate_direct_io(void)
      * isolated to the main migration thread while multifd channels
      * process the aligned data with O_DIRECT enabled.
      */
-    return s->parameters.direct_io &&
-        s->capabilities[MIGRATION_CAPABILITY_MAPPED_RAM] &&
-        s->capabilities[MIGRATION_CAPABILITY_MULTIFD];
+    return s->parameters.direct_io && s->parameters.mapped_ram &&
+        s->parameters.multifd;
 }
 
 uint64_t migrate_downtime_limit(void)
@@ -1451,6 +1405,10 @@ bool migrate_params_check(MigrationParameters *params, Error **errp)
         return false;
     }
 
+    if (!migrate_caps_check(params, errp)) {
+        return false;
+    }
+
     return true;
 }
 
@@ -1471,20 +1429,6 @@ void qmp_migrate_set_parameters(MigrationParameters *input, Error **errp)
     tls_opt_to_str(input->tls_hostname);
     tls_opt_to_str(input->tls_authz);
 
-    /*
-     * FIXME: Temporarily while migrate_caps_check is not
-     * converted to look at s->parameters. Will be gone the end of
-     * the series.
-     */
-    bool new_caps[MIGRATION_CAPABILITY__MAX] = { 0 };
-    for (int i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
-        new_caps[i] = migrate_capability_get_compat(cur, i);
-    }
-    if (!migrate_caps_check(migrate_get_current()->capabilities, new_caps,
-                            errp)) {
-        return;
-    }
-
     /* merge input on top of current */
     if (!migrate_params_merge(cur, input, &new, errp)) {
         return;
diff --git a/migration/options.h b/migration/options.h
index f7bf56e9ff..28cf762ace 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -1,5 +1,5 @@
 /*
- * QEMU migration capabilities
+ * QEMU migration options
  *
  * Copyright (c) 2012-2023 Red Hat Inc
  *
@@ -23,8 +23,6 @@
 extern const Property migration_properties[];
 extern const size_t migration_properties_count;
 
-/* capabilities */
-
 bool migrate_auto_converge(void);
 bool migrate_colo(void);
 bool migrate_dirty_bitmaps(void);
@@ -43,22 +41,12 @@ bool migrate_validate_uuid(void);
 bool migrate_xbzrle(void);
 bool migrate_zero_copy_send(void);
 
-/*
- * pseudo capabilities
- *
- * These are functions that are used in a similar way to capabilities
- * check, but they are not a capability.
- */
-
 bool migrate_multifd_flush_after_each_section(void);
 bool migrate_postcopy(void);
 bool migrate_rdma(void);
 bool migrate_tls(void);
 
-/* capabilities helpers */
-
-bool migrate_rdma_caps_check(bool *caps, Error **errp);
-bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp);
+bool migrate_rdma_caps_check(MigrationParameters *config, Error **errp);
 bool migrate_can_snapshot(Error **errp);
 
 /* parameters */
@@ -89,8 +77,6 @@ uint64_t migrate_xbzrle_cache_size(void);
 ZeroPageDetection migrate_zero_page_detection(void);
 uint64_t migrate_rdma_chunk_size(void);
 
-/* parameters helpers */
-
 bool migrate_params_check(MigrationParameters *params, Error **errp);
 void migrate_params_init(MigrationParameters *params);
 bool migrate_params_free(MigrationParameters *params, Error **errp);
@@ -100,4 +86,5 @@ void migrate_capability_set_compat(MigrationParameters *params, int i,
                                    bool val);
 void migrate_capabilities_set_compat(MigrationParameters *params,
                                      MigrationCapabilityStatusList *caps);
+bool migrate_caps_check(MigrationParameters *new, Error **errp);
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 5b0e89ca7c..c284b76f60 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -291,7 +291,8 @@ static uint32_t get_validatable_capabilities_count(void)
     uint32_t result = 0;
     int i;
     for (i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
-        if (should_validate_capability(i) && s->capabilities[i]) {
+        if (should_validate_capability(i) &&
+            migrate_capability_get_compat(&s->parameters, i)) {
             result++;
         }
     }
@@ -313,7 +314,8 @@ static bool configuration_pre_save(void *opaque, Error **errp)
     state->capabilities = g_renew(MigrationCapability, state->capabilities,
                                   state->caps_count);
     for (i = j = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
-        if (should_validate_capability(i) && s->capabilities[i]) {
+        if (should_validate_capability(i) &&
+            migrate_capability_get_compat(&s->parameters, i)) {
             state->capabilities[j++] = i;
         }
     }
@@ -362,7 +364,7 @@ static bool configuration_validate_capabilities(SaveState *state)
             continue;
         }
         source_state = test_bit(i, source_caps_bm);
-        target_state = s->capabilities[i];
+        target_state = migrate_capability_get_compat(&s->parameters, i);
         if (source_state != target_state) {
             error_report("Capability %s is %s, but received capability is %s",
                          MigrationCapability_str(i),
diff --git a/tests/qtest/migration/misc-tests.c b/tests/qtest/migration/misc-tests.c
index 4ac2f42a5a..b1179d3af7 100644
--- a/tests/qtest/migration/misc-tests.c
+++ b/tests/qtest/migration/misc-tests.c
@@ -55,6 +55,147 @@ HMPTestData test_cases[] = {
     /* bool */
     TEST("cpu-throttle-tailslow", "on", "on"),
     TEST("direct-io", "on", "on"),
+    TEST("events", "on", "on"),
+
+    /* bool, with dependencies */
+
+    /*
+     * background-snapshot:
+     *  rejects dirty-bitmaps
+     *  rejects postcopy-blocktime
+     *  rejects late-block-activate
+     *  rejects multifd
+     *  rejects pause-before-switchover
+     *  rejects auto-converge
+     *  rejects release-ram
+     *  rejects rdma-pin-all
+     *  rejects validate-uuid
+     *  rejects zero-copy-send
+     *  rejects postcopy-ram
+     */
+    TEST("background-snapshot", "on", "on"),
+    TEST("dirty-bitmaps", "on", BG_SNAP_MSG),
+    TEST("postcopy-blocktime", "on", BG_SNAP_MSG),
+    TEST("late-block-activate", "on", BG_SNAP_MSG),
+    TEST("multifd", "on", BG_SNAP_MSG),
+    TEST("pause-before-switchover", "on", BG_SNAP_MSG),
+    TEST("auto-converge", "on", BG_SNAP_MSG),
+    TEST("release-ram", "on", BG_SNAP_MSG),
+    TEST("rdma-pin-all", "on", BG_SNAP_MSG),
+    TEST("validate-uuid", "on", BG_SNAP_MSG),
+    TEST("zero-copy-send", "on", BG_SNAP_MSG),
+    TEST("postcopy-ram", "on", BG_SNAP_MSG),
+    TEST("background-snapshot", "off", "off"),
+
+    TEST("dirty-bitmaps", "on", "on"),
+    TEST("postcopy-blocktime", "on", "on"),
+    TEST("late-block-activate", "on", "on"),
+    TEST("pause-before-switchover", "on", "on"),
+    TEST("auto-converge", "on", "on"),
+    TEST("release-ram", "on", "on"),
+    TEST("rdma-pin-all", "on", "on"),
+    TEST("validate-uuid", "on", "on"),
+
+    /*
+     * postcopy-preempt
+     *  requires postcopy-ram:
+     */
+    TEST("postcopy-preempt", "on",
+         "Error: Postcopy preempt requires postcopy-ram"),
+
+    /*
+     * postcopy-ram:
+     *  required by postcopy-preempt
+     *  rejected by x-ignore-shared
+     *  rejected by background-snapshot
+     *  rejected by mapped-ram
+     */
+    TEST("postcopy-ram", "on", "on"),
+    TEST("postcopy-preempt", "on", "on"),
+    TEST("x-ignore-shared", "on",
+         "Error: Postcopy is not compatible with ignore-shared"),
+    TEST("background-snapshot", "on", BG_SNAP_MSG),
+    TEST("mapped-ram", "on",
+         "Error: Postcopy Preempt is incompatible with fast snapshot load"),
+    TEST("postcopy-ram", "off",
+         "Error: Postcopy preempt requires postcopy-ram"),
+    TEST("postcopy-preempt", "off", "off"),
+    TEST("postcopy-ram", "off", "off"),
+
+    /*
+     * x-ignore-shared:
+     *  rejected by postcopy-ram
+     */
+    TEST("x-ignore-shared", "on", "on"),
+    TEST("postcopy-ram", "on",
+         "Error: Postcopy is not compatible with ignore-shared"),
+    TEST("x-ignore-shared", "off", "off"),
+
+    /*
+     * return-path:
+     *  required by x-colo
+     *  required by switchover-ack
+     *  rejected by background-snapshot
+     */
+    TEST("return-path", "on", "on"),
+    TEST("x-colo", "on", "on"),
+    TEST("switchover-ack", "on", "on"),
+    TEST("background-snapshot", "on", BG_SNAP_MSG),
+
+    TEST("return-path", "off",
+         "Error: Capability 'x-colo' requires capability 'return-path'"),
+    TEST("x-colo", "off", "off"),
+
+    TEST("return-path", "off",
+         "Error: Capability 'switchover-ack' requires capability "
+         "'return-path'"),
+    TEST("switchover-ack", "off", "off"),
+    TEST("return-path", "off", "off"),
+
+    TEST("x-colo", "on",
+         "Error: Capability 'x-colo' requires capability 'return-path'"),
+    TEST("switchover-ack", "on", "Error: Capability 'switchover-ack' requires "
+         "capability 'return-path'"),
+
+    /*
+     * xbzrle:
+     *  rejected by multifd
+     */
+    TEST("xbzrle", "on", "on"),
+    TEST("multifd", "on", "Error: Multifd is not compatible with xbzrle"),
+    TEST("xbzrle", "off", "off"),
+
+    /*
+     * multifd:
+     *  rejected by xbzrle
+     *  required by zero-copy-send
+     */
+    TEST("multifd", "on", "on"),
+    TEST("xbzrle", "on", "Error: Multifd is not compatible with xbzrle"),
+    TEST("zero-copy-send", "on", "on"),
+    TEST("multifd", "off", "Error: Zero copy only available for "
+         "non-compressed non-TLS multifd migration"),
+    TEST("zero-copy-send", "off", "off"),
+    TEST("multifd", "off", "off"),
+
+    /*
+     * auto-converge:
+     *  rejected by dirty-limit
+     */
+    TEST("auto-converge", "on", "on"),
+    TEST("dirty-limit", "on",
+         "Error: dirty-limit conflicts with auto-converge "
+         "either of then available currently"),
+    TEST("auto-converge", "off", "off"),
+
+    /*
+     * dirty-limit:
+     *  rejected by auto-converge
+     *  requires KVM acceleration
+     */
+    TEST("dirty-limit", "on",
+         "Error: dirty-limit requires KVM with accelerator "
+         "property 'dirty-ring-size' set"),
 
     /* uint64_t */
     TEST("announce-initial", "60", "60"),
-- 
2.53.0

[PATCH v2 18/18] qapi/migration: Deprecate capabilities commands

[Fabiano Rosas]

The concept of capabilities is being merged into the concept of
parameters. From now on, the commands that handle capabilities are
deprecated in favor of the commands that handle parameters.

Affected commands:

- migrate-set-capabilities
- query-migrate-capabilities

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 docs/about/deprecated.rst      | 13 +++++++++++++
 migration/migration-hmp-cmds.c |  6 ++++++
 qapi/migration.json            | 16 ++++++++++++++--
 3 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 98c32991c9..1de1efcd9f 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -498,3 +498,16 @@ If the user requests a modern x86 CPU model (i.e. not one of ``486``,
 ``athlon``, ``kvm32``, ``pentium``, ``pentium2``, ``pentium3``or ``qemu32``)
 a warning will be displayed until a future QEMU version when such CPUs will
 be rejected.
+
+Migration
+---------
+
+``migrate-set-capabilities`` command (since 11.1)
+'''''''''''''''''''''''''''''''''''''''''''''''''
+
+Use ``migrate-set-parameters`` instead.
+
+``query-migrate-capabilities`` command (since 11.1)
+'''''''''''''''''''''''''''''''''''''''''''''''''''
+
+Use ``query-migrate-parameters`` instead.
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 720913255f..bd3713dc7a 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -309,6 +309,9 @@ void hmp_info_migrate_capabilities(MonitorHMP *hmp, const QDict *qdict)
 {
     MigrationCapabilityStatusList *caps, *cap;
 
+    warn_report("info migrate_capabilities is deprecated;"
+                " use info migrate_parameters instead");
+
     caps = qmp_query_migrate_capabilities(NULL);
 
     if (caps) {
@@ -516,6 +519,9 @@ void hmp_migrate_set_capability(MonitorHMP *hmp, const QDict *qdict)
     MigrationCapabilityStatus *value;
     int val;
 
+    warn_report("migrate_set_capability is deprecated;"
+                " use migrate_set_parameter instead");
+
     val = qapi_enum_parse(&MigrationCapability_lookup, cap, -1, &err);
     if (val < 0) {
         goto end;
diff --git a/qapi/migration.json b/qapi/migration.json
index 5f4ad965d6..d622b47244 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -564,6 +564,11 @@
 #
 # @capabilities: json array of capability modifications to make
 #
+# Features:
+#
+# @deprecated: This command is deprecated.  Use migrate-set-parameters
+# instead.
+#
 # Since: 1.2
 #
 # .. qmp-example::
@@ -573,13 +578,19 @@
 #     <- { "return": {} }
 ##
 { 'command': 'migrate-set-capabilities',
-  'data': { 'capabilities': ['MigrationCapabilityStatus'] } }
+  'data': { 'capabilities': ['MigrationCapabilityStatus'] },
+  'features': ['deprecated'] }
 
 ##
 # @query-migrate-capabilities:
 #
 # Return information about the current migration capabilities status
 #
+# Features:
+#
+# @deprecated: This command is deprecated.  Use
+# query-migrate-parameters instead.
+#
 # Since: 1.2
 #
 # .. qmp-example::
@@ -594,7 +605,8 @@
 #           {"state": false, "capability": "x-colo"}
 #        ]}
 ##
-{ 'command': 'query-migrate-capabilities', 'returns':   ['MigrationCapabilityStatus']}
+{ 'command': 'query-migrate-capabilities', 'returns':   ['MigrationCapabilityStatus'],
+  'features': ['deprecated'] }
 
 ##
 # @MultiFDCompression:
-- 
2.53.0

Re: [PATCH v2 00/18] migration: MigrationParameters changes

[Peter Xu]

On Wed, Sep 09, 2026 at 06:44:50PM -0300, Fabiano Rosas wrote:
> Fabiano Rosas (18):
>   checkpatch: Fix checking of newlines in error messages
>   migration/options.c: Don't export migrate_tls_opts_free
>   migration: Rename variables in qmp_migrate_set_parameters
>   migration: Use QAPI_CLONE_MEMBERS in migrate_params_apply
>   migration: Merge parameter structs instead of assigning one by one
>   migration: Open code migrate_params_apply
>   migration: Stop freeing s->parameters members individually
>   migration: Use migrate_params_free during finalize
>   tests/qtest/migration: Add a test for HMP
>   tests/qtest/migration: Add a test for HMP completion
>   migration: HMP: Fix bandwidth parameters
>   migration: Change HMP 'info migrate_parameters' output
>   migration: Use keyval input visitor in HMP set command
>   migration: Use output visitor in info command
>   migration: Rewrite migrate_set_parameter_completion using QDict
>   migration: Add capabilities into MigrationParameters
>   migration: Remove s->capabilities
>   qapi/migration: Deprecate capabilities commands

The new HMP test may fail when ran without userfaultfd privilege.

$ QTEST_QEMU_BINARY=./qemu-system-x86_64 ./tests/qtest/migration-test --full -r /x86_64/migration/hmp
TAP version 14
# random seed: R02S0ff0d2920ac5e3bffdb3b5a776110e91
# starting QEMU: exec ./qemu-system-x86_64 -qtest unix:/tmp/qtest-219499.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-219499.qmp,id=char0 -object monitor-qmp,id=qmp0,chardev=char0 -display none -audio none -run-with exit-with-parent=on -machine none  -accel qtest
# Skipping test: userfaultfd not available
# Start of x86_64 tests
# Start of migration tests
# Start of hmp tests
# Running /x86_64/migration/hmp/parameters
# starting QEMU: exec ./qemu-system-x86_64 -qtest unix:/tmp/qtest-219499.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-219499.qmp,id=char0 -object monitor-qmp,id=qmp0,chardev=char0 -display none -audio none -run-with exit-with-parent=on -accel tcg -nodefaults -S  -accel qtest
# HMP command failed:
# 
# Error: Background-snapshot is not supported by host kernel
# 
**
ERROR:../tests/qtest/migration/misc-tests.c:376:assert_hmp_success: code should not be reached
not ok /x86_64/migration/hmp/parameters - ERROR:../tests/qtest/migration/misc-tests.c:376:assert_hmp_success: code should not be reached
Bail out!
Aborted                    (core dumped) QTEST_QEMU_BINARY=./qemu-system-x86_64 ./tests/qtest/migration-test --full -r /x86_64/migration/hmp

We can either fix it (but I don't see easy way to wrap some TESTs to run
conditionally), or drop the verifications for background-snapshot cap for
now.

-- 
Peter Xu

Re: [PATCH v2 00/18] migration: MigrationParameters changes

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Sep 09, 2026 at 06:44:50PM -0300, Fabiano Rosas wrote:
>> Fabiano Rosas (18):
>>   checkpatch: Fix checking of newlines in error messages
>>   migration/options.c: Don't export migrate_tls_opts_free
>>   migration: Rename variables in qmp_migrate_set_parameters
>>   migration: Use QAPI_CLONE_MEMBERS in migrate_params_apply
>>   migration: Merge parameter structs instead of assigning one by one
>>   migration: Open code migrate_params_apply
>>   migration: Stop freeing s->parameters members individually
>>   migration: Use migrate_params_free during finalize
>>   tests/qtest/migration: Add a test for HMP
>>   tests/qtest/migration: Add a test for HMP completion
>>   migration: HMP: Fix bandwidth parameters
>>   migration: Change HMP 'info migrate_parameters' output
>>   migration: Use keyval input visitor in HMP set command
>>   migration: Use output visitor in info command
>>   migration: Rewrite migrate_set_parameter_completion using QDict
>>   migration: Add capabilities into MigrationParameters
>>   migration: Remove s->capabilities
>>   qapi/migration: Deprecate capabilities commands
>
> The new HMP test may fail when ran without userfaultfd privilege.
>
> $ QTEST_QEMU_BINARY=./qemu-system-x86_64 ./tests/qtest/migration-test --full -r /x86_64/migration/hmp
> TAP version 14
> # random seed: R02S0ff0d2920ac5e3bffdb3b5a776110e91
> # starting QEMU: exec ./qemu-system-x86_64 -qtest unix:/tmp/qtest-219499.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-219499.qmp,id=char0 -object monitor-qmp,id=qmp0,chardev=char0 -display none -audio none -run-with exit-with-parent=on -machine none  -accel qtest
> # Skipping test: userfaultfd not available
> # Start of x86_64 tests
> # Start of migration tests
> # Start of hmp tests
> # Running /x86_64/migration/hmp/parameters
> # starting QEMU: exec ./qemu-system-x86_64 -qtest unix:/tmp/qtest-219499.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-219499.qmp,id=char0 -object monitor-qmp,id=qmp0,chardev=char0 -display none -audio none -run-with exit-with-parent=on -accel tcg -nodefaults -S  -accel qtest
> # HMP command failed:
> # 
> # Error: Background-snapshot is not supported by host kernel
> # 
> **
> ERROR:../tests/qtest/migration/misc-tests.c:376:assert_hmp_success: code should not be reached
> not ok /x86_64/migration/hmp/parameters - ERROR:../tests/qtest/migration/misc-tests.c:376:assert_hmp_success: code should not be reached
> Bail out!
> Aborted                    (core dumped) QTEST_QEMU_BINARY=./qemu-system-x86_64 ./tests/qtest/migration-test --full -r /x86_64/migration/hmp
>
> We can either fix it (but I don't see easy way to wrap some TESTs to run
> conditionally), or drop the verifications for background-snapshot cap for
> now.

Thank you for testing this. I think I can just move the array into the
test function and check env->has_uffd in there.