[PATCH 0/5] qapi: Use visitors for migration parameters handling

[PATCH 0/5] qapi: Use visitors for migration parameters handling

[Fabiano Rosas]

I split this series from the original[0] because these changes are
more QAPI-related and self-contained.

The idea is to use QAPI_CLONE_MEMBERS and (the new) QAPI_MERGE to
reduce the amount of open-coded parameters handling in options.c, such
as checks to QAPI's has_* fields and manual inclusion of future
parameters.

- Patches 1 & 2 are unchanged

- Patch 3 adds a new variant of the dealloc visitor to free the to-be
  unreferenced memory when the input visitor eventually allocates new
  memory and overwrites the pointers.

- Patches 4 & 5 are mostly unchanged, but now use the new visitor.

0 - migration: Unify capabilities and parameters
https://lore.kernel.org/r/20251215220041.12657-1-farosas@suse.de

CI run: https://gitlab.com/farosas/qemu/-/pipelines/2262453042

Fabiano Rosas (5):
  migration: Use QAPI_CLONE_MEMBERS in migrate_params_test_apply
  migration: Use QAPI_CLONE_MEMBERS in migrate_params_apply
  qapi: Implement qapi_dealloc_present_visitor
  qapi: Add QAPI_MERGE
  migration/options: Use QAPI_MERGE in migrate_params_test_apply

 include/qapi/dealloc-visitor.h |   6 +
 include/qapi/type-helpers.h    |  40 +++++
 migration/options.c            | 282 ++++-----------------------------
 qapi/qapi-dealloc-visitor.c    | 173 +++++++++++++++++++-
 4 files changed, 246 insertions(+), 255 deletions(-)

-- 
2.51.0

[PATCH 1/5] migration: Use QAPI_CLONE_MEMBERS in migrate_params_test_apply

[Fabiano Rosas]

Use QAPI_CLONE_MEMBERS instead of making an assignment. The QAPI
method makes the handling of the TLS strings more intuitive because it
clones them as well.

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

