Re: [PATCH 19/21] migration: Allow migrate commands to provide the migration config

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Mon, Jun 02, 2025 at 10:38:08PM -0300, Fabiano Rosas wrote:
>> Allow the migrate and migrate_incoming commands to pass the migration
>> configuration options all at once, dispensing the use of
>> migrate-set-parameters and migrate-set-capabilities.
>> 
>> The motivation of this is to simplify the interface with the
>> management layer and avoid the usage of several command invocations to
>> configure a migration. It also avoids stale parameters from a previous
>> migration to influence the current migration.
>> 
>> The options that are changed during the migration can still be set
>> with the existing commands.
>> 
>> The order of precedence is:
>> 
>> 'config' argument > -global cmdline > defaults (migration_properties)
>
> Could we still keep the QMP migrate-set-parameters values?
>
>   'config' argument > QMP setups using migrate-set-parameters >
>     -global cmdline > defaults (migration_properties)
>

That's the case. I failed to mention it in the commit message. IOW it
behaves just like today, but the new 'config' way takes precedence over
all.

> I asked this before, maybe I forgot the answer..
>
>> 
>> I.e. the config takes precedence over all, values not present in the
>> config assume the default values. The (debug) -global command line
>> option allows the defaults to be overridden.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/migration-hmp-cmds.c |  5 +++--
>>  migration/migration.c          | 29 ++++++++++++++++++++++++++---
>>  migration/migration.h          |  1 +
>>  migration/options.c            | 30 ++++++++++++++++++++++++++++++
>>  migration/options.h            |  3 +++
>>  qapi/migration.json            | 25 +++++++++++++++++++++++--
>>  system/vl.c                    |  3 ++-
>>  7 files changed, 88 insertions(+), 8 deletions(-)
>> 
>> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
>> index a8c3515e9d..38b289e8d8 100644
>> --- a/migration/migration-hmp-cmds.c
>> +++ b/migration/migration-hmp-cmds.c
>> @@ -575,7 +575,7 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
>>      }
>>      QAPI_LIST_PREPEND(caps, g_steal_pointer(&channel));
>>  
>> -    qmp_migrate_incoming(NULL, true, caps, true, false, &err);
>> +    qmp_migrate_incoming(NULL, true, caps, NULL, true, false, &err);
>>      qapi_free_MigrationChannelList(caps);
>>  
>>  end:
>> @@ -952,7 +952,8 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
>>      }
>>      QAPI_LIST_PREPEND(caps, g_steal_pointer(&channel));
>>  
>> -    qmp_migrate(NULL, true, caps, false, false, true, resume, &err);
>> +    qmp_migrate(NULL, true, caps, NULL, false, false, true, resume,
>> +                &err);
>>      if (hmp_handle_error(mon, err)) {
>>          return;
>>      }
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 75c4ec9a95..7b450b8836 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -335,6 +335,7 @@ void migration_object_init(void)
>>      current_incoming->exit_on_error = INMIGRATE_DEFAULT_EXIT_ON_ERROR;
>>  
>>      migration_object_check(current_migration, &error_fatal);
>> +    migrate_params_store_defaults(current_migration);
>>  
>>      ram_mig_init();
>>      dirty_bitmap_mig_init();
>> @@ -1916,13 +1917,24 @@ void migrate_del_blocker(Error **reasonp)
>>  
>>  void qmp_migrate_incoming(const char *uri, bool has_channels,
>>                            MigrationChannelList *channels,
>> -                          bool has_exit_on_error, bool exit_on_error,
>> -                          Error **errp)
>> +                          MigrationParameters *config, bool has_exit_on_error,
>> +                          bool exit_on_error, Error **errp)
>>  {
>>      Error *local_err = NULL;
>>      static bool once = true;
>> +    MigrationState *s = migrate_get_current();
>>      MigrationIncomingState *mis = migration_incoming_get_current();
>>  
>> +    if (config) {
>> +        /*
>> +         * If a config was provided, all options set previously get
>> +         * ignored.
>> +         */
>> +        if (!migrate_params_override(s, config, errp)) {
>> +            return;
>> +        }
>> +    }
>> +
>>      if (!once) {
>>          error_setg(errp, "The incoming migration has already been started");
>>          return;
>> @@ -2182,7 +2194,8 @@ static gboolean qmp_migrate_finish_cb(QIOChannel *channel,
>>  }
>>  
>>  void qmp_migrate(const char *uri, bool has_channels,
>> -                 MigrationChannelList *channels, bool has_detach, bool detach,
>> +                 MigrationChannelList *channels,
>> +                 bool has_detach, bool detach, MigrationParameters *config,
>>                   bool has_resume, bool resume, Error **errp)
>>  {
>>      bool resume_requested;
>> @@ -2193,6 +2206,16 @@ void qmp_migrate(const char *uri, bool has_channels,
>>      MigrationChannel *channelv[MIGRATION_CHANNEL_TYPE__MAX] = { NULL };
>>      MigrationChannel *cpr_channel = NULL;
>>  
>> +    if (config) {
>> +        /*
>> +         * If a config was provided, all options set previously get
>> +         * ignored.
>> +         */
>> +        if (!migrate_params_override(s, config, errp)) {
>> +            return;
>> +        }
>> +    }
>> +
>>      /*
>>       * Having preliminary checks for uri and channel
>>       */
>> diff --git a/migration/migration.h b/migration/migration.h
>> index 993d51aedd..49761f4699 100644
>> --- a/migration/migration.h
>> +++ b/migration/migration.h
>> @@ -319,6 +319,7 @@ struct MigrationState {
>>  
>>      /* params from 'migrate-set-parameters' */
>>      MigrationParameters parameters;
>> +    MigrationParameters defaults;
>
> This is also prone to be a pointer; I still think embeded qapi objects are
> too error prone.  Since it's new, make it a pointer from start?
>

Ok.

>>  
>>      MigrationStatus state;
>>  
>> diff --git a/migration/options.c b/migration/options.c
>> index fa3f7035c8..dd2288187d 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -1333,6 +1333,36 @@ static void migrate_params_apply(MigrationParameters *params)
>>                                             params->block_bitmap_mapping);
>>  }
>>  
>> +void migrate_params_store_defaults(MigrationState *s)
>> +{
>> +    /*
>> +     * The defaults set for each qdev property in migration_properties
>> +     * will be stored as the default values for each migration
>> +     * parameter. For debugging, using -global can override the
>> +     * defaults.
>> +     */
>> +    QAPI_CLONE_MEMBERS(MigrationParameters, &s->defaults, &s->parameters);
>> +}
>> +
>> +bool migrate_params_override(MigrationState *s, MigrationParameters *new,
>> +                             Error **errp)
>> +{
>> +    ERRP_GUARD();
>> +
>> +    assert(bql_locked());
>> +
>> +    /* reset to default parameters */
>> +    migrate_params_apply(&s->defaults);
>> +
>> +    /* overwrite with the new ones */
>> +    qmp_migrate_set_parameters(new, errp);
>> +    if (*errp) {
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>>  void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
>>  {
>>      MigrationParameters *tmp = g_new0(MigrationParameters, 1);
>> diff --git a/migration/options.h b/migration/options.h
>> index fcfd120cd7..3630c2a0dd 100644
>> --- a/migration/options.h
>> +++ b/migration/options.h
>> @@ -83,4 +83,7 @@ void migrate_capability_set_compat(MigrationParameters *params, int i,
>>  void migrate_capabilities_set_compat(MigrationParameters *params,
>>                                       MigrationCapabilityStatusList *caps);
>>  bool migrate_caps_check(MigrationParameters *new, Error **errp);
>> +void migrate_params_store_defaults(MigrationState *s);
>> +bool migrate_params_override(MigrationState *s, MigrationParameters *new,
>> +                             Error **errp);
>>  #endif
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index 7282e4b9eb..64a92d8d28 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1474,9 +1474,16 @@
>>  #
>>  # @resume: resume one paused migration, default "off".  (since 3.0)
>>  #
>> +# @config: migration configuration options, previously set via
>> +#     @migrate-set-parameters and @migrate-set-capabilities.  (since
>> +#     10.1)
>> +#
>>  # Features:
>>  #
>>  # @deprecated: Argument @detach is deprecated.
>> +# @config: Indicates this command can receive the entire migration
>> +# configuration via the @config field, dispensing the use of
>> +# @migrate-set-parameters.
>>  #
>>  # Since: 0.14
>>  #
>> @@ -1538,7 +1545,9 @@
>>    'data': {'*uri': 'str',
>>             '*channels': [ 'MigrationChannel' ],
>>             '*detach': { 'type': 'bool', 'features': [ 'deprecated' ] },
>> -           '*resume': 'bool' } }
>> +           '*config': 'MigrationParameters',
>> +           '*resume': 'bool' },
>> +  'features': [ 'config' ] }
>>  
>>  ##
>>  # @migrate-incoming:
>> @@ -1557,6 +1566,16 @@
>>  #     error details could be retrieved with query-migrate.
>>  #     (since 9.1)
>>  #
>> +# @config: migration configuration options, previously set via
>> +#     @migrate-set-parameters and @migrate-set-capabilities.  (since
>> +#     10.1)
>> +#
>> +# Features:
>> +#
>> +# @config: Indicates this command can receive the entire migration
>> +# configuration via the @config field, dispensing the use of
>> +# @migrate-set-parameters.
>> +#
>>  # Since: 2.3
>>  #
>>  # .. admonition:: Notes
>> @@ -1610,7 +1629,9 @@
>>  { 'command': 'migrate-incoming',
>>               'data': {'*uri': 'str',
>>                        '*channels': [ 'MigrationChannel' ],
>> -                      '*exit-on-error': 'bool' } }
>> +                      '*config': 'MigrationParameters',
>> +                      '*exit-on-error': 'bool' },
>> +             'features': [ 'config' ] }
>>  
>>  ##
>>  # @xen-save-devices-state:
>> diff --git a/system/vl.c b/system/vl.c
>> index 3b7057e6c6..b29fd24d08 100644
>> --- a/system/vl.c
>> +++ b/system/vl.c
>> @@ -2823,7 +2823,8 @@ void qmp_x_exit_preconfig(Error **errp)
>>                  g_new0(MigrationChannelList, 1);
>>  
>>              channels->value = incoming_channels[MIGRATION_CHANNEL_TYPE_MAIN];
>> -            qmp_migrate_incoming(NULL, true, channels, true, true, &local_err);
>> +            qmp_migrate_incoming(NULL, true, channels, NULL, true, true,
>> +                                 &local_err);
>>              if (local_err) {
>>                  error_reportf_err(local_err, "-incoming %s: ", incoming);
>>                  exit(1);
>> -- 
>> 2.35.3
>>