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.0On 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
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
>>
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
>>>
© 2016 - 2026 Red Hat, Inc.