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

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?

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

-- 
Peter Xu

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.

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

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

On Thu, Jun 12, 2025 at 05:58:14PM -0300, Fabiano Rosas wrote:
> 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);

[1]

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

[2]

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

Oops, indeed.  Now you have that function to set all has_*, hopefully this
is trivial now to still do so.

Since you mentioned tls_* won't have has_*, but they will get properly
cloned IIUC as you mentioned above [2].  Does it mean we can also drop the
three lines at [1] too?

In general, I am curious why we can't already use QAPI_CLONE() like:

  params = QAPI_CLONE(&s->parameters);

And if my wish came true once more on having it a pointer (meanwhile if it
even happened before this patch):

  params = QAPI_CLONE(s->parameters);

I thought with that, any of "g_malloc0(), copying of tls_*, copying of
block_bitmap things" are all not needed?

-- 
Peter Xu

Peter Xu <peterx@redhat.com> writes:

> On Thu, Jun 12, 2025 at 05:58:14PM -0300, Fabiano Rosas wrote:
>> 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);
>
> [1]
>
>> >>>  
>> >>> -    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;
>
> [2]
>
>> 
>> 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.
>
> Oops, indeed.  Now you have that function to set all has_*, hopefully this
> is trivial now to still do so.
>

Yes.

> Since you mentioned tls_* won't have has_*, but they will get properly
> cloned IIUC as you mentioned above [2].  Does it mean we can also drop the
> three lines at [1] too?
>

I'm thinking yes as well, still woking on it.

> In general, I am curious why we can't already use QAPI_CLONE() like:
>
>   params = QAPI_CLONE(&s->parameters);
>
> And if my wish came true once more on having it a pointer (meanwhile if it
> even happened before this patch):
>
>   params = QAPI_CLONE(s->parameters);
>
> I thought with that, any of "g_malloc0(), copying of tls_*, copying of
> block_bitmap things" are all not needed?

Same here.