Re: [PATCH v2 14/24] migration: Use visitors in migrate_params_test_apply

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Mon, Jun 30, 2025 at 04:59:03PM -0300, Fabiano Rosas wrote:
>> Convert the code in migrate_params_test_apply() from an open-coded
>> copy of every migration parameter to a copy using visitors. The
>> current code has conditionals for each parameter's has_* field, which
>> is exactly what the visitors do.
>> 
>> This hides the details of QAPI from the migration code and avoids the
>> need to update migrate_params_test_apply() every time a new migration
>> parameter is added. Both were very confusing and while the visitor
>> code can become a bit involved, there is no need for new contributors
>> to ever touch it.
>> 
>> Change the name of the function to a more direct reference of what it
>> does: merging the user params with the temporary copy.
>> 
>> Move the QAPI_CLONE_MEMBERS into the caller, so QAPI_CLONE can be used
>> and there's no need to allocate memory in the migration
>> code. Similarly, turn 'tmp' into a pointer so the proper qapi_free_
>> routine can be used.
>> 
>> An extra call to migrate_mark_all_params_present() is now needed
>> because the visitors update the has_ field for non-present fields, but
>> we actually want them all set so migrate_params_apply() can copy all
>> of them.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/options.c | 157 +++++++++++++++-----------------------------
>>  1 file changed, 54 insertions(+), 103 deletions(-)
>> 
>> diff --git a/migration/options.c b/migration/options.c
>> index 6619b5f21a..695bec5b8f 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -20,6 +20,10 @@
>>  #include "qapi/qapi-commands-migration.h"
>>  #include "qapi/qapi-visit-migration.h"
>>  #include "qapi/qmp/qerror.h"
>> +#include "qapi/qobject-input-visitor.h"
>> +#include "qapi/qobject-output-visitor.h"
>> +#include "qapi/visitor.h"
>> +#include "qobject/qdict.h"
>>  #include "qobject/qnull.h"
>>  #include "system/runstate.h"
>>  #include "migration/colo.h"
>> @@ -1223,120 +1227,63 @@ bool migrate_params_check(MigrationParameters *params, Error **errp)
>>      return true;
>>  }
>>  
>> -static void migrate_params_test_apply(MigrationParameters *params,
>> -                                      MigrationParameters *dest)
>> +static bool migrate_params_merge(MigrationParameters *dst,
>> +                                 MigrationParameters *src, Error **errp)
>>  {
>> -    MigrationState *s = migrate_get_current();
>> +    QObject *ret_out = NULL;
>> +    Visitor *v;
>> +    bool ok;
>>  
>> -    QAPI_CLONE_MEMBERS(MigrationParameters, dest, &s->parameters);
>> -
>> -    if (params->has_throttle_trigger_threshold) {
>> -        dest->throttle_trigger_threshold = params->throttle_trigger_threshold;
>> -    }
>> -
>> -    if (params->has_cpu_throttle_initial) {
>> -        dest->cpu_throttle_initial = params->cpu_throttle_initial;
>> -    }
>> -
>> -    if (params->has_cpu_throttle_increment) {
>> -        dest->cpu_throttle_increment = params->cpu_throttle_increment;
>> +    /* free memory from pointers that are about to be assigned */
>> +    if (src->has_block_bitmap_mapping) {
>> +        qapi_free_BitmapMigrationNodeAliasList(dst->block_bitmap_mapping);
>
> There're quite a few similar cases like this one in this series (including
> the rest below parameters below), where we free stuff but keep the dangling
> pointer around for a while (..hopefully!)..
>
> Should we still try to reset the pointers?  Am I the only one that is
> nervous with it?
>

Yeah, no worries, I can clear them all just to be sure.

>>      }
>>  
>> -    if (params->has_cpu_throttle_tailslow) {
>> -        dest->cpu_throttle_tailslow = params->cpu_throttle_tailslow;
>> +    if (src->tls_creds) {
>> +        qapi_free_StrOrNull(dst->tls_creds);
>>      }
>>  
>> -    if (params->tls_creds) {
>> -        qapi_free_StrOrNull(dest->tls_creds);
>> -        dest->tls_creds = QAPI_CLONE(StrOrNull, params->tls_creds);
>> +    if (src->tls_hostname) {
>> +        qapi_free_StrOrNull(dst->tls_hostname);
>>      }
>>  
>> -    if (params->tls_hostname) {
>> -        qapi_free_StrOrNull(dest->tls_hostname);
>> -        dest->tls_hostname = QAPI_CLONE(StrOrNull, params->tls_hostname);
>> +    if (src->tls_authz) {
>> +        qapi_free_StrOrNull(dst->tls_authz);
>>      }
>>  
>> -    if (params->tls_authz) {
>> -        qapi_free_StrOrNull(dest->tls_authz);
>> -        dest->tls_authz = QAPI_CLONE(StrOrNull, params->tls_authz);
>> +    /* read in from src */
>> +    v = qobject_output_visitor_new(&ret_out);
>> +    ok = visit_type_MigrationParameters(v, NULL, &src, errp);
>> +    if (!ok) {
>> +        goto out;
>>      }
>> +    visit_complete(v, &ret_out);
>> +    visit_free(v);
>>  
>> -    if (params->has_max_bandwidth) {
>> -        dest->max_bandwidth = params->max_bandwidth;
>> +    /*
>> +     * Write to dst but leave existing fields intact (except for has_*
>> +     * which will be updated according to their presence in src).
>> +     */
>> +    v = qobject_input_visitor_new(ret_out);
>> +    ok = visit_start_struct(v, NULL, NULL, 0, errp);
>> +    if (!ok) {
>> +        goto out;
>>      }
>> -
>> -    if (params->has_avail_switchover_bandwidth) {
>> -        dest->avail_switchover_bandwidth = params->avail_switchover_bandwidth;
>> +    ok = visit_type_MigrationParameters_members(v, dst, errp);
>> +    if (!ok) {
>> +        goto out;
>>      }
>> -
>> -    if (params->has_downtime_limit) {
>> -        dest->downtime_limit = params->downtime_limit;
>> +    ok = visit_check_struct(v, errp);
>> +    visit_end_struct(v, NULL);
>> +    if (!ok) {
>> +        goto out;
>>      }
>>  
>> -    if (params->has_x_checkpoint_delay) {
>> -        dest->x_checkpoint_delay = params->x_checkpoint_delay;
>> -    }
>> +out:
>> +    visit_free(v);
>> +    qobject_unref(ret_out);
>
> IIUC this is essential the trick we used to play before QAPI_CLONE, before
> commit a15fcc3cf69e.
>
> https://lore.kernel.org/all/1465490926-28625-15-git-send-email-eblake@redhat.com/
>
> Yes, looks similar..
>
> QAPI_CLONE_MEMBERS() will copy everything, which we do not want here. We
> only want to copy where has_* is set.  So it's indeed a sligntly different
> request versus the current clone API.
>
> IIUC that can be implemented using a similar qapi clone visitor, however
> instead of g_memdup() on the structs/lists first (or in the case of
> QAPI_CLONE_MEMBERS, we did *dst=*src), we lazy copy all the fields.
>
> I wished this is a generic API we could use.  I think it means we'll
> maintain this ourselves.  Maybe it's OK.
>

I'm not sure how easy it is to provide a generic API for this. I don't
think there's much space for this code to change anyway, so is fine to
keep it in migration. I'll try to implement a
QAPI_CLONE_PRESENT_MEMBERS, let's see.

>>  
>> -    if (params->has_multifd_channels) {
>> -        dest->multifd_channels = params->multifd_channels;
>> -    }
>> -    if (params->has_multifd_compression) {
>> -        dest->multifd_compression = params->multifd_compression;
>> -    }
>> -    if (params->has_multifd_qatzip_level) {
>> -        dest->multifd_qatzip_level = params->multifd_qatzip_level;
>> -    }
>> -    if (params->has_multifd_zlib_level) {
>> -        dest->multifd_zlib_level = params->multifd_zlib_level;
>> -    }
>> -    if (params->has_multifd_zstd_level) {
>> -        dest->multifd_zstd_level = params->multifd_zstd_level;
>> -    }
>> -    if (params->has_xbzrle_cache_size) {
>> -        dest->xbzrle_cache_size = params->xbzrle_cache_size;
>> -    }
>> -    if (params->has_max_postcopy_bandwidth) {
>> -        dest->max_postcopy_bandwidth = params->max_postcopy_bandwidth;
>> -    }
>> -    if (params->has_max_cpu_throttle) {
>> -        dest->max_cpu_throttle = params->max_cpu_throttle;
>> -    }
>> -    if (params->has_announce_initial) {
>> -        dest->announce_initial = params->announce_initial;
>> -    }
>> -    if (params->has_announce_max) {
>> -        dest->announce_max = params->announce_max;
>> -    }
>> -    if (params->has_announce_rounds) {
>> -        dest->announce_rounds = params->announce_rounds;
>> -    }
>> -    if (params->has_announce_step) {
>> -        dest->announce_step = params->announce_step;
>> -    }
>> -
>> -    if (params->has_block_bitmap_mapping) {
>> -        dest->block_bitmap_mapping = params->block_bitmap_mapping;
>> -    }
>> -
>> -    if (params->has_x_vcpu_dirty_limit_period) {
>> -        dest->x_vcpu_dirty_limit_period =
>> -            params->x_vcpu_dirty_limit_period;
>> -    }
>> -    if (params->has_vcpu_dirty_limit) {
>> -        dest->vcpu_dirty_limit = params->vcpu_dirty_limit;
>> -    }
>> -
>> -    if (params->has_mode) {
>> -        dest->mode = params->mode;
>> -    }
>> -
>> -    if (params->has_zero_page_detection) {
>> -        dest->zero_page_detection = params->zero_page_detection;
>> -    }
>> -
>> -    if (params->has_direct_io) {
>> -        dest->direct_io = params->direct_io;
>> -    }
>> +    return ok;
>>  }
>>  
>>  static void migrate_params_apply(MigrationParameters *params)
>> @@ -1353,7 +1300,9 @@ static void migrate_params_apply(MigrationParameters *params)
>>  
>>  void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
>>  {
>> -    MigrationParameters tmp;
>> +    MigrationState *s = migrate_get_current();
>> +    g_autoptr(MigrationParameters) tmp = QAPI_CLONE(MigrationParameters,
>> +                                                    &s->parameters);
>>  
>>      /*
>>       * Convert QTYPE_QNULL and NULL to the empty string (""). Even
>> @@ -1367,7 +1316,9 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
>>      tls_opt_to_str(&params->tls_hostname);
>>      tls_opt_to_str(&params->tls_authz);
>>  
>> -    migrate_params_test_apply(params, &tmp);
>> +    if (!migrate_params_merge(tmp, params, errp)) {
>> +        return;
>> +    }
>>  
>>      /*
>>       * Mark block_bitmap_mapping as present now while we have the
>> @@ -1377,10 +1328,10 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
>>          migrate_get_current()->has_block_bitmap_mapping = true;
>>      }
>>  
>> -    if (migrate_params_check(&tmp, errp)) {
>> -        migrate_params_apply(&tmp);
>> +    if (migrate_params_check(tmp, errp)) {
>> +        /* mark all present, so they're all copied */
>> +        migrate_mark_all_params_present(tmp);
>
> Ah, I just asked a question in the previous patch on what happens if some
> of the fields will be present.  Looks like this line is the answer?
>

Yes, it's either here, or via migrate_param_init().

> This isn't very obvious.  Maybe some comment in migrate_params_apply()
> would slightly help (which mentions it must be used when all parameters are
> set)?  Or some way to assert it?
>

Yep, I'll add more safeguards.

>> +        migrate_params_apply(tmp);
>>          migrate_post_update_params(params, errp);
>>      }
>> -
>> -    migrate_tls_opts_free(&tmp);
>>  }
>> -- 
>> 2.35.3
>>