Re: [PATCH 10/21] migration: Use QAPI_CLONE_MEMBERS in query_migrate_parameters

[Fabiano Rosas <farosas@suse.de>]

Fabiano Rosas <farosas@suse.de> writes:

> Peter Xu <peterx@redhat.com> writes:
>
>> On Mon, Jun 02, 2025 at 10:37:59PM -0300, Fabiano Rosas wrote:
>>> QAPI_CLONE_MEMBERS is a better option than copying parameters one by
>>> one because it operates on the entire struct and follows pointers. It
>>> also avoids the need to alter this function every time a new parameter
>>> is added.
>>> 
>>> Note, since this is a deep clone, now we must free the TLS strings
>>> before assignment.
>>> 
>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>> ---
>>>  migration/options.c | 31 ++++---------------------------
>>>  1 file changed, 4 insertions(+), 27 deletions(-)
>>> 
>>> diff --git a/migration/options.c b/migration/options.c
>>> index dd62e726cb..0a2a3050ec 100644
>>> --- a/migration/options.c
>>> +++ b/migration/options.c
>>> @@ -918,7 +918,9 @@ static void tls_option_set_str(StrOrNull **dstp, StrOrNull *src)
>>>  {
>>>      StrOrNull *dst = *dstp;
>>>  
>>> -    assert(!dst);
>>> +    if (dst) {
>>> +        qapi_free_StrOrNull(dst);
>>> +    }
>>>  
>>>      dst = *dstp = g_new0(StrOrNull, 1);
>>>      dst->type = QTYPE_QSTRING;
>>> @@ -975,42 +977,17 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>>>      MigrationParameters *params;
>>>      MigrationState *s = migrate_get_current();
>>>  
>>> -    /* TODO use QAPI_CLONE() instead of duplicating it inline */
>>>      params = g_malloc0(sizeof(*params));
>>>  
>>> -    params->throttle_trigger_threshold = s->parameters.throttle_trigger_threshold;
>>> -    params->cpu_throttle_initial = s->parameters.cpu_throttle_initial;
>>> -    params->cpu_throttle_increment = s->parameters.cpu_throttle_increment;
>>> -    params->cpu_throttle_tailslow = s->parameters.cpu_throttle_tailslow;
>>> +    QAPI_CLONE_MEMBERS(MigrationParameters, params, &s->parameters);
>>>  
>>>      tls_option_set_str(&params->tls_creds, s->parameters.tls_creds);
>>>      tls_option_set_str(&params->tls_hostname, s->parameters.tls_hostname);
>>>      tls_option_set_str(&params->tls_authz, s->parameters.tls_authz);
>>>  
>>> -    params->max_bandwidth = s->parameters.max_bandwidth;
>>> -    params->avail_switchover_bandwidth = s->parameters.avail_switchover_bandwidth;
>>> -    params->downtime_limit = s->parameters.downtime_limit;
>>> -    params->x_checkpoint_delay = s->parameters.x_checkpoint_delay;
>>> -    params->multifd_channels = s->parameters.multifd_channels;
>>> -    params->multifd_compression = s->parameters.multifd_compression;
>>> -    params->multifd_zlib_level = s->parameters.multifd_zlib_level;
>>> -    params->multifd_qatzip_level = s->parameters.multifd_qatzip_level;
>>> -    params->multifd_zstd_level = s->parameters.multifd_zstd_level;
>>> -    params->xbzrle_cache_size = s->parameters.xbzrle_cache_size;
>>> -    params->max_postcopy_bandwidth = s->parameters.max_postcopy_bandwidth;
>>> -    params->max_cpu_throttle = s->parameters.max_cpu_throttle;
>>> -    params->announce_initial = s->parameters.announce_initial;
>>> -    params->announce_max = s->parameters.announce_max;
>>> -    params->announce_rounds = s->parameters.announce_rounds;
>>> -    params->announce_step = s->parameters.announce_step;
>>>      params->block_bitmap_mapping =
>>>          QAPI_CLONE(BitmapMigrationNodeAliasList,
>>>                     s->parameters.block_bitmap_mapping);
>>
>> Wouldn't the QAPI_CLONE_MEMBERS() have deep cloned this too?
>>
>
> Hmm, I think it should. But it definitely broke something without this
> line. I'll double check.
>

Thanks for the question, this was indeed wrong. QAPI_CLONE_MEMBERS
depend on the has_* fields on src, otherwise it's just a glorified
assignment (*dst = src). The reason I got this wrong is that I was using
the TLS strings to test and they have a different handling in QAPI:

visit_type_MigrationParameters_members():

    bool has_tls_creds = !!obj->tls_creds;

So the code was working for them, but not for block_bitmap_mapping, for
which the QAPI has:

if (visit_optional(v, "block-bitmap-mapping", &obj->has_block_bitmap_mapping)) {
                                                    ^
    if (!visit_type_BitmapMigrationNodeAliasList(v, "block-bitmap-mapping",
        &obj->block_bitmap_mapping, errp)) {
        return false;
    }
}

IOW, the QAPI_CLONE routines depend on the has_ fields (in retrospect:
obviously).

That assert you didn't like will have to go then and s->parameters will
have to have all has_* fields permanently set. Not a huge deal, but it
undermines my argument of keeping it free from QAPI details.

>>> -    params->x_vcpu_dirty_limit_period = s->parameters.x_vcpu_dirty_limit_period;
>>> -    params->vcpu_dirty_limit = s->parameters.vcpu_dirty_limit;
>>> -    params->mode = s->parameters.mode;
>>> -    params->zero_page_detection = s->parameters.zero_page_detection;
>>> -    params->direct_io = s->parameters.direct_io;
>>>  
>>>      /*
>>>       * query-migrate-parameters expects all members of
>>> -- 
>>> 2.35.3
>>>