Fabiano Rosas <[email protected]> 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 <[email protected]>
