| Message ID | 20260902221547.1812481-16-farosas@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | migration: MigrationParameters changes | expand |
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>
On Wed, Sep 02, 2026 at 07:15:43PM -0300, Fabiano Rosas wrote: > 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. I may have a slightly different feeling, but that takes a few things into account, (1) we don't have issue duplicating docs for the two parameter names anymore, (2) the dup is only about adding the same string once more in qapi/, (3) we now have this migrate_mark_all_params_present() function that must set all has_* fields, which the __MAX did help to guard a bit.. I think it's also fine to remove it completely, we just need to be more careful instead on migrate_mark_all_params_present() later. Which one is easier to be forgotten? I don't know.. Acked-by: Peter Xu <peterx@redhat.com> > > 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; > } > } > 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: > # > -- > 2.53.0 >
Peter Xu <peterx@redhat.com> writes: > On Wed, Sep 02, 2026 at 07:15:43PM -0300, Fabiano Rosas wrote: >> 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. > > I may have a slightly different feeling, but that takes a few things into > account, (1) we don't have issue duplicating docs for the two parameter > names anymore, (2) the dup is only about adding the same string once more > in qapi/, (3) we now have this migrate_mark_all_params_present() function > that must set all has_* fields, which the __MAX did help to guard a bit.. > I understand your points, I'm also unsure. I slightly prefer to have the "problem" inside migration/ than inside qapi/. Let me ponder a bit more during the respin, let's see. > I think it's also fine to remove it completely, we just need to be more > careful instead on migrate_mark_all_params_present() later. Which one is > easier to be forgotten? I don't know.. > > Acked-by: Peter Xu <peterx@redhat.com> > >> >> 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; >> } >> } >> 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: >> # >> -- >> 2.53.0 >>
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; } } 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: #
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(-)