Re: [PATCH 2/5] migration: Use QAPI_CLONE_MEMBERS in migrate_params_apply

[Fabiano Rosas <farosas@suse.de>]

Prasad Pandit <ppandit@redhat.com> writes:

> On Wed, 14 Jan 2026 at 18:55, Fabiano Rosas <farosas@suse.de> wrote:
>> Instead of setting parameters one by one, use the temporary object,
>> which already contains the current migration parameters plus the new
>> ones and was just validated by migration_params_check(). Use cloning
>> to overwrite it.
>>
>> This avoids the need to alter this function every time a new parameter
>> is added.
>>
>> Reviewed-by: Peter Xu <peterx@redhat.com>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/options.c | 134 ++++----------------------------------------
>>  1 file changed, 12 insertions(+), 122 deletions(-)
>>
>> diff --git a/migration/options.c b/migration/options.c
>> index 994e1cc5ac..7a16119ff8 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -13,6 +13,7 @@
>>
>>  #include "qemu/osdep.h"
>>  #include "qemu/error-report.h"
>> +#include "qemu/main-loop.h"
>>  #include "exec/target_page.h"
>>  #include "qapi/clone-visitor.h"
>>  #include "qapi/error.h"
>> @@ -1384,132 +1385,21 @@ static void migrate_params_test_apply(MigrationParameters *params,
>>      }
>>  }
>>
>> +/*
>> + * Caller must ensure all has_* fields of @params are true to ensure
>> + * all fields get copied and the pointer members don't dangle.
>> + */
>
> * Maybe skip initial -> Caller must ensure - to avoid double usage of  'ensure'
>      "All has_* fields of @params must be true to ensure that they are
> copied ..."
>

ack

>>  static void migrate_params_apply(MigrationParameters *params)
>>  {
>>      MigrationState *s = migrate_get_current();
>> +    MigrationParameters *cur = &s->parameters;
>>
>> -    /* TODO use QAPI_CLONE() instead of duplicating it inline */
>> +    assert(bql_locked());
>
> * Why are we confirming that bql_lock is taken? Is it because we are
> updating a global MigrationState field (s->parameters)? IIUC
> 'migrate_params_apply' is called via QMP_ call, which is handled by
> the main thread, we don't generally update/write these parameters in
> any other threads (multifd, postcopy etc.).  
>

In general, I think that whenever we determine what protects a data
structure from concurrent access we should make it obvious. For the BQL
specifically, it's a known issue that it's an overloaded lock and the
places that need it are poorly documented.

So this assert is here to provide _some_ assurance that this routine is
protected. I don't think it is, btw, because I don't see anything
proving that migration_is_running() & friends are not succeptible to
TOCTOU bugs.

> /* I'm thinking if it was
> not checked before, why do we need it now? We are including the
> main-loop.h header for this as well. */

What happens is that smart people write code they can prove is correct
in their head and later other smart people - not living inside the first
person's head - change the code and establish their own correctness
proof inside their own heads. =)

