Re: [PATCH 15/18] qapi/migration: Remove MigrationParameter

[Markus Armbruster <armbru@redhat.com>]

Fabiano Rosas <farosas@suse.de> 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 <farosas@suse.de>
> ---
>  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 <armbru@redhat.com>