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

[Markus Armbruster <armbru@redhat.com>]

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>