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.
+ */
 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());
 
-    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);
     }
 
-- 
2.51.0

On Wed, 14 Jan 2026 at 18:55, Fabiano Rosas <farosas@suse.de> wrote:
>  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);

* This change looks unrelated to the rest of this patch. I see it is
done in another [PATCH 5/5] ... QAPI_MERGE patch, we should move this
there.

Thank you.
---
  - Prasad

Prasad Pandit <ppandit@redhat.com> writes:

> On Wed, 14 Jan 2026 at 18:55, Fabiano Rosas <farosas@suse.de> wrote:
>>  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);
>
> * This change looks unrelated to the rest of this patch. I see it is
> done in another [PATCH 5/5] ... QAPI_MERGE patch, we should move this
> there.
>

This is the only reason we can use QAPI_CLONE_MEMBERS. The params
contain only the information that came from QAPI. What we really want to
apply is what's in tmp. I refer to it in the commit message:

"use the temporary object, which already contains the current migration
parameters plus the new ones and was just validated by
migration_params_check"

> Thank you.
> ---
>   - Prasad

On Thu, 22 Jan 2026 at 02:35, Fabiano Rosas <farosas@suse.de> wrote:
> > On Wed, 14 Jan 2026 at 18:55, Fabiano Rosas <farosas@suse.de> wrote:
> >>  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);
> >
> > * This change looks unrelated to the rest of this patch. I see it is
> > done in another [PATCH 5/5] ... QAPI_MERGE patch, we should move this
> > there.
> >
>
> This is the only reason we can use QAPI_CLONE_MEMBERS. The params
> contain only the information that came from QAPI. What we really want to
> apply is what's in tmp. I refer to it in the commit message:
>
> "use the temporary object, which already contains the current migration
> parameters plus the new ones and was just validated by
> migration_params_check"

* Yes, here we are changing from params to &tpm. And the other [PATCH
5/5] changes the same from

     void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
              ...
     -        migrate_params_apply(&tmp);
    +        migrate_params_apply(tmp);

along with other related changes. I was thinking we change
'migrate_params_apply' in [PATCH 2/5] and 'qmp_migrate_set_parameters'
in [PATCH 5/5].

Thank you.
---
  - Prasad

Prasad Pandit <ppandit@redhat.com> writes:

> On Thu, 22 Jan 2026 at 02:35, Fabiano Rosas <farosas@suse.de> wrote:
>> > On Wed, 14 Jan 2026 at 18:55, Fabiano Rosas <farosas@suse.de> wrote:
>> >>  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);
>> >
>> > * This change looks unrelated to the rest of this patch. I see it is
>> > done in another [PATCH 5/5] ... QAPI_MERGE patch, we should move this
>> > there.
>> >
>>
>> This is the only reason we can use QAPI_CLONE_MEMBERS. The params
>> contain only the information that came from QAPI. What we really want to
>> apply is what's in tmp. I refer to it in the commit message:
>>
>> "use the temporary object, which already contains the current migration
>> parameters plus the new ones and was just validated by
>> migration_params_check"
>
> * Yes, here we are changing from params to &tpm. And the other [PATCH
> 5/5] changes the same from
>
>      void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
>               ...
>      -        migrate_params_apply(&tmp);
>     +        migrate_params_apply(tmp);
>
> along with other related changes. I was thinking we change
> 'migrate_params_apply' in [PATCH 2/5] and 'qmp_migrate_set_parameters'
> in [PATCH 5/5].
>

No, because then this patch breaks entirely. This patch can only exist
if tmp is being used. Or maybe I'm not following...

> Thank you.
> ---
>   - Prasad

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 ..."

>  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.).  /* 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. */

>
> -    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

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

On Wed, 21 Jan 2026 at 18:12, Fabiano Rosas <farosas@suse.de> wrote:
> In general, I think that whenever we determine what protects a data
> structure from concurrent access we should make it obvious.
>
> So this assert is here to provide _some_ assurance that this routine is protected.

