diff mbox series

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

Message ID 20260902221547.1812481-13-farosas@suse.de
State New
Headers show
Series migration: MigrationParameters changes | expand

Commit Message

Fabiano Rosas Sept. 2, 2026, 10:15 p.m. UTC 
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.

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

Comments

Peter Xu Sept. 3, 2026, 8:25 p.m. UTC | #1 
On Wed, Sep 02, 2026 at 07:15:40PM -0300, Fabiano Rosas wrote:
> 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.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

HMP info commands are less of a worry.  I think this is ok, but I didn't
check the block layer details.  From migration side:

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

Patch

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 220ac28b5e3..089c6d4ff46 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -336,16 +336,16 @@  void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
     params = qmp_query_migrate_parameters(NULL);
 
     if (params) {
-        monitor_printf(mon, "%s: %" PRIu64 " ms\n",
+        monitor_printf(mon, "%s: %" PRIu64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_INITIAL),
             params->announce_initial);
-        monitor_printf(mon, "%s: %" PRIu64 " ms\n",
+        monitor_printf(mon, "%s: %" PRIu64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_MAX),
             params->announce_max);
         monitor_printf(mon, "%s: %" PRIu64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_ROUNDS),
             params->announce_rounds);
-        monitor_printf(mon, "%s: %" PRIu64 " ms\n",
+        monitor_printf(mon, "%s: %" PRIu64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_STEP),
             params->announce_step);
         assert(params->has_throttle_trigger_threshold);
@@ -369,35 +369,35 @@  void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
             MigrationParameter_str(MIGRATION_PARAMETER_MAX_CPU_THROTTLE),
             params->max_cpu_throttle);
         assert(params->tls_creds);
-        monitor_printf(mon, "%s: '%s'\n",
+        monitor_printf(mon, "%s: %s\n",
             MigrationParameter_str(MIGRATION_PARAMETER_TLS_CREDS),
                        params->tls_creds->u.s);
         assert(params->tls_hostname);
-        monitor_printf(mon, "%s: '%s'\n",
+        monitor_printf(mon, "%s: %s\n",
             MigrationParameter_str(MIGRATION_PARAMETER_TLS_HOSTNAME),
                        params->tls_hostname->u.s);
         assert(params->tls_authz);
-        monitor_printf(mon, "%s: '%s'\n",
+        monitor_printf(mon, "%s: %s\n",
             MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
                        params->tls_authz->u.s);
         assert(params->has_max_bandwidth);
-        monitor_printf(mon, "%s: %" PRIu64 " bytes/second\n",
+        monitor_printf(mon, "%s: %" PRIu64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_MAX_BANDWIDTH),
             params->max_bandwidth);
         assert(params->has_avail_switchover_bandwidth);
-        monitor_printf(mon, "%s: %" PRIu64 " bytes/second\n",
+        monitor_printf(mon, "%s: %" PRIu64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_AVAIL_SWITCHOVER_BANDWIDTH),
             params->avail_switchover_bandwidth);
         assert(params->has_max_postcopy_bandwidth);
-        monitor_printf(mon, "%s: %" PRIu64 " bytes/second\n",
+        monitor_printf(mon, "%s: %" PRIu64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_MAX_POSTCOPY_BANDWIDTH),
             params->max_postcopy_bandwidth);
         assert(params->has_downtime_limit);
-        monitor_printf(mon, "%s: %" PRIu64 " ms\n",
+        monitor_printf(mon, "%s: %" PRIu64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_DOWNTIME_LIMIT),
             params->downtime_limit);
         assert(params->has_x_checkpoint_delay);
-        monitor_printf(mon, "%s: %u ms\n",
+        monitor_printf(mon, "%s: %u\n",
             MigrationParameter_str(MIGRATION_PARAMETER_X_CHECKPOINT_DELAY),
             params->x_checkpoint_delay);
         monitor_printf(mon, "%s: %u\n",
@@ -411,41 +411,51 @@  void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
             MigrationParameter_str(MIGRATION_PARAMETER_ZERO_PAGE_DETECTION),
             qapi_enum_lookup(&ZeroPageDetection_lookup,
                 params->zero_page_detection));
-        monitor_printf(mon, "%s: %" PRIu64 " bytes\n",
+        monitor_printf(mon, "%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_printf(mon, "%s:\n",
+            monitor_printf(mon, "%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_printf(mon, " bitmaps:");
+                for (bal = na->bitmaps; bal; bal = bal->next) {
+                    ba = bal->value;
+                    bat = ba->transform;
 
-                monitor_printf(mon, "  '%s' -> '%s'\n",
-                               bmna->node_name, bmna->alias);
-
-                for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
-                    const BitmapMigrationBitmapAlias *bmba = bmbal->value;
-
-                    monitor_printf(mon, "    '%s' -> '%s'\n",
-                                   bmba->name, bmba->alias);
+                    monitor_printf(mon, " name: %s", ba->name);
+                    if (bat && bat->has_persistent) {
+                        if (bat->persistent) {
+                            monitor_printf(mon, " persistent: on");
+                        } else {
+                            monitor_printf(mon, " persistent: off");
+                        }
+                    }
+                    monitor_printf(mon, " alias: %s", ba->alias);
                 }
+                monitor_printf(mon, " node-name: %s alias: %s",
+                               na->node_name, na->alias);
             }
+
+            monitor_printf(mon, "\n");
         }
 
-        monitor_printf(mon, "%s: %" PRIu64 " ms\n",
+        monitor_printf(mon, "%s: %" PRIu64 "\n",
         MigrationParameter_str(MIGRATION_PARAMETER_X_VCPU_DIRTY_LIMIT_PERIOD),
         params->x_vcpu_dirty_limit_period);
 
-        monitor_printf(mon, "%s: %" PRIu64 " MB/s\n",
+        monitor_printf(mon, "%s: %" PRIu64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_VCPU_DIRTY_LIMIT),
             params->vcpu_dirty_limit);
 
@@ -462,7 +472,7 @@  void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
         }
 
         if (params->has_x_rdma_chunk_size) {
-            monitor_printf(mon, "%s: %" PRIu64 " bytes\n",
+            monitor_printf(mon, "%s: %" PRIu64 "\n",
                            MigrationParameter_str(
                                MIGRATION_PARAMETER_X_RDMA_CHUNK_SIZE),
                            params->x_rdma_chunk_size);
diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index e46616d7b19..03248f4474b 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 ba8183978b3..34b376562ff 100644
--- a/tests/qtest/migration/misc-tests.c
+++ b/tests/qtest/migration/misc-tests.c
@@ -55,21 +55,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", "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"),
+    TEST("announce-step", "15", "15"),
+    TEST("downtime-limit", "400", "400"),
+    TEST("avail-switchover-bandwidth", "2097152", "2199023255552"),
+    TEST("max-bandwidth", "9876543", "10356305952768"),
+    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"),
@@ -82,9 +82,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 */