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

[Peter Xu <peterx@redhat.com>]

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)

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?

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

-- 
Peter Xu