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.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/options.c | 123 +++-----------------------------------------
 1 file changed, 7 insertions(+), 116 deletions(-)

diff --git a/migration/options.c b/migration/options.c
index 4564db77f2..6619b5f21a 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"
@@ -1341,123 +1342,13 @@ static void migrate_params_test_apply(MigrationParameters *params,
 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;
-    }
+    migrate_tls_opts_free(cur);
+    qapi_free_BitmapMigrationNodeAliasList(cur->block_bitmap_mapping);
+    QAPI_CLONE_MEMBERS(MigrationParameters, cur, params);
 }
 
 void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
@@ -1487,7 +1378,7 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
     }
 
     if (migrate_params_check(&tmp, errp)) {
-        migrate_params_apply(params);
+        migrate_params_apply(&tmp);
         migrate_post_update_params(params, errp);
     }
 
-- 
2.35.3

On Mon, Jun 30, 2025 at 04:59:02PM -0300, Fabiano Rosas 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.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/options.c | 123 +++-----------------------------------------
>  1 file changed, 7 insertions(+), 116 deletions(-)
> 
> diff --git a/migration/options.c b/migration/options.c
> index 4564db77f2..6619b5f21a 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"
> @@ -1341,123 +1342,13 @@ static void migrate_params_test_apply(MigrationParameters *params,
>  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;
> -    }
> +    migrate_tls_opts_free(cur);
> +    qapi_free_BitmapMigrationNodeAliasList(cur->block_bitmap_mapping);

So we free these without resetting the pointers.  Now, for example,
cur->tls_creds can point to garbage.  Then..

> +    QAPI_CLONE_MEMBERS(MigrationParameters, cur, params);

How does this patch guarantee cur->tls_creds's garbage pointer being
updated?  What if params->tls_creds is NULL?  Could it?

>  }
>  
>  void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
> @@ -1487,7 +1378,7 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
>      }
>  
>      if (migrate_params_check(&tmp, errp)) {
> -        migrate_params_apply(params);
> +        migrate_params_apply(&tmp);
>          migrate_post_update_params(params, errp);
>      }
>  
> -- 
> 2.35.3
> 

-- 
Peter Xu

Peter Xu <peterx@redhat.com> writes:

> On Mon, Jun 30, 2025 at 04:59:02PM -0300, Fabiano Rosas 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.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/options.c | 123 +++-----------------------------------------
>>  1 file changed, 7 insertions(+), 116 deletions(-)
>> 
>> diff --git a/migration/options.c b/migration/options.c
>> index 4564db77f2..6619b5f21a 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"
>> @@ -1341,123 +1342,13 @@ static void migrate_params_test_apply(MigrationParameters *params,
>>  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;
>> -    }
>> +    migrate_tls_opts_free(cur);
>> +    qapi_free_BitmapMigrationNodeAliasList(cur->block_bitmap_mapping);
>
> So we free these without resetting the pointers.  Now, for example,
> cur->tls_creds can point to garbage.  Then..
>
>> +    QAPI_CLONE_MEMBERS(MigrationParameters, cur, params);
>
> How does this patch guarantee cur->tls_creds's garbage pointer being
> updated?  What if params->tls_creds is NULL?  Could it?
>

As you've spotted later in the series, all callers of
migrate_params_apply() provide a MigrationParameters struct with all
has_* fields set.

>>  }
>>  
>>  void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
>> @@ -1487,7 +1378,7 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
>>      }
>>  
>>      if (migrate_params_check(&tmp, errp)) {
>> -        migrate_params_apply(params);
>> +        migrate_params_apply(&tmp);
>>          migrate_post_update_params(params, errp);
>>      }
>>  
>> -- 
>> 2.35.3
>>