>>
>> -    if (params->has_throttle_trigger_threshold) {
>> -        s->parameters.throttle_trigger_threshold = params->throttle_trigger_threshold;
>> -    }
>> -
>> -    if (params->has_cpu_throttle_initial) {
>> -        s->parameters.cpu_throttle_initial = params->cpu_throttle_initial;
>> -    }
>> -
>> -    if (params->has_cpu_throttle_increment) {
>> -        s->parameters.cpu_throttle_increment = params->cpu_throttle_increment;
>> -    }
>> -
>> -    if (params->has_cpu_throttle_tailslow) {
>> -        s->parameters.cpu_throttle_tailslow = params->cpu_throttle_tailslow;
>> -    }
>> -
>> -    if (params->tls_creds) {
>> -        qapi_free_StrOrNull(s->parameters.tls_creds);
>> -        s->parameters.tls_creds = QAPI_CLONE(StrOrNull, params->tls_creds);
>> -    }
>> -
>> -    if (params->tls_hostname) {
>> -        qapi_free_StrOrNull(s->parameters.tls_hostname);
>> -        s->parameters.tls_hostname = QAPI_CLONE(StrOrNull,
>> -                                                params->tls_hostname);
>> -    }
>> -
>> -    if (params->tls_authz) {
>> -        qapi_free_StrOrNull(s->parameters.tls_authz);
>> -        s->parameters.tls_authz = QAPI_CLONE(StrOrNull, params->tls_authz);
>> -    }
>> -
>> -    if (params->has_max_bandwidth) {
>> -        s->parameters.max_bandwidth = params->max_bandwidth;
>> -    }
>> -
>> -    if (params->has_avail_switchover_bandwidth) {
>> -        s->parameters.avail_switchover_bandwidth = params->avail_switchover_bandwidth;
>> -    }
>> -
>> -    if (params->has_downtime_limit) {
>> -        s->parameters.downtime_limit = params->downtime_limit;
>> -    }
>> -
>> -    if (params->has_x_checkpoint_delay) {
>> -        s->parameters.x_checkpoint_delay = params->x_checkpoint_delay;
>> -    }
>> -
>> -    if (params->has_multifd_channels) {
>> -        s->parameters.multifd_channels = params->multifd_channels;
>> -    }
>> -    if (params->has_multifd_compression) {
>> -        s->parameters.multifd_compression = params->multifd_compression;
>> -    }
>> -    if (params->has_multifd_qatzip_level) {
>> -        s->parameters.multifd_qatzip_level = params->multifd_qatzip_level;
>> -    }
>> -    if (params->has_multifd_zlib_level) {
>> -        s->parameters.multifd_zlib_level = params->multifd_zlib_level;
>> -    }
>> -    if (params->has_multifd_zstd_level) {
>> -        s->parameters.multifd_zstd_level = params->multifd_zstd_level;
>> -    }
>> -    if (params->has_xbzrle_cache_size) {
>> -        s->parameters.xbzrle_cache_size = params->xbzrle_cache_size;
>> -    }
>> -    if (params->has_max_postcopy_bandwidth) {
>> -        s->parameters.max_postcopy_bandwidth = params->max_postcopy_bandwidth;
>> -    }
>> -    if (params->has_max_cpu_throttle) {
>> -        s->parameters.max_cpu_throttle = params->max_cpu_throttle;
>> -    }
>> -    if (params->has_announce_initial) {
>> -        s->parameters.announce_initial = params->announce_initial;
>> -    }
>> -    if (params->has_announce_max) {
>> -        s->parameters.announce_max = params->announce_max;
>> -    }
>> -    if (params->has_announce_rounds) {
>> -        s->parameters.announce_rounds = params->announce_rounds;
>> -    }
>> -    if (params->has_announce_step) {
>> -        s->parameters.announce_step = params->announce_step;
>> -    }
>> -
>> -    if (params->has_block_bitmap_mapping) {
>> -        qapi_free_BitmapMigrationNodeAliasList(
>> -            s->parameters.block_bitmap_mapping);
>> -
>> -        s->has_block_bitmap_mapping = true;
>> -        s->parameters.block_bitmap_mapping =
>> -            QAPI_CLONE(BitmapMigrationNodeAliasList,
>> -                       params->block_bitmap_mapping);
>> -    }
>> -
>> -    if (params->has_x_vcpu_dirty_limit_period) {
>> -        s->parameters.x_vcpu_dirty_limit_period =
>> -            params->x_vcpu_dirty_limit_period;
>> -    }
>> -    if (params->has_vcpu_dirty_limit) {
>> -        s->parameters.vcpu_dirty_limit = params->vcpu_dirty_limit;
>> -    }
>> -
>> -    if (params->has_mode) {
>> -        s->parameters.mode = params->mode;
>> -    }
>> -
>> -    if (params->has_zero_page_detection) {
>> -        s->parameters.zero_page_detection = params->zero_page_detection;
>> -    }
>> -
>> -    if (params->has_direct_io) {
>> -        s->parameters.direct_io = params->direct_io;
>> -    }
>> -
>> -    if (params->has_cpr_exec_command) {
>> -        qapi_free_strList(s->parameters.cpr_exec_command);
>> -        s->parameters.cpr_exec_command =
>> -            QAPI_CLONE(strList, params->cpr_exec_command);
>> -    }
>> +    migrate_tls_opts_free(cur);
>> +    qapi_free_BitmapMigrationNodeAliasList(cur->block_bitmap_mapping);
>> +    qapi_free_strList(cur->cpr_exec_command);
>> +    QAPI_CLONE_MEMBERS(MigrationParameters, cur, params);
>>  }
>>
>>  void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
>> @@ -1539,7 +1429,7 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
>>              migrate_get_current()->has_block_bitmap_mapping = true;
>>          }
>>
>> -        migrate_params_apply(params);
>> +        migrate_params_apply(&tmp);
>>          migrate_post_update_params(params, errp);
>>      }
>>
>> --
>
> * Change looks okay.
> Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
>
> Thank you.
> ---
>   - Prasad