diff --git a/migration/options.c b/migration/options.c
index 9a5a39c886..994e1cc5ac 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -1264,9 +1264,9 @@ bool migrate_params_check(MigrationParameters *params, Error **errp)
 static void migrate_params_test_apply(MigrationParameters *params,
                                       MigrationParameters *dest)
 {
-    *dest = migrate_get_current()->parameters;
+    MigrationState *s = migrate_get_current();
 
-    /* TODO use QAPI_CLONE() instead of duplicating it inline */
+    QAPI_CLONE_MEMBERS(MigrationParameters, dest, &s->parameters);
 
     if (params->has_throttle_trigger_threshold) {
         dest->throttle_trigger_threshold = params->throttle_trigger_threshold;
@@ -1285,24 +1285,18 @@ static void migrate_params_test_apply(MigrationParameters *params,
     }
 
     if (params->tls_creds) {
+        qapi_free_StrOrNull(dest->tls_creds);
         dest->tls_creds = QAPI_CLONE(StrOrNull, params->tls_creds);
-    } else {
-        /* clear the reference, it's owned by s->parameters */
-        dest->tls_creds = NULL;
     }
 
     if (params->tls_hostname) {
+        qapi_free_StrOrNull(dest->tls_hostname);
         dest->tls_hostname = QAPI_CLONE(StrOrNull, params->tls_hostname);
-    } else {
-        /* clear the reference, it's owned by s->parameters */
-        dest->tls_hostname = NULL;
     }
 
     if (params->tls_authz) {
+        qapi_free_StrOrNull(dest->tls_authz);
         dest->tls_authz = QAPI_CLONE(StrOrNull, params->tls_authz);
-    } else {
-        /* clear the reference, it's owned by s->parameters */
-        dest->tls_authz = NULL;
     }
 
     if (params->has_max_bandwidth) {
@@ -1359,8 +1353,9 @@ static void migrate_params_test_apply(MigrationParameters *params,
     }
 
     if (params->has_block_bitmap_mapping) {
-        dest->has_block_bitmap_mapping = true;
-        dest->block_bitmap_mapping = params->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) {
@@ -1384,7 +1379,8 @@ static void migrate_params_test_apply(MigrationParameters *params,
     }
 
     if (params->has_cpr_exec_command) {
-        dest->cpr_exec_command = params->cpr_exec_command;
+        qapi_free_strList(dest->cpr_exec_command);
+        dest->cpr_exec_command = QAPI_CLONE(strList, params->cpr_exec_command);
     }
 }
 
@@ -1535,6 +1531,14 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
     migrate_params_test_apply(params, &tmp);
 
     if (migrate_params_check(&tmp, errp)) {
+        /*
+         * Mark block_bitmap_mapping as present now while we have the
+         * params structure with the user input around.
+         */
+        if (params->has_block_bitmap_mapping) {
+            migrate_get_current()->has_block_bitmap_mapping = true;
+        }
+
         migrate_params_apply(params);
         migrate_post_update_params(params, errp);
     }
-- 
2.51.0

Re: [PATCH 1/5] migration: Use QAPI_CLONE_MEMBERS in migrate_params_test_apply

[Peter Xu]

On Wed, Jan 14, 2026 at 10:23:05AM -0300, Fabiano Rosas wrote:
> Use QAPI_CLONE_MEMBERS instead of making an assignment. The QAPI
> method makes the handling of the TLS strings more intuitive because it
> clones them as well.

The cover letter said this patch didn't change, but it has changed at least
somewhere.. anyway, I'm re-reviewing every line here.

> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/options.c | 32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/migration/options.c b/migration/options.c
> index 9a5a39c886..994e1cc5ac 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -1264,9 +1264,9 @@ bool migrate_params_check(MigrationParameters *params, Error **errp)
>  static void migrate_params_test_apply(MigrationParameters *params,
>                                        MigrationParameters *dest)
>  {
> -    *dest = migrate_get_current()->parameters;
> +    MigrationState *s = migrate_get_current();
>  
> -    /* TODO use QAPI_CLONE() instead of duplicating it inline */
> +    QAPI_CLONE_MEMBERS(MigrationParameters, dest, &s->parameters);
>  
>      if (params->has_throttle_trigger_threshold) {
>          dest->throttle_trigger_threshold = params->throttle_trigger_threshold;
> @@ -1285,24 +1285,18 @@ static void migrate_params_test_apply(MigrationParameters *params,
>      }
>  
>      if (params->tls_creds) {
> +        qapi_free_StrOrNull(dest->tls_creds);
>          dest->tls_creds = QAPI_CLONE(StrOrNull, params->tls_creds);
> -    } else {
> -        /* clear the reference, it's owned by s->parameters */
> -        dest->tls_creds = NULL;
>      }
>  
>      if (params->tls_hostname) {
> +        qapi_free_StrOrNull(dest->tls_hostname);
>          dest->tls_hostname = QAPI_CLONE(StrOrNull, params->tls_hostname);
> -    } else {
> -        /* clear the reference, it's owned by s->parameters */
> -        dest->tls_hostname = NULL;
>      }
>  
>      if (params->tls_authz) {
> +        qapi_free_StrOrNull(dest->tls_authz);
>          dest->tls_authz = QAPI_CLONE(StrOrNull, params->tls_authz);
> -    } else {
> -        /* clear the reference, it's owned by s->parameters */
> -        dest->tls_authz = NULL;
>      }
>  
>      if (params->has_max_bandwidth) {
> @@ -1359,8 +1353,9 @@ static void migrate_params_test_apply(MigrationParameters *params,
>      }
>  
>      if (params->has_block_bitmap_mapping) {
> -        dest->has_block_bitmap_mapping = true;
> -        dest->block_bitmap_mapping = params->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) {
> @@ -1384,7 +1379,8 @@ static void migrate_params_test_apply(MigrationParameters *params,
>      }
>  
>      if (params->has_cpr_exec_command) {
> -        dest->cpr_exec_command = params->cpr_exec_command;
> +        qapi_free_strList(dest->cpr_exec_command);
> +        dest->cpr_exec_command = QAPI_CLONE(strList, params->cpr_exec_command);
>      }
>  }

So we have 5 special cases here, (1-3) for tls, (4) for block bitmap, (5)
for cpr-exec-cmd.  All good.

>  
> @@ -1535,6 +1531,14 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
>      migrate_params_test_apply(params, &tmp);
>  
>      if (migrate_params_check(&tmp, errp)) {
> +        /*
> +         * Mark block_bitmap_mapping as present now while we have the
> +         * params structure with the user input around.
> +         */
> +        if (params->has_block_bitmap_mapping) {
> +            migrate_get_current()->has_block_bitmap_mapping = true;
> +        }

Now I'm looking at the lastest master branch, we have:

migrate_params_apply():
    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);
    }

Do we really need above change?

> +
>          migrate_params_apply(params);
>          migrate_post_update_params(params, errp);
>      }

The other thing is, when reaching here, after we have all 5 special cases
dynamically allocated, do we need to always free it?

We used to do it for the initial (1-3) for tls (in migrate_tls_opts_free()).
I think we should also free (4,5) now from &tmp?

> -- 
> 2.51.0
> 

-- 
Peter Xu

Re: [PATCH 1/5] migration: Use QAPI_CLONE_MEMBERS in migrate_params_test_apply

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Jan 14, 2026 at 10:23:05AM -0300, Fabiano Rosas wrote:
>> Use QAPI_CLONE_MEMBERS instead of making an assignment. The QAPI
>> method makes the handling of the TLS strings more intuitive because it
>> clones them as well.
>
> The cover letter said this patch didn't change, but it has changed at least
> somewhere.. anyway, I'm re-reviewing every line here.
>

Sorry, I forgot I had already addressed your review comments from the
other series in this patch.

>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/options.c | 32 ++++++++++++++++++--------------
>>  1 file changed, 18 insertions(+), 14 deletions(-)
>> 
>> diff --git a/migration/options.c b/migration/options.c
>> index 9a5a39c886..994e1cc5ac 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -1264,9 +1264,9 @@ bool migrate_params_check(MigrationParameters *params, Error **errp)
>>  static void migrate_params_test_apply(MigrationParameters *params,
>>                                        MigrationParameters *dest)
>>  {
>> -    *dest = migrate_get_current()->parameters;
>> +    MigrationState *s = migrate_get_current();
>>  
>> -    /* TODO use QAPI_CLONE() instead of duplicating it inline */
>> +    QAPI_CLONE_MEMBERS(MigrationParameters, dest, &s->parameters);
>>  
>>      if (params->has_throttle_trigger_threshold) {
>>          dest->throttle_trigger_threshold = params->throttle_trigger_threshold;
>> @@ -1285,24 +1285,18 @@ static void migrate_params_test_apply(MigrationParameters *params,
>>      }
>>  
>>      if (params->tls_creds) {
>> +        qapi_free_StrOrNull(dest->tls_creds);
>>          dest->tls_creds = QAPI_CLONE(StrOrNull, params->tls_creds);
>> -    } else {
>> -        /* clear the reference, it's owned by s->parameters */
>> -        dest->tls_creds = NULL;
>>      }
>>  
>>      if (params->tls_hostname) {
>> +        qapi_free_StrOrNull(dest->tls_hostname);
>>          dest->tls_hostname = QAPI_CLONE(StrOrNull, params->tls_hostname);
>> -    } else {
>> -        /* clear the reference, it's owned by s->parameters */
>> -        dest->tls_hostname = NULL;
>>      }
>>  
>>      if (params->tls_authz) {
>> +        qapi_free_StrOrNull(dest->tls_authz);
>>          dest->tls_authz = QAPI_CLONE(StrOrNull, params->tls_authz);
>> -    } else {
>> -        /* clear the reference, it's owned by s->parameters */
>> -        dest->tls_authz = NULL;
>>      }
>>  
>>      if (params->has_max_bandwidth) {
>> @@ -1359,8 +1353,9 @@ static void migrate_params_test_apply(MigrationParameters *params,
>>      }
>>  
>>      if (params->has_block_bitmap_mapping) {
>> -        dest->has_block_bitmap_mapping = true;
>> -        dest->block_bitmap_mapping = params->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) {
>> @@ -1384,7 +1379,8 @@ static void migrate_params_test_apply(MigrationParameters *params,
>>      }
>>  
>>      if (params->has_cpr_exec_command) {
>> -        dest->cpr_exec_command = params->cpr_exec_command;
>> +        qapi_free_strList(dest->cpr_exec_command);
>> +        dest->cpr_exec_command = QAPI_CLONE(strList, params->cpr_exec_command);
>>      }
>>  }
>
> So we have 5 special cases here, (1-3) for tls, (4) for block bitmap, (5)
> for cpr-exec-cmd.  All good.
>
>>  
>> @@ -1535,6 +1531,14 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
>>      migrate_params_test_apply(params, &tmp);
>>  
>>      if (migrate_params_check(&tmp, errp)) {
>> +        /*
>> +         * Mark block_bitmap_mapping as present now while we have the
>> +         * params structure with the user input around.
>> +         */
>> +        if (params->has_block_bitmap_mapping) {
>> +            migrate_get_current()->has_block_bitmap_mapping = true;
>> +        }
>
> Now I'm looking at the lastest master branch, we have:
>
> migrate_params_apply():
>     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);
>     }
>
> Do we really need above change?
>

It should be in the next patch.

>> +
>>          migrate_params_apply(params);
>>          migrate_post_update_params(params, errp);
>>      }
>
> The other thing is, when reaching here, after we have all 5 special cases
> dynamically allocated, do we need to always free it?
>
> We used to do it for the initial (1-3) for tls (in migrate_tls_opts_free()).
> I think we should also free (4,5) now from &tmp?
>

Yes!

>> -- 
>> 2.51.0
>> 

Re: [PATCH 1/5] migration: Use QAPI_CLONE_MEMBERS in migrate_params_test_apply

[Fabiano Rosas]

Fabiano Rosas <farosas@suse.de> writes:

> Peter Xu <peterx@redhat.com> writes:
>
>> On Wed, Jan 14, 2026 at 10:23:05AM -0300, Fabiano Rosas wrote:
>>> Use QAPI_CLONE_MEMBERS instead of making an assignment. The QAPI
>>> method makes the handling of the TLS strings more intuitive because it
>>> clones them as well.
>>
>> The cover letter said this patch didn't change, but it has changed at least
>> somewhere.. anyway, I'm re-reviewing every line here.
>>
>
> Sorry, I forgot I had already addressed your review comments from the
> other series in this patch.
>
>>> 
>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>> ---
>>>  migration/options.c | 32 ++++++++++++++++++--------------
>>>  1 file changed, 18 insertions(+), 14 deletions(-)
>>> 
>>> diff --git a/migration/options.c b/migration/options.c
>>> index 9a5a39c886..994e1cc5ac 100644
>>> --- a/migration/options.c
>>> +++ b/migration/options.c
>>> @@ -1264,9 +1264,9 @@ bool migrate_params_check(MigrationParameters *params, Error **errp)
>>>  static void migrate_params_test_apply(MigrationParameters *params,
>>>                                        MigrationParameters *dest)
>>>  {
>>> -    *dest = migrate_get_current()->parameters;
>>> +    MigrationState *s = migrate_get_current();
>>>  
>>> -    /* TODO use QAPI_CLONE() instead of duplicating it inline */
>>> +    QAPI_CLONE_MEMBERS(MigrationParameters, dest, &s->parameters);
>>>  
>>>      if (params->has_throttle_trigger_threshold) {
>>>          dest->throttle_trigger_threshold = params->throttle_trigger_threshold;
>>> @@ -1285,24 +1285,18 @@ static void migrate_params_test_apply(MigrationParameters *params,
>>>      }
>>>  
>>>      if (params->tls_creds) {
>>> +        qapi_free_StrOrNull(dest->tls_creds);
>>>          dest->tls_creds = QAPI_CLONE(StrOrNull, params->tls_creds);
>>> -    } else {
>>> -        /* clear the reference, it's owned by s->parameters */
>>> -        dest->tls_creds = NULL;
>>>      }
>>>  
>>>      if (params->tls_hostname) {
>>> +        qapi_free_StrOrNull(dest->tls_hostname);
>>>          dest->tls_hostname = QAPI_CLONE(StrOrNull, params->tls_hostname);
>>> -    } else {
>>> -        /* clear the reference, it's owned by s->parameters */
>>> -        dest->tls_hostname = NULL;
>>>      }
>>>  
>>>      if (params->tls_authz) {
>>> +        qapi_free_StrOrNull(dest->tls_authz);
>>>          dest->tls_authz = QAPI_CLONE(StrOrNull, params->tls_authz);
>>> -    } else {
>>> -        /* clear the reference, it's owned by s->parameters */
>>> -        dest->tls_authz = NULL;
>>>      }
>>>  
>>>      if (params->has_max_bandwidth) {
>>> @@ -1359,8 +1353,9 @@ static void migrate_params_test_apply(MigrationParameters *params,
>>>      }
>>>  
>>>      if (params->has_block_bitmap_mapping) {
>>> -        dest->has_block_bitmap_mapping = true;
>>> -        dest->block_bitmap_mapping = params->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) {
>>> @@ -1384,7 +1379,8 @@ static void migrate_params_test_apply(MigrationParameters *params,
>>>      }
>>>  
>>>      if (params->has_cpr_exec_command) {
>>> -        dest->cpr_exec_command = params->cpr_exec_command;
>>> +        qapi_free_strList(dest->cpr_exec_command);
>>> +        dest->cpr_exec_command = QAPI_CLONE(strList, params->cpr_exec_command);
>>>      }
>>>  }
>>
>> So we have 5 special cases here, (1-3) for tls, (4) for block bitmap, (5)
>> for cpr-exec-cmd.  All good.
>>
>>>  
>>> @@ -1535,6 +1531,14 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
>>>      migrate_params_test_apply(params, &tmp);
>>>  
>>>      if (migrate_params_check(&tmp, errp)) {
>>> +        /*
>>> +         * Mark block_bitmap_mapping as present now while we have the
>>> +         * params structure with the user input around.
>>> +         */
>>> +        if (params->has_block_bitmap_mapping) {
>>> +            migrate_get_current()->has_block_bitmap_mapping = true;
>>> +        }
>>
>> Now I'm looking at the lastest master branch, we have:
>>
>> migrate_params_apply():
>>     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);
>>     }
>>
>> Do we really need above change?
>>
>
> It should be in the next patch.
>
>>> +
>>>          migrate_params_apply(params);
>>>          migrate_post_update_params(params, errp);
>>>      }
>>
>> The other thing is, when reaching here, after we have all 5 special cases
>> dynamically allocated, do we need to always free it?
>>
>> We used to do it for the initial (1-3) for tls (in migrate_tls_opts_free()).
>> I think we should also free (4,5) now from &tmp?
>>
>
> Yes!
>

Although, for CPR, the ASAN, valgrind, etc tools will never catch the
leak because of the execvp(). It took me a while to figure that one.

>>> -- 
>>> 2.51.0
>>> 

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

[Fabiano Rosas]

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

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

[Prasad Pandit]

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

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

[Fabiano Rosas]

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

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

[Prasad Pandit]

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

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

[Fabiano Rosas]

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

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

[Prasad Pandit]

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

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

[Fabiano Rosas]

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

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

[Prasad Pandit]

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

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

[Fabiano Rosas]

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

[PATCH 3/5] qapi: Implement qapi_dealloc_present_visitor

[Fabiano Rosas]

Implement a visitor that frees the pointer members of the visited QAPI
object in the same way that qapi_dealloc_visitor does, but similarly
to qobject_input_visitor, takes an input QObject that will dictate
which members get freed and which don't. Members not present in the
input QObject will be left unchanged in the visited QAPI object.

This is useful to free memory just before perfoming a visit with
qobject_input_visitor on a pre-existing, non-null QAPI object. If the
same QObject is passed to both visitors, the pointers overwritten by
the input visitor match the ones that are freed by the dealloc
visitor.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 include/qapi/dealloc-visitor.h |   6 ++
 qapi/qapi-dealloc-visitor.c    | 173 ++++++++++++++++++++++++++++++++-
 2 files changed, 178 insertions(+), 1 deletion(-)

diff --git a/include/qapi/dealloc-visitor.h b/include/qapi/dealloc-visitor.h
index c36715fdf3..96c7bf35c3 100644
--- a/include/qapi/dealloc-visitor.h
+++ b/include/qapi/dealloc-visitor.h
@@ -25,4 +25,10 @@ typedef struct QapiDeallocVisitor QapiDeallocVisitor;
  */
 Visitor *qapi_dealloc_visitor_new(void);
 
+/*
+ * Like qapi_dealloc_visitor_new but visits a QObject and only frees
+ * present members.
+ */
+Visitor *qapi_dealloc_present_visitor_new(QObject *);
+
 #endif
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 57a2c904bb..90b017cc93 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -14,14 +14,146 @@
 
 #include "qemu/osdep.h"
 #include "qapi/dealloc-visitor.h"
+#include "qemu/queue.h"
+#include "qobject/qdict.h"
+#include "qobject/qlist.h"
 #include "qobject/qnull.h"
 #include "qapi/visitor-impl.h"
 
+typedef struct QStackEntry {
+    QObject *obj;               /* QDict or QList being visited */
+    void *qapi;
+    const QListEntry *entry;    /* If @obj is QList: unvisited tail */
+    QSLIST_ENTRY(QStackEntry) node;
+} QStackEntry;
+
 struct QapiDeallocVisitor
 {
     Visitor visitor;
+    QObject *root;
+    QSLIST_HEAD(, QStackEntry) stack;
 };
 
+static void qapi_dealloc_pop(Visitor *v, void **obj)
+{
+    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
+    QStackEntry *se = QSLIST_FIRST(&qdv->stack);
+
+    assert(se && se->qapi == obj);
+    QSLIST_REMOVE_HEAD(&qdv->stack, node);
+    g_free(se);
+}
+
+static void qapi_dealloc_push(Visitor *v, QObject *obj, void *qapi)
+{
+    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
+    QStackEntry *se = g_new0(QStackEntry, 1);
+
+    assert(obj);
+    se->obj = obj;
+    se->qapi = qapi;
+
+    if (qobject_type(obj) == QTYPE_QLIST) {
+        se->entry = qlist_first(qobject_to(QList, obj));
+    }
+
+    QSLIST_INSERT_HEAD(&qdv->stack, se, node);
+}
+
+static QObject *qapi_dealloc_try_get_object(QapiDeallocVisitor *qdv, const char *name)
+{
+    QStackEntry *se = QSLIST_FIRST(&qdv->stack);
+    QObject *qobj;
+    QObject *ret = NULL;
+
+    if (!se) {
+        assert(qdv->root);
+        return qdv->root;
+    }
+
+    qobj = se->obj;
+    assert(qobj);
+
+    if (qobject_type(qobj) == QTYPE_QDICT) {
+        assert(name);
+        ret = qdict_get(qobject_to(QDict, qobj), name);
+    } else {
+        assert(qobject_type(qobj) == QTYPE_QLIST);
+        assert(!name);
+        if (se->entry) {
+            ret = qlist_entry_obj(se->entry);
+        }
+    }
+
+    return ret;
+}
+
+static bool qapi_dealloc_present_start_struct(Visitor *v, const char *name,
+                                              void **obj, size_t size,
+                                              Error **errp)
+{
+    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
+    QObject *qobj = qapi_dealloc_try_get_object(qdv, name);
+
+    if (!qobj) {
+        return false;
+    }
+    assert(qobject_type(qobj) == QTYPE_QDICT);
+    qapi_dealloc_push(v, qobj, obj);
+    return true;
+}
+
+static void qapi_dealloc_present_end_struct(Visitor *v, void **obj)
+{
+    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
+    QStackEntry *se = QSLIST_FIRST(&qdv->stack);
+
+    assert(qobject_type(se->obj) == QTYPE_QDICT);
+    qapi_dealloc_pop(v, obj);
+
+    if (obj) {
+        g_free(*obj);
+    }
+}
+
+static bool qapi_dealloc_present_start_list(Visitor *v, const char *name,
+                                            GenericList **list, size_t size,
+                                            Error **errp)
+{
+    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
+    QObject *qobj = qapi_dealloc_try_get_object(qdv, name);
+
+    if (!qobj) {
+        return false;
+    }
+    assert(qobject_type(qobj) == QTYPE_QLIST);
+    qapi_dealloc_push(v, qobj, list);
+    return true;
+}
+
+static void qapi_dealloc_present_end_list(Visitor *v, void **obj)
+{
+    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
+    QStackEntry *se = QSLIST_FIRST(&qdv->stack);
+
+    assert(qobject_type(se->obj) == QTYPE_QLIST);
+    qapi_dealloc_pop(v, obj);
+}
+
+static void qapi_dealloc_present_free(Visitor *v)
+{
+    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
+
+    while (!QSLIST_EMPTY(&qdv->stack)) {
+        QStackEntry *se = QSLIST_FIRST(&qdv->stack);
+
+        QSLIST_REMOVE_HEAD(&qdv->stack, node);
+        g_free(se);
+    }
+    qobject_unref(qdv->root);
+    g_free(qdv);
+}
+
 static bool qapi_dealloc_start_struct(Visitor *v, const char *name, void **obj,
                                       size_t unused, Error **errp)
 {
@@ -35,6 +167,21 @@ static void qapi_dealloc_end_struct(Visitor *v, void **obj)
     }
 }
 
+static bool qapi_dealloc_start_alternate(Visitor *v, const char *name,
+                                         GenericAlternate **obj, size_t size,
+                                         Error **errp)
+{
+    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
+    QObject *qobj = qapi_dealloc_try_get_object(qdv, name);
+
+    if (!qobj) {
+        return false;
+    }
+    assert(*obj);
+    (*obj)->type = qobject_type(qobj);
+    return true;
+}
+
 static void qapi_dealloc_end_alternate(Visitor *v, void **obj)
 {
     if (obj) {
@@ -117,13 +264,14 @@ static void qapi_dealloc_free(Visitor *v)
     g_free(container_of(v, QapiDeallocVisitor, visitor));
 }
 
-Visitor *qapi_dealloc_visitor_new(void)
+static QapiDeallocVisitor *qapi_dealloc_visitor_new_base(void)
 {
     QapiDeallocVisitor *v;
 
     v = g_malloc0(sizeof(*v));
 
     v->visitor.type = VISITOR_DEALLOC;
+
     v->visitor.start_struct = qapi_dealloc_start_struct;
     v->visitor.end_struct = qapi_dealloc_end_struct;
     v->visitor.end_alternate = qapi_dealloc_end_alternate;
@@ -139,5 +287,28 @@ Visitor *qapi_dealloc_visitor_new(void)
     v->visitor.type_null = qapi_dealloc_type_null;
     v->visitor.free = qapi_dealloc_free;
 
+    return v;
+}
+
+Visitor *qapi_dealloc_visitor_new(void)
+{
+    QapiDeallocVisitor *v = qapi_dealloc_visitor_new_base();
+
+    return &v->visitor;
+}
+
+Visitor *qapi_dealloc_present_visitor_new(QObject *obj)
+{
+    QapiDeallocVisitor *v = qapi_dealloc_visitor_new_base();
+
+    v->visitor.start_alternate = qapi_dealloc_start_alternate;
+    v->visitor.start_list = qapi_dealloc_present_start_list;
+    v->visitor.end_list = qapi_dealloc_present_end_list;
+    v->visitor.start_struct = qapi_dealloc_present_start_struct;
+    v->visitor.end_struct = qapi_dealloc_present_end_struct;
+    v->visitor.free = qapi_dealloc_present_free;
+
+    v->root = qobject_ref(obj);
+
     return &v->visitor;
 }
-- 
2.51.0

[PATCH 4/5] qapi: Add QAPI_MERGE

[Fabiano Rosas]

The migration subsystem currently has code to merge two objects of the
same type. It does so by checking which fields are present in a source
object and overwriting the corresponding fields on the destination
object. This leads to a lot of open-coded lines such as:

    if (src->has_foobar) {
        dst->foobar = src->foobar;
    }

This pattern could be replaced by a copy using visitors. Implement a
macro that extracts elements from a source object using an output
visitor and merges it with a destination object using an input
visitor.

Acked-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 include/qapi/type-helpers.h | 40 +++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/include/qapi/type-helpers.h b/include/qapi/type-helpers.h
index fc8352cdec..f22d7b7139 100644
--- a/include/qapi/type-helpers.h
+++ b/include/qapi/type-helpers.h
@@ -10,6 +10,9 @@
  */
 
 #include "qapi/qapi-types-common.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qobject-output-visitor.h"
+#include "qapi/dealloc-visitor.h"
 
 HumanReadableText *human_readable_text_from_str(GString *str);
 
@@ -20,3 +23,40 @@ HumanReadableText *human_readable_text_from_str(GString *str);
  * cleanup.
  */
 char **strv_from_str_list(const strList *list);
+
+/*
+ * Merge @src over @dst by copying deep clones of the present members
+ * from @src to @dst. Non-present on @src are left untouched on
+ * @dst. Pointer members that are overwritten are also freed.
+ */
+#define QAPI_MERGE(type, dst_, src_)                                    \
+    ({                                                                  \
+        QObject *out_ = NULL;                                           \
+        Visitor *v_;                                                    \
+        /* read in from src */                                          \
+        v_ = qobject_output_visitor_new(&out_);                         \
+        visit_type_ ## type(v_, NULL, &src_, &error_abort);             \
+        visit_complete(v_, &out_);                                      \
+        visit_free(v_);                                                 \
+        /*                                                              \
+         * Free the memory for which the pointers will be overwritten   \
+         * in the next step.                                            \
+         */                                                             \
+        v_ = qapi_dealloc_present_visitor_new(out_);                    \
+        visit_start_struct(v_, NULL, NULL, 0, &error_abort);            \
+        visit_type_ ## type ## _members(v_, dst_, &error_abort);        \
+        visit_end_struct(v_, NULL);                                     \
+        visit_free(v_);                                                 \
+        /*                                                              \
+         * Write to dst but leave existing fields intact (except for    \
+         * has_* which will be updated according to their presence in   \
+         * src).                                                        \
+         */                                                             \
+        v_ = qobject_input_visitor_new(out_);                           \
+        visit_start_struct(v_, NULL, NULL, 0, &error_abort);            \
+        visit_type_ ## type ## _members(v_, dst_, &error_abort);        \
+        visit_check_struct(v_, &error_abort);                           \
+        visit_end_struct(v_, NULL);                                     \
+        visit_free(v_);                                                 \
+        qobject_unref(out_);                                            \
+    })
-- 
2.51.0

[PATCH 5/5] migration/options: Use QAPI_MERGE in migrate_params_test_apply

[Fabiano Rosas]

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 <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 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);
 }
-- 
2.51.0

Re: [PATCH 5/5] migration/options: Use QAPI_MERGE in migrate_params_test_apply

[Prasad Pandit]

On Wed, 14 Jan 2026 at 18:55, Fabiano Rosas <farosas@suse.de> 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 <peterx@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  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

Re: [PATCH 5/5] migration/options: Use QAPI_MERGE in migrate_params_test_apply

[Fabiano Rosas]

Prasad Pandit <ppandit@redhat.com> writes:

> On Wed, 14 Jan 2026 at 18:55, Fabiano Rosas <farosas@suse.de> 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 <peterx@redhat.com>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  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?

I'm not sure I follow, do you want to open-code migrate_params_apply? It
already does QAPI_CLONE_MEMBERS(s->parameters, tmp). We still need to
free the memory from the old parameters. It seems adequate to have it as
a single unit. I do have a patch that wraps the freeing in a function,
so maybe that improves the readability a bit.

>      5. migrate_post_update_params() - checks few updated values and
> calls respective functions   <= we could remove this one too.
>

I'd prefer not, It's explicitly done like so because this function
applies side-effects. I don't want options that can affect a running
migration to be hidden amid validation code. It also must read from the
new params, not from the resulting tmp.

One thing you made me think of is that the block_bitmap_mapping logic
could very well be in that function as well.

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

Functions bring the benefit of being able to document the code in the
function name. I think it's friendly to newcomers: "if (check) apply".

Having written a few migration parameters and having reviewed the
addition of others, I want a new contributor to be able to answer the
following questions themselves.

Q: Where do I validate my new migration parameter?
A: migrate_params_check.

Q: Where do I check my new parameters has been set?
A: After migrate_params_apply.

Q: What do I need to change to add a new parameter?
A: Choose one from the existing ones and grep for it.

Q: Where do the docs go?
A: @MigrationParameters in migration.json

See if the following looks better to you. It's not one single function,
but I folded what I could while keeping the distinct function names.

static void migrate_params_apply(MigrationParameters *new, Error **errp)
{
    MigrationState *s = migrate_get_current();
    MigrationParameters *cur = &s->parameters;
    MigrationParameters *tmp = QAPI_CLONE(MigrationParameters, cur);

    QAPI_MERGE(MigrationParameters, tmp, new);
    
    if (!migrate_params_check(tmp, errp)) {
        return;
    }

    migrate_ptr_opts_free(cur);

    /* mark all present, so they're all copied */
    migrate_mark_all_params_present(params);
    QAPI_CLONE_MEMBERS(MigrationParameters, cur, params);

    migrate_post_update_params(new, errp);
}

void qmp_migrate_set_parameters(MigrationParameters *new, Error **errp)
{
    /*
     * Convert QTYPE_QNULL and NULL to the empty string (""). Even
     * though NULL is cleaner to deal with in C code, that would force
     * query-migrate-parameters to convert it once more to the empty
     * string, so avoid that. The migrate_tls_*() helpers that expose
     * the options to the rest of the migration code already use
     * return NULL when the empty string is found.
     */
    tls_opt_to_str(new->tls_creds);
    tls_opt_to_str(new->tls_hostname);
    tls_opt_to_str(new->tls_authz);

    migrate_params_apply(new, errp);
}

Re: [PATCH 5/5] migration/options: Use QAPI_MERGE in migrate_params_test_apply

[Prasad Pandit]

Hi,

On Thu, 22 Jan 2026 at 03:41, Fabiano Rosas <farosas@suse.de> wrote:
> See if the following looks better to you. It's not one single function,
> but I folded what I could while keeping the distinct function names.
>
> static void migrate_params_apply(MigrationParameters *new, Error **errp)
> {
>     MigrationState *s = migrate_get_current();
>     MigrationParameters *cur = &s->parameters;
>     MigrationParameters *tmp = QAPI_CLONE(MigrationParameters, cur);
>
>     QAPI_MERGE(MigrationParameters, tmp, new);
>
>     if (!migrate_params_check(tmp, errp)) {
>         return;
>     }
>
>     migrate_ptr_opts_free(cur);
>
>     /* mark all present, so they're all copied */
>     migrate_mark_all_params_present(params);
>     QAPI_CLONE_MEMBERS(MigrationParameters, cur, params);
>
>     migrate_post_update_params(new, errp);
> }
>
> void qmp_migrate_set_parameters(MigrationParameters *new, Error **errp)
> {
>     /*
>      * Convert QTYPE_QNULL and NULL to the empty string (""). Even
>      * though NULL is cleaner to deal with in C code, that would force
>      * query-migrate-parameters to convert it once more to the empty
>      * string, so avoid that. The migrate_tls_*() helpers that expose
>      * the options to the rest of the migration code already use
>      * return NULL when the empty string is found.
>      */
>     tls_opt_to_str(new->tls_creds);
>     tls_opt_to_str(new->tls_hostname);
>     tls_opt_to_str(new->tls_authz);
>
>     migrate_params_apply(new, errp);
> }
>

* I'm thinking as:
===
v0:

void qmp_migrate_set_parameters(MigrationParameters *new, Error **errp)
{
    /*
     * Convert QTYPE_QNULL and NULL to the empty string (""). Even
     * though NULL is cleaner to deal with in C code, that would force
     * query-migrate-parameters to convert it once more to the empty
     * string, so avoid that. The migrate_tls_*() helpers that expose
     * the options to the rest of the migration code already use
     * return NULL when the empty string is found.
     */
    tls_opt_to_str(new->tls_creds);
    tls_opt_to_str(new->tls_hostname);
    tls_opt_to_str(new->tls_authz);

    MigrationState *s = migrate_get_current();
    MigrationParameters *cur = &s->parameters;
    MigrationParameters *tmp = QAPI_CLONE(MigrationParameters, cur);

    QAPI_MERGE(MigrationParameters, tmp, new);

    if (!migrate_params_check(tmp, errp)) {
        return;
    }
    migrate_ptr_opts_free(cur);

    /* mark all present, so they're all copied */
    migrate_mark_all_params_present(tpm);
    QAPI_CLONE_MEMBERS(MigrationParameters, cur, tmp);

    migrate_post_update_params(cur, errp);
}
===
v1:

void qmp_migrate_set_parameters(MigrationParameters *new, Error **errp)
{
    /*
     * Convert QTYPE_QNULL and NULL to the empty string (""). Even
     * though NULL is cleaner to deal with in C code, that would force
     * query-migrate-parameters to convert it once more to the empty
     * string, so avoid that. The migrate_tls_*() helpers that expose
     * the options to the rest of the migration code already use
     * return NULL when the empty string is found.
     */
    tls_opt_to_str(new->tls_creds);
    tls_opt_to_str(new->tls_hostname);
    tls_opt_to_str(new->tls_authz);

    MigrationState *s = migrate_get_current();
    MigrationParameters *cur = &s->parameters;
    MigrationParameters *tmp = QAPI_CLONE(MigrationParameters, cur);      ...[1]

    QAPI_MERGE(MigrationParameters, tmp, new);
                 ...[2]

    if (!migrate_params_check(tmp, errp)) {
        return;
    }
    migrate_ptr_opts_free(cur);

    /* mark all present, so they're all copied */
    migrate_mark_all_params_present(tpm);
    QAPI_CLONE_MEMBERS(MigrationParameters, cur, tmp);
       ...[3]

    /* update MigrationState after parameters change */
                     ...[4]
    if (cur->has_max_bandwidth) {
        if (s->to_dst_file && !migration_in_postcopy()) {
            migration_rate_set(cur->max_bandwidth);
        }
    }
    if (cur->has_x_checkpoint_delay) {
        colo_checkpoint_delay_set();
    }
    if (cur->has_xbzrle_cache_size) {
        xbzrle_cache_resize(cur->xbzrle_cache_size, errp);
    }
    if (cur->has_max_postcopy_bandwidth) {
        if (s->to_dst_file && migration_in_postcopy()) {
            migration_rate_set(cur->max_postcopy_bandwidth);
        }
    }
}
===

* I'd go with v1 above.
* Reasons for folding migrate_params_apply() and migrate_post_update_params():
   - Both static functions are called _only_ from
qmp_migrate_set_parameters(). They are not reusable from elsewhere.
   - Earlier migrate_params_apply() was quite long due to  if...else
conditionals, so it made sense to break one long function into 2-3
smaller ones.
   - Now with QAPI_CLONE/MERGE changes qmp_migrate_set_parameters() is
concise and easy to follow.
   - Reader can see all steps (1,2,3,4 above) together in one function.

* Just sharing my thoughts. You pick what you think is best.

Thank you.
---
  - Prasad

Re: [PATCH 5/5] migration/options: Use QAPI_MERGE in migrate_params_test_apply

[Fabiano Rosas]

Prasad Pandit <ppandit@redhat.com> writes:

> Hi,
>
> On Thu, 22 Jan 2026 at 03:41, Fabiano Rosas <farosas@suse.de> wrote:
>> See if the following looks better to you. It's not one single function,
>> but I folded what I could while keeping the distinct function names.
>>
>> static void migrate_params_apply(MigrationParameters *new, Error **errp)
>> {
>>     MigrationState *s = migrate_get_current();
>>     MigrationParameters *cur = &s->parameters;
>>     MigrationParameters *tmp = QAPI_CLONE(MigrationParameters, cur);
>>
>>     QAPI_MERGE(MigrationParameters, tmp, new);
>>
>>     if (!migrate_params_check(tmp, errp)) {
>>         return;
>>     }
>>
>>     migrate_ptr_opts_free(cur);
>>
>>     /* mark all present, so they're all copied */
>>     migrate_mark_all_params_present(params);
>>     QAPI_CLONE_MEMBERS(MigrationParameters, cur, params);
>>
>>     migrate_post_update_params(new, errp);
>> }
>>
>> void qmp_migrate_set_parameters(MigrationParameters *new, Error **errp)
>> {
>>     /*
>>      * Convert QTYPE_QNULL and NULL to the empty string (""). Even
>>      * though NULL is cleaner to deal with in C code, that would force
>>      * query-migrate-parameters to convert it once more to the empty
>>      * string, so avoid that. The migrate_tls_*() helpers that expose
>>      * the options to the rest of the migration code already use
>>      * return NULL when the empty string is found.
>>      */
>>     tls_opt_to_str(new->tls_creds);
>>     tls_opt_to_str(new->tls_hostname);
>>     tls_opt_to_str(new->tls_authz);
>>
>>     migrate_params_apply(new, errp);
>> }
>>
>
> * I'm thinking as:
> ===
> v0:
>
> void qmp_migrate_set_parameters(MigrationParameters *new, Error **errp)
> {
>     /*
>      * Convert QTYPE_QNULL and NULL to the empty string (""). Even
>      * though NULL is cleaner to deal with in C code, that would force
>      * query-migrate-parameters to convert it once more to the empty
>      * string, so avoid that. The migrate_tls_*() helpers that expose
>      * the options to the rest of the migration code already use
>      * return NULL when the empty string is found.
>      */
>     tls_opt_to_str(new->tls_creds);
>     tls_opt_to_str(new->tls_hostname);
>     tls_opt_to_str(new->tls_authz);
>
>     MigrationState *s = migrate_get_current();
>     MigrationParameters *cur = &s->parameters;
>     MigrationParameters *tmp = QAPI_CLONE(MigrationParameters, cur);
>
>     QAPI_MERGE(MigrationParameters, tmp, new);
>
>     if (!migrate_params_check(tmp, errp)) {
>         return;
>     }
>     migrate_ptr_opts_free(cur);
>
>     /* mark all present, so they're all copied */
>     migrate_mark_all_params_present(tpm);
>     QAPI_CLONE_MEMBERS(MigrationParameters, cur, tmp);
>
>     migrate_post_update_params(cur, errp);
> }
> ===
> v1:
>
> void qmp_migrate_set_parameters(MigrationParameters *new, Error **errp)
> {
>     /*
>      * Convert QTYPE_QNULL and NULL to the empty string (""). Even
>      * though NULL is cleaner to deal with in C code, that would force
>      * query-migrate-parameters to convert it once more to the empty
>      * string, so avoid that. The migrate_tls_*() helpers that expose
>      * the options to the rest of the migration code already use
>      * return NULL when the empty string is found.
>      */
>     tls_opt_to_str(new->tls_creds);
>     tls_opt_to_str(new->tls_hostname);
>     tls_opt_to_str(new->tls_authz);
>
>     MigrationState *s = migrate_get_current();
>     MigrationParameters *cur = &s->parameters;
>     MigrationParameters *tmp = QAPI_CLONE(MigrationParameters, cur);      ...[1]
>
>     QAPI_MERGE(MigrationParameters, tmp, new);
>                  ...[2]
>
>     if (!migrate_params_check(tmp, errp)) {
>         return;
>     }
>     migrate_ptr_opts_free(cur);
>
>     /* mark all present, so they're all copied */
>     migrate_mark_all_params_present(tpm);
>     QAPI_CLONE_MEMBERS(MigrationParameters, cur, tmp);
>        ...[3]
>
>     /* update MigrationState after parameters change */
>                      ...[4]
>     if (cur->has_max_bandwidth) {
>         if (s->to_dst_file && !migration_in_postcopy()) {
>             migration_rate_set(cur->max_bandwidth);
>         }
>     }
>     if (cur->has_x_checkpoint_delay) {
>         colo_checkpoint_delay_set();
>     }
>     if (cur->has_xbzrle_cache_size) {
>         xbzrle_cache_resize(cur->xbzrle_cache_size, errp);
>     }
>     if (cur->has_max_postcopy_bandwidth) {
>         if (s->to_dst_file && migration_in_postcopy()) {
>             migration_rate_set(cur->max_postcopy_bandwidth);
>         }
>     }
> }
> ===
>
> * I'd go with v1 above.
> * Reasons for folding migrate_params_apply() and migrate_post_update_params():
>    - Both static functions are called _only_ from
> qmp_migrate_set_parameters(). They are not reusable from elsewhere.
>    - Earlier migrate_params_apply() was quite long due to  if...else
> conditionals, so it made sense to break one long function into 2-3
> smaller ones.
>    - Now with QAPI_CLONE/MERGE changes qmp_migrate_set_parameters() is
> concise and easy to follow.
>    - Reader can see all steps (1,2,3,4 above) together in one function.
>
> * Just sharing my thoughts. You pick what you think is best.
>

Ok, thanks, let's see in what mood I'm in when reposting =)

> Thank you.
> ---
>   - Prasad