Peter Xu <[email protected]> writes:
> 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.
>
Hm, let me google around, there might be some terms that fit here. Like
'base' and ... something else.
>> + 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().
>
There is a very obvious reason to keep it. I just don't remember what it
is. I'll check.
>> + 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
>>
