On Wed, 14 Jan 2026 at 18:55, Fabiano Rosas <[email protected]> wrote:
> Convert the code in migrate_params_test_apply() from an open-coded
> copy of every migration parameter to a copy using visitors. The
> current code has conditionals for each parameter's has_* field, which
> is exactly what the visitors do.
>
> This hides the details of QAPI from the migration code and avoids the
> need to update migrate_params_test_apply() every time a new migration
> parameter is added. Both were very confusing and while the visitor
> code can become a bit involved, there is no need for new contributors
> to ever touch it.
>
> Move the QAPI_CLONE_MEMBERS into the caller, so QAPI_CLONE can be used
> and there's no need to allocate memory in the migration
> code. Similarly, turn 'tmp' into a pointer so the proper qapi_free_
> routine can be used.
>
> An extra call to migrate_mark_all_params_present() is now needed
> because the visitors update the has_ field for non-present fields, but
> we actually want them all set so migrate_params_apply() can copy all
> of them.
>
> Reviewed-by: Peter Xu <[email protected]>
> Signed-off-by: Fabiano Rosas <[email protected]>
> ---
> migration/options.c | 138 +++-----------------------------------------
> 1 file changed, 9 insertions(+), 129 deletions(-)
>
> diff --git a/migration/options.c b/migration/options.c
> index 7a16119ff8..b4773fecc5 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -20,6 +20,7 @@
> #include "qapi/qapi-commands-migration.h"
> #include "qapi/qapi-visit-migration.h"
> #include "qapi/qmp/qerror.h"
> +#include "qapi/type-helpers.h"
> #include "qobject/qnull.h"
> #include "system/runstate.h"
> #include "migration/colo.h"
> @@ -1262,129 +1263,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_cpr_exec_command) {
> - qapi_free_strList(dest->cpr_exec_command);
> - dest->cpr_exec_command = QAPI_CLONE(strList,
> params->cpr_exec_command);
> - }
> -}
> -
> /*
> * Caller must ensure all has_* fields of @params are true to ensure
> * all fields get copied and the pointer members don't dangle.
> @@ -1404,7 +1282,9 @@ static void migrate_params_apply(MigrationParameters
> *params)
>
> void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
> {
> - MigrationParameters tmp;
> + MigrationState *s = migrate_get_current();
> + g_autoptr(MigrationParameters) tmp = QAPI_CLONE(MigrationParameters,
> + &s->parameters);
>
> /*
> * Convert QTYPE_QNULL and NULL to the empty string (""). Even
> @@ -1418,9 +1298,9 @@ void qmp_migrate_set_parameters(MigrationParameters
> *params, Error **errp)
> tls_opt_to_str(params->tls_hostname);
> tls_opt_to_str(params->tls_authz);
>
> - migrate_params_test_apply(params, &tmp);
> + QAPI_MERGE(MigrationParameters, tmp, params);
>
> - if (migrate_params_check(&tmp, errp)) {
> + if (migrate_params_check(tmp, errp)) {
> /*
> * Mark block_bitmap_mapping as present now while we have the
> * params structure with the user input around.
> @@ -1429,9 +1309,9 @@ void qmp_migrate_set_parameters(MigrationParameters
> *params, Error **errp)
> migrate_get_current()->has_block_bitmap_mapping = true;
> }
>
> - migrate_params_apply(&tmp);
> + /* mark all present, so they're all copied */
> + migrate_mark_all_params_present(tmp);
> + migrate_params_apply(tmp);
> migrate_post_update_params(params, errp);
> }
> -
> - migrate_tls_opts_free(&tmp);
> }
> --
* IIUC, qmp_migrate_set_parameters receives @params from libvirtd(8)
or such external QMP_ caller; And the function sets those @params
values to the respective field in the current migration state
's->parameters', right? To do this:
1. It creates a temporary MigrationParameters object 'tmp', by
cloning the 's->parameters' object. @tmp = s->parameters.
2. it merges @params values into the 'tmp' object via
QAPI_MERGE(, tmp, params). @tmp = s->parameters + @params.
3. migrate_params_checks if values in @tmp are valid or not.
4. migrate_params_apply copies @tmp values into s->parameters.
<= do we need this migrate_params_apply() really? Couldn't we do
QAPI_COPY(s->parameters, tmp) OR QAPI_CLONE_MEMBERS(s->parameters,
tmp) here?
5. migrate_post_update_params() - checks few updated values and
calls respective functions <= we could remove this one too.
* Could we remove 'migrate_params_apply()' and
'migrate_post_update_params()' altogether and do the entire update
(clone -> update -> clone-back) operation and post-update actions in
one function (qmp_migrate_set_parameters), rather than scattered
across multiple functions?
Thank you.
---
- Prasad
