Re: [PATCH 05/18] migration: Merge parameter structs instead of assigning one by one

[Peter Xu <peterx@redhat.com>]

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 <farosas@suse.de>

Nice,

Reviewed-by: Peter Xu <peterx@redhat.com>

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