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

[Fabiano Rosas <farosas@suse.de>]

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>