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