On Wed, Sep 02, 2026 at 07:15:33PM -0300, Fabiano Rosas wrote: > Convert the code in migrate_params_test_apply() from an open-coded > copy of every migration parameter to a merge operation using QAPI > visitors and QDict. > > The purpose of that routine is to update a temporary structure > (pre-populated with the current migration parameters), with the values > received from the user via QAPI. As a result, the temporary structure > will then contain the "to be applied" parameters and it's validated > before being used to overwrite the parameters currently in use. > > The update is currently done as follows: > > where 'params' is the user input from QAPI, > for each parameter: > > a) check if the option is present > params->has_<name> == true > params-><name> != NULL // for strings > > b) if the parameter is a pointer, free the to-be-assigned member and > allocate memory for the copy from params > > c) assign the user provided value to the temporary structure. > > Step (a) is the same in principle as what the QAPI visitors do at > visit_type_MigrationParameters_members(). > > Steps (b) and (c) are roughly the same as what the QDict > implementation does when qdict_del() and qdict_put_obj() are combined. > > Therefore, replace the open-coded function with > migrate_params_merge(), which achieves the same goal, but uses > visitors and QDict. This hides the details of QAPI (has_*) from the > migration code and avoids the need to update > migrate_params_test_apply() every time a new migration parameter is > added. > > Signed-off-by: Fabiano Rosas <[email protected]>
Nice, Reviewed-by: Peter Xu <[email protected]> Nitpicks only, inline, > --- > migration/options.c | 201 +++++++++++++++----------------------------- > 1 file changed, 66 insertions(+), 135 deletions(-) > > diff --git a/migration/options.c b/migration/options.c > index 6b787950808..de091d2eee3 100644 > --- a/migration/options.c > +++ b/migration/options.c > @@ -20,6 +20,9 @@ > #include "qapi/qapi-commands-migration.h" > #include "qapi/qapi-visit-migration.h" > #include "qapi/qmp/qerror.h" > +#include "qapi/qobject-input-visitor.h" > +#include "qapi/qobject-output-visitor.h" > +#include "qobject/qdict.h" > #include "qobject/qnull.h" > #include "system/runstate.h" > #include "migration/colo.h" > @@ -1074,6 +1077,28 @@ static void tls_opt_to_str(StrOrNull *opt) > opt->u.s = g_strdup(""); > } > > +static QDict *migrate_params_to_dict(MigrationParameters *p, Error **errp) > +{ > + QObject *obj = NULL; > + Visitor *v = qobject_output_visitor_new(&obj); > + > + if (visit_type_MigrationParameters(v, NULL, &p, errp)) { > + visit_complete(v, &obj); > + } > + visit_free(v); > + return qobject_to(QDict, obj); > +} > + > +static MigrationParameters *migrate_params_from_dict(QDict *d, Error **errp) > +{ > + Visitor *v = qobject_input_visitor_new(QOBJECT(d)); > + MigrationParameters *tmp = NULL; > + > + visit_type_MigrationParameters(v, NULL, &tmp, errp); > + visit_free(v); > + return tmp; > +} > + > /* > * query-migrate-parameters expects all members of MigrationParameters > * to be present, but we cannot mark them non-optional in QAPI because > @@ -1166,6 +1191,39 @@ static void > migrate_post_update_params(MigrationParameters *new, Error **errp) > } > } > > +static bool migrate_params_merge(MigrationParameters *in1, > + MigrationParameters *in2, Perhaps rename in1/in2/d1/d2 similarly with "cur" / "new" / ..? As in1 and in2 are not equal: when merge it only overwrites in1 with in2, not vice versa. > + MigrationParameters **out, > + Error **errp) > +{ > + g_autoptr(QDict) d1 = NULL; > + g_autoptr(QDict) d2 = NULL; > + const QDictEntry *e; > + > + d1 = migrate_params_to_dict(in1, errp); > + if (!d1) { > + return false; > + } > + > + d2 = migrate_params_to_dict(in2, errp); > + if (!d2) { > + return false; > + } > + > + for (e = qdict_first(d2); e; e = qdict_next(d2, e)) { > + const char *key = qdict_entry_key(e); > + QObject *value = qdict_entry_value(e); > + > + qdict_del(d1, key); IIUC this line can be dropped due to a smart enough qdict_put_obj(). > + qobject_ref(value); > + qdict_put_obj(d1, key, value); > + } > + > + *out = migrate_params_from_dict(d1, errp); > + > + return !!*out; > +} > + > /* > * Check whether the parameters are valid. Error will be put into errp > * (if provided). Return true if valid, otherwise false. > @@ -1328,133 +1386,6 @@ bool migrate_params_check(MigrationParameters > *params, Error **errp) > return true; > } > > -static void migrate_params_test_apply(MigrationParameters *params, > - MigrationParameters *dest) > -{ > - MigrationState *s = migrate_get_current(); > - > - QAPI_CLONE_MEMBERS(MigrationParameters, dest, &s->parameters); > - > - if (params->has_throttle_trigger_threshold) { > - dest->throttle_trigger_threshold = > params->throttle_trigger_threshold; > - } > - > - if (params->has_cpu_throttle_initial) { > - dest->cpu_throttle_initial = params->cpu_throttle_initial; > - } > - > - if (params->has_cpu_throttle_increment) { > - dest->cpu_throttle_increment = params->cpu_throttle_increment; > - } > - > - if (params->has_cpu_throttle_tailslow) { > - dest->cpu_throttle_tailslow = params->cpu_throttle_tailslow; > - } > - > - if (params->tls_creds) { > - qapi_free_StrOrNull(dest->tls_creds); > - dest->tls_creds = QAPI_CLONE(StrOrNull, params->tls_creds); > - } > - > - if (params->tls_hostname) { > - qapi_free_StrOrNull(dest->tls_hostname); > - dest->tls_hostname = QAPI_CLONE(StrOrNull, params->tls_hostname); > - } > - > - if (params->tls_authz) { > - qapi_free_StrOrNull(dest->tls_authz); > - dest->tls_authz = QAPI_CLONE(StrOrNull, params->tls_authz); > - } > - > - if (params->has_max_bandwidth) { > - dest->max_bandwidth = params->max_bandwidth; > - } > - > - if (params->has_avail_switchover_bandwidth) { > - dest->avail_switchover_bandwidth = > params->avail_switchover_bandwidth; > - } > - > - if (params->has_downtime_limit) { > - dest->downtime_limit = params->downtime_limit; > - } > - > - if (params->has_x_checkpoint_delay) { > - dest->x_checkpoint_delay = params->x_checkpoint_delay; > - } > - > - if (params->has_multifd_channels) { > - dest->multifd_channels = params->multifd_channels; > - } > - if (params->has_multifd_compression) { > - dest->multifd_compression = params->multifd_compression; > - } > - if (params->has_multifd_qatzip_level) { > - dest->multifd_qatzip_level = params->multifd_qatzip_level; > - } > - if (params->has_multifd_zlib_level) { > - dest->multifd_zlib_level = params->multifd_zlib_level; > - } > - if (params->has_multifd_zstd_level) { > - dest->multifd_zstd_level = params->multifd_zstd_level; > - } > - if (params->has_xbzrle_cache_size) { > - dest->xbzrle_cache_size = params->xbzrle_cache_size; > - } > - if (params->has_max_postcopy_bandwidth) { > - dest->max_postcopy_bandwidth = params->max_postcopy_bandwidth; > - } > - if (params->has_max_cpu_throttle) { > - dest->max_cpu_throttle = params->max_cpu_throttle; > - } > - if (params->has_announce_initial) { > - dest->announce_initial = params->announce_initial; > - } > - if (params->has_announce_max) { > - dest->announce_max = params->announce_max; > - } > - if (params->has_announce_rounds) { > - dest->announce_rounds = params->announce_rounds; > - } > - if (params->has_announce_step) { > - dest->announce_step = params->announce_step; > - } > - > - if (params->has_block_bitmap_mapping) { > - qapi_free_BitmapMigrationNodeAliasList(dest->block_bitmap_mapping); > - dest->block_bitmap_mapping = QAPI_CLONE(BitmapMigrationNodeAliasList, > - > params->block_bitmap_mapping); > - } > - > - if (params->has_x_vcpu_dirty_limit_period) { > - dest->x_vcpu_dirty_limit_period = > - params->x_vcpu_dirty_limit_period; > - } > - if (params->has_vcpu_dirty_limit) { > - dest->vcpu_dirty_limit = params->vcpu_dirty_limit; > - } > - > - if (params->has_mode) { > - dest->mode = params->mode; > - } > - > - if (params->has_zero_page_detection) { > - dest->zero_page_detection = params->zero_page_detection; > - } > - > - if (params->has_direct_io) { > - dest->direct_io = params->direct_io; > - } > - > - if (params->has_x_rdma_chunk_size) { > - dest->x_rdma_chunk_size = params->x_rdma_chunk_size; > - } > - > - if (params->has_cpr_exec_command) { > - qapi_free_strList(dest->cpr_exec_command); > - dest->cpr_exec_command = QAPI_CLONE(strList, > params->cpr_exec_command); > - } > -} > - > /* > * Caller must ensure the has_* fields of @params are true so they all > * get copied and the pointer members don't dangle. > @@ -1472,7 +1403,8 @@ static void migrate_params_apply(MigrationParameters > *params) > > void qmp_migrate_set_parameters(MigrationParameters *input, Error **errp) > { > - MigrationParameters new; > + MigrationParameters *cur = &migrate_get_current()->parameters; > + g_autoptr(MigrationParameters) new = NULL; > > /* > * Convert QTYPE_QNULL and NULL to the empty string (""). Even > @@ -1486,14 +1418,13 @@ void qmp_migrate_set_parameters(MigrationParameters > *input, Error **errp) > tls_opt_to_str(input->tls_hostname); > tls_opt_to_str(input->tls_authz); > > - migrate_params_test_apply(input, &new); > + /* merge input on top of current */ > + if (!migrate_params_merge(cur, input, &new, errp)) { > + return; > + } > > - if (migrate_params_check(&new, errp)) { > - migrate_params_apply(&new); > + if (migrate_params_check(new, errp)) { > + migrate_params_apply(new); > migrate_post_update_params(input, errp); > } > - > - migrate_tls_opts_free(&new); > - qapi_free_BitmapMigrationNodeAliasList(new.block_bitmap_mapping); > - qapi_free_strList(new.cpr_exec_command); > } > -- > 2.53.0 > -- Peter Xu
