Peter Xu <pet...@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 <faro...@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
>>
