Fabiano Rosas <[email protected]> writes:
> This enum is convenient in two ways (just how enums work):
>
> 1- It provides the number of migration parameters as its __MAX member.
>
> 2- It allows iterating over an integer range and get a migration
> parameter name string corresponding to that position in the enum.
>
> The migration code doesn't have the need for (2) anymore.
>
> Balancing the benefit of (1) versus the disadvantage of requiring
> migration.json to be updated in two different places whenever a
> parameter is added, experience shows that the latter churn is enough
> to decide to remove the enum.
>
> Signed-off-by: Fabiano Rosas <[email protected]>
> ---
> migration/options.c | 6 +-----
> qapi/migration.json | 36 ------------------------------------
> 2 files changed, 1 insertion(+), 41 deletions(-)
>
> diff --git a/migration/options.c b/migration/options.c
> index 5d17acdd881..f988b181f0e 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -1127,7 +1127,6 @@ static MigrationParameters
> *migrate_params_from_dict(QDict *d, Error **errp)
> */
> static void migrate_mark_all_params_present(MigrationParameters *p)
> {
> - int len, n_str_args = 3; /* tls-creds, tls-hostname, tls-authz */
> bool *has_fields[] = {
> &p->has_throttle_trigger_threshold, &p->has_cpu_throttle_initial,
> &p->has_cpu_throttle_increment, &p->has_cpu_throttle_tailslow,
> @@ -1144,10 +1143,7 @@ static void
> migrate_mark_all_params_present(MigrationParameters *p)
> &p->has_x_rdma_chunk_size, &p->has_cpr_exec_command,
> };
>
> - len = ARRAY_SIZE(has_fields);
> - assert(len + n_str_args == MIGRATION_PARAMETER__MAX);
> -
> - for (int i = 0; i < len; i++) {
> + for (int i = 0; i < ARRAY_SIZE(has_fields); i++) {
> *has_fields[i] = true;
> }
> }
We lose the guard against forgetting to update has_fields[]. It's the
last user of MigrationParameter. I agree the guard doesn't justify
keeping MigrationParameter.
The guard is somewhat unclean anyway: it assumes the number of
MigrationParameter values matches the number of MigrationParameters
members.
Add a non-doc comment to MigrationParameters to remind of of updating
has_fields[] when it changes?
We don't have a convenient way to find the number of members.
> diff --git a/qapi/migration.json b/qapi/migration.json
> index b1eaf7b0545..78c6e933cf1 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -796,42 +796,6 @@
> 'bitmaps': [ 'BitmapMigrationBitmapAlias' ]
> } }
>
> -##
> -# @MigrationParameter:
> -#
> -# Migration parameters enumeration. The enumeration values mirror the
> -# members of @MigrationParameters.
> -#
> -# Features:
> -#
> -# @unstable: Members @x-checkpoint-delay, @x-rdma-chunk-size, and
> -# @x-vcpu-dirty-limit-period are experimental.
> -#
> -# Since: 2.4
> -##
> -{ 'enum': 'MigrationParameter',
> - 'data': ['announce-initial', 'announce-max',
> - 'announce-rounds', 'announce-step',
> - 'throttle-trigger-threshold',
> - 'cpu-throttle-initial', 'cpu-throttle-increment',
> - 'cpu-throttle-tailslow',
> - 'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth',
> - 'avail-switchover-bandwidth', 'downtime-limit',
> - { 'name': 'x-checkpoint-delay', 'features': [ 'unstable' ] },
> - 'multifd-channels',
> - 'xbzrle-cache-size', 'max-postcopy-bandwidth',
> - 'max-cpu-throttle', 'multifd-compression',
> - 'multifd-zlib-level', 'multifd-zstd-level',
> - 'multifd-qatzip-level',
> - 'block-bitmap-mapping',
> - { 'name': 'x-vcpu-dirty-limit-period', 'features': ['unstable'] },
> - 'vcpu-dirty-limit',
> - 'mode',
> - 'zero-page-detection',
> - 'direct-io',
> - { 'name': 'x-rdma-chunk-size', 'features': [ 'unstable' ] },
> - 'cpr-exec-command'] }
> -
> ##
> # @migrate-set-parameters:
> #
Reviewed-by: Markus Armbruster <[email protected]>
