Peter Xu <pet...@redhat.com> writes:
> On Thu, Jun 12, 2025 at 05:58:14PM -0300, Fabiano Rosas wrote:
>> Fabiano Rosas <faro...@suse.de> writes:
>>
>> > Peter Xu <pet...@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 <faro...@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(¶ms->tls_creds, s->parameters.tls_creds);
>> >>> tls_option_set_str(¶ms->tls_hostname,
>> >>> s->parameters.tls_hostname);
>> >>> tls_option_set_str(¶ms->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.