* Yes, but it is not obvious that we are checking bql_locked() to
protect from concurrent access to the global MigrationState object.
Secondly for concurrent access, other threads should be running.
===
Thread 1 "qemu-system-x86" hit Breakpoint 1, migrate_params_apply
(params=0x7ffcb6627840) at ../migration/options.c:1392
1392    {
(gdb) n
1393        MigrationState *s = migrate_get_current();
(gdb) p bql_locked()
$3 = true
(gdb) p migration_is_running()
$4 = false
(gdb)

Thread 1 "qemu-system-x86" hit Breakpoint 1, migrate_params_apply
(params=0x7ffcb6627840) at ../migration/options.c:1392
1392    {
(gdb) n
1393        MigrationState *s = migrate_get_current();
(gdb) p bql_locked()
$5 = true
(gdb) p migration_is_running()
$6 = false
(gdb)
===
* migrate_params_apply() is called twice:
    1. At the beginning when libvirtd(8) initiates migration
    2. To reset migration options if we kill migration on the
destination side by killing VM or connection.
Both times it is called from the main Thread-1, BQL is locked and
other migration threads are not running.

* If we are trying to futureproof this function so that no one should
be able to call migrate_params_apply() from other threads without bql
- that seems like a long shot. Because it is reachable only from
qmp_migrate_set_parameters(), which is invoked by an external QMP
user. Ie. someone would have to explicitly send
qmp_migrate_set_parameters() command while migration is running, but
even then it'll still be invoked by the main thread-1, with
bql_locked=true. no?

* I'm thinking maybe we can skip assert(bql_locked()) and related
header inclusion.

Thank you.
---
  - Prasad

Prasad Pandit <ppandit@redhat.com> writes:

> On Wed, 21 Jan 2026 at 18:12, Fabiano Rosas <farosas@suse.de> wrote:
>> In general, I think that whenever we determine what protects a data
>> structure from concurrent access we should make it obvious.
>>
>> So this assert is here to provide _some_ assurance that this routine is protected.
>
> * Yes, but it is not obvious that we are checking bql_locked() to
> protect from concurrent access to the global MigrationState object.
> Secondly for concurrent access, other threads should be running.
> ===
> Thread 1 "qemu-system-x86" hit Breakpoint 1, migrate_params_apply
> (params=0x7ffcb6627840) at ../migration/options.c:1392
> 1392    {
> (gdb) n
> 1393        MigrationState *s = migrate_get_current();
> (gdb) p bql_locked()
> $3 = true
> (gdb) p migration_is_running()
> $4 = false
> (gdb)
>
> Thread 1 "qemu-system-x86" hit Breakpoint 1, migrate_params_apply
> (params=0x7ffcb6627840) at ../migration/options.c:1392
> 1392    {
> (gdb) n
> 1393        MigrationState *s = migrate_get_current();
> (gdb) p bql_locked()
> $5 = true
> (gdb) p migration_is_running()
> $6 = false
> (gdb)
> ===
> * migrate_params_apply() is called twice:
>     1. At the beginning when libvirtd(8) initiates migration
>     2. To reset migration options if we kill migration on the
> destination side by killing VM or connection.

This is one usage. People are free to invoke QMP commands whenever they want.

> Both times it is called from the main Thread-1, BQL is locked and
> other migration threads are not running.
>
> * If we are trying to futureproof this function so that no one should
> be able to call migrate_params_apply() from other threads without bql
> - that seems like a long shot. Because it is reachable only from
> qmp_migrate_set_parameters(), which is invoked by an external QMP
> user. Ie. someone would have to explicitly send
> qmp_migrate_set_parameters() command while migration is running, but
> even then it'll still be invoked by the main thread-1, with
> bql_locked=true. no?
>

Setting parameters while running is common, our tests do it all the time
to adjust convergence options.

Where the command is invoked from is a bad assumption to make. There's
coroutines, iocontexts, oob, etc. These things change all the time and
we don't see it. Look at all the work done in the block layer to assert 

In this case, it will be invoked from the main loop and with BQL taken,
yes. That's not code under the migration subsystem's control, we should
not rely on that never changing. If we can check the requirements at the
entry point into the subsystem, we'll be better protected against
changes in other parts of QEMU that we didn't oversee.

Peter is now going through the painful exercise of removing the incoming
coroutine and playing the whack-a-mole of "BQL or no BQL?". Locks should
be fine-grained, protect specific bits of data and be heavily
documented. Analysing where the function is currently invoked from and
inferring about thread-safety is a pitfall.

(note that a simple /*called with BQL taken*/ would already count)

> * I'm thinking maybe we can skip assert(bql_locked()) and related
> header inclusion.
>

Fine, on the basis that it's out of scope for the patch anyway.

> Thank you.
> ---
>   - Prasad