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

[Fabiano Rosas <farosas@suse.de>]

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