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

[Markus Armbruster <armbru@redhat.com>]

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>