Hi everyone, I did a cleanup (if it can be called that) of the user
input validation for capabilities and parameters and turned the two
concepts into a single 'options' to be stored in a MigrationConfig
object.

RFC mostly because this idea exposes (pre-existing) issues around how
to validate capabilities that are mutually excludent and options that
need to be enabled together.

I'd also like some feedback on what approach to take regarding
compatibility.

Let me know what you think. I know it's a lot to look at, any comments
are welcomed and don't worry on trying to cover everything. This is
long-term work. Thank you.

The reasons for this work are:
------------------------------

Capabilities are just boolean parameters that can only be set before
migration. For the majority of the code there's no distinction between
the two.

Having a single structure allows the qmp_migrate command to receive
the migration configuration all at once:

{ 'command': 'migrate',
  'data': {'*uri': 'str',
           '*channels': [ 'MigrationChannel' ],
+	   '*config': 'MigrationConfig',
           '*detach': 'bool', '*resume': 'bool' } }

+{ 'struct': 'MigrationConfig',
+  'data': { '*announce-initial': 'size',
+            '*announce-max': 'size',
+	     ...   <-- all parameters and capabilities as optional
+} }

(optionally fold 'detach' and 'resume' into MigrationConfig)

Other benefits of a single configuration struct:

- allows the removal of two commands:
  migrate-set-capabilities/query-capabilities which simplifies the
  user interface (migrate-set-parameters, or similar, is still
  required for options that need to be adjusted during migration);

- removes (some of) the triplication in migration.json;

- simplifies the (future) work of implementing a handshake feature for
  migration: only one structure to negotiate over and less reliance on
  commands other than 'migrate'.

The major changes in this series are:
-------------------------------------

- Add a (QAPI) type hierarchy:

                               MigrationConfigBase
                              (most config options)
                                       |
             +-------------------------|-------------------------+
             |                         |                         |
    MigrationConfig          MigrationParameters        MigrateSetParameters
 (internal use, s->config,   (compat with               (compat with
 new query/set-config)       query-migrate-parameters)  migrate-set-parameters)

- Remove migrate_params_test_apply. This function duplicates a lot of code;

- Add compatibility routines to convert from the existing QMP user
  input into the new MigrationConfig for internal use;

- Merge capabilities and parameters validation into one function;

- Convert qmp_migrate and qmp_migrate_incoming to use the new structure.

Open questions:
---------------

- Deprecations/compat?

I think we should deprecate migrate-set/query-capabilities and everything to do
with capabilities (specifically the validation in the JSON at the end of the
stream).

For migrate-set/query-parameters, we could probably keep it around indefinitely,
but it'd be convenient to introduce new commands so we can give them new
semantics.

- How to restrict the options that should not be set when the migration is in
progress?

i.e.:
  all options can be set before migration (initial config)
  some options can be set during migration (runtime)

I thought of adding another type at the top of the hierarchy, with
just the options allowed to change at runtime, but that doesn't really
stop the others being also set at runtime. I'd need a way to have a
set of options that are rejected 'if migration_is_running()', without
adding more duplication all around.

- What about savevm?

None of this solves the issue of random caps/params being set before
calling savevm. We still need to special-case savevm and reject
everything. Unless we entirely deprecate setting initial options via
set-parameters (or set-config) and require all options to be set as
savevm (and migrate) arguments.

- HMP?

Can we convert the strings passed via hmp_set_parameters without
having an enum of parameters? Duplication problem again.

- incoming defer?

It seems we cannot do the final step of removing
migrate-set-capabilites before we have a form of handshake
implemented. That would take the config from qmp_migrate on source and
send it to the destination for negotiation.

- last but definitely not least:

Changing caps from a list of bools into struct members complicates the
validation because some caps must be checked against every other cap
already set. And the user could be playing games with switching caps
on and off, so there's a lot of redundant checking the must be
made. The current code already has a bunch of gaps in that regard.

For this series I opted to not check has_* fields for capabilities,
i.e. to validate them all every time migrate_config_check() is called.

Fabiano Rosas (12):
  migration: Normalize tls arguments
  migration: Run a post update routine after setting parameters
  migration: Fix parameter validation
  migration: Reduce a bit of duplication in migration.json
  migration: Remove the parameters copy during validation
  migration: Introduce new MigrationConfig structure
  migration: Replace s->parameters with s->config
  migration: Do away with usage of QERR_INVALID_PARAMETER_VALUE
  migration: Replace s->capabilities with s->config
  migration: Merge parameters and capability checks
  [PoC] migration: Add query/set commands for MigrationConfig
  [PoC] migration: Allow migrate commands to provide the migration
    config

Markus Armbruster (1):
  migration: Fix latent bug in migrate_params_test_apply()

 migration/migration-hmp-cmds.c |    5 +-
 migration/migration.c          |   52 +-
 migration/migration.h          |    5 +-
 migration/options.c            | 1386 ++++++++++++++++----------------
 migration/options.h            |   25 +-
 migration/page_cache.c         |    6 +-
 migration/ram.c                |    9 +-
 migration/savevm.c             |    8 +-
 migration/tls.c                |    2 +-
 qapi/migration.json            |  571 +++++++------
 system/vl.c                    |    3 +-
 11 files changed, 1079 insertions(+), 993 deletions(-)

-- 
2.35.3

On Fri, Apr 11, 2025 at 04:14:30PM -0300, Fabiano Rosas wrote:
> Open questions:
> ---------------
> 
> - Deprecations/compat?
> 
> I think we should deprecate migrate-set/query-capabilities and everything to do
> with capabilities (specifically the validation in the JSON at the end of the
> stream).
> 
> For migrate-set/query-parameters, we could probably keep it around indefinitely,
> but it'd be convenient to introduce new commands so we can give them new
> semantics.
> 
> - How to restrict the options that should not be set when the migration is in
> progress?
> 
> i.e.:
>   all options can be set before migration (initial config)
>   some options can be set during migration (runtime)
> 
> I thought of adding another type at the top of the hierarchy, with
> just the options allowed to change at runtime, but that doesn't really
> stop the others being also set at runtime. I'd need a way to have a
> set of options that are rejected 'if migration_is_running()', without
> adding more duplication all around.
> 
> - What about savevm?
> 
> None of this solves the issue of random caps/params being set before
> calling savevm. We still need to special-case savevm and reject
> everything. Unless we entirely deprecate setting initial options via
> set-parameters (or set-config) and require all options to be set as
> savevm (and migrate) arguments.

I'd suggest we aim for a world where the commands take all options
as direct args and try to remove the global state eventually.

For savevm/loadvm in particular it is very much a foot-gun that
'migrate-set-*' will affect them, because savevm/loadvm aren't
obviously connected to 'migrate-*' commands unless you're aware
of how QEMU implements savevm internally.

> - HMP?
> 
> Can we convert the strings passed via hmp_set_parameters without
> having an enum of parameters? Duplication problem again.

> 
> - incoming defer?
> 
> It seems we cannot do the final step of removing
> migrate-set-capabilites before we have a form of handshake
> implemented. That would take the config from qmp_migrate on source and
> send it to the destination for negotiation.

I'm not sure I understand why the QAPI design changes are tied
to the new protocol handshake ? I guess you're wanting to avoid
updating 'migrate_incoming' to accept the new parameters directly ?


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Apr 11, 2025 at 04:14:30PM -0300, Fabiano Rosas wrote:
>> Open questions:
>> ---------------
>> 
>> - Deprecations/compat?
>> 
>> I think we should deprecate migrate-set/query-capabilities and everything to do
>> with capabilities (specifically the validation in the JSON at the end of the
>> stream).
>> 
>> For migrate-set/query-parameters, we could probably keep it around indefinitely,
>> but it'd be convenient to introduce new commands so we can give them new
>> semantics.
>> 
>> - How to restrict the options that should not be set when the migration is in
>> progress?
>> 
>> i.e.:
>>   all options can be set before migration (initial config)
>>   some options can be set during migration (runtime)
>> 
>> I thought of adding another type at the top of the hierarchy, with
>> just the options allowed to change at runtime, but that doesn't really
>> stop the others being also set at runtime. I'd need a way to have a
>> set of options that are rejected 'if migration_is_running()', without
>> adding more duplication all around.
>> 
>> - What about savevm?
>> 
>> None of this solves the issue of random caps/params being set before
>> calling savevm. We still need to special-case savevm and reject
>> everything. Unless we entirely deprecate setting initial options via
>> set-parameters (or set-config) and require all options to be set as
>> savevm (and migrate) arguments.
>
> I'd suggest we aim for a world where the commands take all options
> as direct args and try to remove the global state eventually.

Yes.

Even better: make it a job.

[...]

Markus Armbruster <armbru@redhat.com> writes:

> Daniel P. Berrangé <berrange@redhat.com> writes:
>
>> On Fri, Apr 11, 2025 at 04:14:30PM -0300, Fabiano Rosas wrote:
>>> Open questions:
>>> ---------------
>>> 
>>> - Deprecations/compat?
>>> 
>>> I think we should deprecate migrate-set/query-capabilities and everything to do
>>> with capabilities (specifically the validation in the JSON at the end of the
>>> stream).
>>> 
>>> For migrate-set/query-parameters, we could probably keep it around indefinitely,
>>> but it'd be convenient to introduce new commands so we can give them new
>>> semantics.
>>> 
>>> - How to restrict the options that should not be set when the migration is in
>>> progress?
>>> 
>>> i.e.:
>>>   all options can be set before migration (initial config)
>>>   some options can be set during migration (runtime)
>>> 
>>> I thought of adding another type at the top of the hierarchy, with
>>> just the options allowed to change at runtime, but that doesn't really
>>> stop the others being also set at runtime. I'd need a way to have a
>>> set of options that are rejected 'if migration_is_running()', without
>>> adding more duplication all around.
>>> 
>>> - What about savevm?
>>> 
>>> None of this solves the issue of random caps/params being set before
>>> calling savevm. We still need to special-case savevm and reject
>>> everything. Unless we entirely deprecate setting initial options via
>>> set-parameters (or set-config) and require all options to be set as
>>> savevm (and migrate) arguments.
>>
>> I'd suggest we aim for a world where the commands take all options
>> as direct args and try to remove the global state eventually.
>
> Yes.
>
> Even better: make it a job.
>

What do we gain from that in relation to being able to pass ~50
parameters to a command? I don't see it. I think that's the actual crux
here, too many options and how to get them from the QAPI into the
migration core for consumption.

The current usage of MigrationParameters as both the return type for
query-set-parameters and the global parameter store for the migration
state is really dissonant. What do the has_* fields even mean when
accessed via MigrationState::parameters? This series is not doing any
better in that regard, mind you. I'm almost tempted to ask that we
expose the QDict from the marshaling function directly to the migration
code, at least that's a data type that makes sense internally.

Fabiano Rosas <farosas@suse.de> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>
>>> On Fri, Apr 11, 2025 at 04:14:30PM -0300, Fabiano Rosas wrote:
>>>> Open questions:
>>>> ---------------
>>>> 
>>>> - Deprecations/compat?
>>>> 
>>>> I think we should deprecate migrate-set/query-capabilities and everything to do
>>>> with capabilities (specifically the validation in the JSON at the end of the
>>>> stream).
>>>> 
>>>> For migrate-set/query-parameters, we could probably keep it around indefinitely,
>>>> but it'd be convenient to introduce new commands so we can give them new
>>>> semantics.
>>>> 
>>>> - How to restrict the options that should not be set when the migration is in
>>>> progress?
>>>> 
>>>> i.e.:
>>>>   all options can be set before migration (initial config)
>>>>   some options can be set during migration (runtime)
>>>> 
>>>> I thought of adding another type at the top of the hierarchy, with
>>>> just the options allowed to change at runtime, but that doesn't really
>>>> stop the others being also set at runtime. I'd need a way to have a
>>>> set of options that are rejected 'if migration_is_running()', without
>>>> adding more duplication all around.
>>>> 
>>>> - What about savevm?
>>>> 
>>>> None of this solves the issue of random caps/params being set before
>>>> calling savevm. We still need to special-case savevm and reject
>>>> everything. Unless we entirely deprecate setting initial options via
>>>> set-parameters (or set-config) and require all options to be set as
>>>> savevm (and migrate) arguments.
>>>
>>> I'd suggest we aim for a world where the commands take all options
>>> as direct args and try to remove the global state eventually.
>>
>> Yes.
>>
>> Even better: make it a job.
>
> What do we gain from that in relation to being able to pass ~50
> parameters to a command? I don't see it. I think that's the actual crux
> here, too many options and how to get them from the QAPI into the
> migration core for consumption.

Two separate interface design problems:

1. Query and manipulate complex configuration

2. Control and monitor long-running jobs

Let's start with 1.

Migration is not the only instance of complex configuration.  Block
devices are arguably even more complex.  A difference: we can have many
block devices, but just one migration.

Here's the usual way to configure things: commands creating a thing take
the thing's configuration as arguments, other commands query and
manipulate a thing's configuration.  Works fine both for simple and
complex configuration.  Block devices work this way.

Migration doesn't.  Instead, we have global configuration state, and
commands to query and manipulate it.  Also works.

Instead of passing the entire configuration to the "create" command, you
can pass it piecemeal to "manipulate global configuration" commands.
You pay for the convenience by having to reset the entire global
configuration to a known state in certain situations.  Since use of the
global configuration state is implicit, it can be surprising, as Daniel
pointed out for savevm.

Overall, this isn't simpler or more convenient than the usual way, just
different.

Both ways use QAPI objects to hold configuration.  Query commands can
return such an object.  Commands that deal with partial configuration
can only take the same object when its members are optional.

In places where configuration is complete (internal configuration state,
the query command that returns it), optional makes no sense, and is
annoying.  We either accept that, or duplicate the configuration object.
Duplicating violates DRY.  It does result in better command
documentation, though.

On to 2.

Migration is not the only kind of long-running job.  There's also
block-commit, block-stream, drive-mirror, drive-backup, blockdev-create,
x-blockdev-amend, snapshot-load, snapshot-save, snapshot-delete.

These all create a "job" object that represents the long-running job.
We have commands and events to control and monitor such jobs: job-pause,
job-resume, job-cancel, job-complete, job-dismiss, job-finalize,
query-jobs, JOB_STATUS_CHANGE.

Migration does its own thing instead.

> The current usage of MigrationParameters as both the return type for
> query-set-parameters and the global parameter store for the migration
> state is really dissonant. What do the has_* fields even mean when
> accessed via MigrationState::parameters?

Yes, it's ugly, and the alternative is differently ugly, as discussed
above.

>                                          This series is not doing any
> better in that regard, mind you. I'm almost tempted to ask that we
> expose the QDict from the marshaling function directly to the migration
> code, at least that's a data type that makes sense internally.

Based on experience in the block layer, I predict you'd regret this :)

Use of native C data types is just so much easier on the eyes than
messing with QObjects.  Moreover, the compiler can't help you as much
with QObjects.

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Apr 11, 2025 at 04:14:30PM -0300, Fabiano Rosas wrote:
>> Open questions:
>> ---------------
>> 
>> - Deprecations/compat?
>> 
>> I think we should deprecate migrate-set/query-capabilities and everything to do
>> with capabilities (specifically the validation in the JSON at the end of the
>> stream).
>> 
>> For migrate-set/query-parameters, we could probably keep it around indefinitely,
>> but it'd be convenient to introduce new commands so we can give them new
>> semantics.
>> 
>> - How to restrict the options that should not be set when the migration is in
>> progress?
>> 
>> i.e.:
>>   all options can be set before migration (initial config)
>>   some options can be set during migration (runtime)
>> 
>> I thought of adding another type at the top of the hierarchy, with
>> just the options allowed to change at runtime, but that doesn't really
>> stop the others being also set at runtime. I'd need a way to have a
>> set of options that are rejected 'if migration_is_running()', without
>> adding more duplication all around.
>> 
>> - What about savevm?
>> 
>> None of this solves the issue of random caps/params being set before
>> calling savevm. We still need to special-case savevm and reject
>> everything. Unless we entirely deprecate setting initial options via
>> set-parameters (or set-config) and require all options to be set as
>> savevm (and migrate) arguments.
>
> I'd suggest we aim for a world where the commands take all options
> as direct args and try to remove the global state eventually.
>

Well, except the options that are adjusted during migration. But yes, I
agree. It all depends on how we proceed with keeping the old commands
around and for how long. If they're still around we can't stop people
from using them and later invoking "savevm" for instance.

> For savevm/loadvm in particular it is very much a foot-gun that
> 'migrate-set-*' will affect them, because savevm/loadvm aren't
> obviously connected to 'migrate-*' commands unless you're aware
> of how QEMU implements savevm internally.
>

Yes, I could perhaps reset all options once savevm is called, maybe that
would be acceptable, then we don't need to check and block every single
one. Once we add support to migration options to savevm, then they'd be
set in the savevm command-line from day 1 and those wouldn't be
reset. We could also keep HMP restricted to savevm without any migration
options. That's be easy to enforce. If the user wants fancy savevm, they
can invoke via QMP.

>> - HMP?
>> 
>> Can we convert the strings passed via hmp_set_parameters without
>> having an enum of parameters? Duplication problem again.
>
>> 
>> - incoming defer?
>> 
>> It seems we cannot do the final step of removing
>> migrate-set-capabilites before we have a form of handshake
>> implemented. That would take the config from qmp_migrate on source and
>> send it to the destination for negotiation.
>
> I'm not sure I understand why the QAPI design changes are tied
> to the new protocol handshake ? I guess you're wanting to avoid
> updating 'migrate_incoming' to accept the new parameters directly ?
>

Yes, without migrate-set-capabilities, we'd need to pass an enormous
command line to -incoming defer to be able to enable capabilities on the
destination. With the handshake, we could transfer them over the wire
somehow. Does that make sense?

>
> With regards,
> Daniel

On Mon, Apr 14, 2025 at 02:12:35PM -0300, Fabiano Rosas wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Fri, Apr 11, 2025 at 04:14:30PM -0300, Fabiano Rosas wrote:
> >> Open questions:
> >> ---------------
> >> 
> >> - Deprecations/compat?
> >> 
> >> I think we should deprecate migrate-set/query-capabilities and everything to do
> >> with capabilities (specifically the validation in the JSON at the end of the
> >> stream).
> >> 
> >> For migrate-set/query-parameters, we could probably keep it around indefinitely,
> >> but it'd be convenient to introduce new commands so we can give them new
> >> semantics.
> >> 
> >> - How to restrict the options that should not be set when the migration is in
> >> progress?
> >> 
> >> i.e.:
> >>   all options can be set before migration (initial config)
> >>   some options can be set during migration (runtime)
> >> 
> >> I thought of adding another type at the top of the hierarchy, with
> >> just the options allowed to change at runtime, but that doesn't really
> >> stop the others being also set at runtime. I'd need a way to have a
> >> set of options that are rejected 'if migration_is_running()', without
> >> adding more duplication all around.
> >> 
> >> - What about savevm?
> >> 
> >> None of this solves the issue of random caps/params being set before
> >> calling savevm. We still need to special-case savevm and reject
> >> everything. Unless we entirely deprecate setting initial options via
> >> set-parameters (or set-config) and require all options to be set as
> >> savevm (and migrate) arguments.
> >
> > I'd suggest we aim for a world where the commands take all options
> > as direct args and try to remove the global state eventually.
> >
> 
> Well, except the options that are adjusted during migration. But yes, I
> agree. It all depends on how we proceed with keeping the old commands
> around and for how long. If they're still around we can't stop people
> from using them and later invoking "savevm" for instance.
> 
> > For savevm/loadvm in particular it is very much a foot-gun that
> > 'migrate-set-*' will affect them, because savevm/loadvm aren't
> > obviously connected to 'migrate-*' commands unless you're aware
> > of how QEMU implements savevm internally.
> >
> 
> Yes, I could perhaps reset all options once savevm is called, maybe that
> would be acceptable, then we don't need to check and block every single
> one. Once we add support to migration options to savevm, then they'd be
> set in the savevm command-line from day 1 and those wouldn't be
> reset. We could also keep HMP restricted to savevm without any migration
> options. That's be easy to enforce. If the user wants fancy savevm, they
> can invoke via QMP.

Can we make the two approaches mutually exclusive ? Taking your
'migrate' command example addition:

  { 'command': 'migrate',
    'data': {'*uri': 'str',
             '*channels': [ 'MigrationChannel' ],
  +          '*config': 'MigrationConfig',
             '*detach': 'bool', '*resume': 'bool' } }

if 'migrate' is invoked with the '*config' data being non-nil,
then we should ignore *all* global state previously set with
migrate-set-XXXX, and exclusively use '*config'.

That gives a clean semantic break between old and new approaches,
without us having to worry about removing the existing commands
quickly.


> >> - incoming defer?
> >> 
> >> It seems we cannot do the final step of removing
> >> migrate-set-capabilites before we have a form of handshake
> >> implemented. That would take the config from qmp_migrate on source and
> >> send it to the destination for negotiation.
> >
> > I'm not sure I understand why the QAPI design changes are tied
> > to the new protocol handshake ? I guess you're wanting to avoid
> > updating 'migrate_incoming' to accept the new parameters directly ?
> >
> 
> Yes, without migrate-set-capabilities, we'd need to pass an enormous
> command line to -incoming defer to be able to enable capabilities on the
> destination. With the handshake, we could transfer them over the wire
> somehow. Does that make sense?

'-incoming defer' still gets paired with 'migrate-incoming' on the
target, so no matter what, there's no reason to ever pass parameters
on the CLI with '-incoming defer'.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Mon, Apr 14, 2025 at 02:12:35PM -0300, Fabiano Rosas wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Fri, Apr 11, 2025 at 04:14:30PM -0300, Fabiano Rosas wrote:
>> >> Open questions:
>> >> ---------------
>> >> 
>> >> - Deprecations/compat?
>> >> 
>> >> I think we should deprecate migrate-set/query-capabilities and everything to do
>> >> with capabilities (specifically the validation in the JSON at the end of the
>> >> stream).
>> >> 
>> >> For migrate-set/query-parameters, we could probably keep it around indefinitely,
>> >> but it'd be convenient to introduce new commands so we can give them new
>> >> semantics.
>> >> 
>> >> - How to restrict the options that should not be set when the migration is in
>> >> progress?
>> >> 
>> >> i.e.:
>> >>   all options can be set before migration (initial config)
>> >>   some options can be set during migration (runtime)
>> >> 
>> >> I thought of adding another type at the top of the hierarchy, with
>> >> just the options allowed to change at runtime, but that doesn't really
>> >> stop the others being also set at runtime. I'd need a way to have a
>> >> set of options that are rejected 'if migration_is_running()', without
>> >> adding more duplication all around.
>> >> 
>> >> - What about savevm?
>> >> 
>> >> None of this solves the issue of random caps/params being set before
>> >> calling savevm. We still need to special-case savevm and reject
>> >> everything. Unless we entirely deprecate setting initial options via
>> >> set-parameters (or set-config) and require all options to be set as
>> >> savevm (and migrate) arguments.
>> >
>> > I'd suggest we aim for a world where the commands take all options
>> > as direct args and try to remove the global state eventually.
>> >
>> 
>> Well, except the options that are adjusted during migration. But yes, I
>> agree. It all depends on how we proceed with keeping the old commands
>> around and for how long. If they're still around we can't stop people
>> from using them and later invoking "savevm" for instance.
>> 
>> > For savevm/loadvm in particular it is very much a foot-gun that
>> > 'migrate-set-*' will affect them, because savevm/loadvm aren't
>> > obviously connected to 'migrate-*' commands unless you're aware
>> > of how QEMU implements savevm internally.
>> >
>> 
>> Yes, I could perhaps reset all options once savevm is called, maybe that
>> would be acceptable, then we don't need to check and block every single
>> one. Once we add support to migration options to savevm, then they'd be
>> set in the savevm command-line from day 1 and those wouldn't be
>> reset. We could also keep HMP restricted to savevm without any migration
>> options. That's be easy to enforce. If the user wants fancy savevm, they
>> can invoke via QMP.
>
> Can we make the two approaches mutually exclusive ? Taking your
> 'migrate' command example addition:
>
>   { 'command': 'migrate',
>     'data': {'*uri': 'str',
>              '*channels': [ 'MigrationChannel' ],
>   +          '*config': 'MigrationConfig',
>              '*detach': 'bool', '*resume': 'bool' } }
>
> if 'migrate' is invoked with the '*config' data being non-nil,
> then we should ignore *all* global state previously set with
> migrate-set-XXXX, and exclusively use '*config'.
>
> That gives a clean semantic break between old and new approaches,
> without us having to worry about removing the existing commands
> quickly.
>

Good idea. I will need to do something about the -global options because
they also set the defaults for the various options. But we should be
able to decouple setting defaults from -global. Or I could just apply
-global again on top of what came in '*config'.

>
>> >> - incoming defer?
>> >> 
>> >> It seems we cannot do the final step of removing
>> >> migrate-set-capabilites before we have a form of handshake
>> >> implemented. That would take the config from qmp_migrate on source and
>> >> send it to the destination for negotiation.
>> >
>> > I'm not sure I understand why the QAPI design changes are tied
>> > to the new protocol handshake ? I guess you're wanting to avoid
>> > updating 'migrate_incoming' to accept the new parameters directly ?
>> >
>> 
>> Yes, without migrate-set-capabilities, we'd need to pass an enormous
>> command line to -incoming defer to be able to enable capabilities on the
>> destination. With the handshake, we could transfer them over the wire
>> somehow. Does that make sense?
>
> '-incoming defer' still gets paired with 'migrate-incoming' on the
> target, so no matter what, there's no reason to ever pass parameters
> on the CLI with '-incoming defer'.
>

Oops, I misread the strcmp in vl.c. I mean -incoming uri is the one
that'll need a huge cmdline.

But if we follow your suggestion above we could just tie -incoming URI
to the existing commands and make the new format require defer.

>
> With regards,
> Daniel

On Mon, Apr 14, 2025 at 02:40:25PM -0300, Fabiano Rosas wrote:
> > Can we make the two approaches mutually exclusive ? Taking your
> > 'migrate' command example addition:
> >
> >   { 'command': 'migrate',
> >     'data': {'*uri': 'str',
> >              '*channels': [ 'MigrationChannel' ],
> >   +          '*config': 'MigrationConfig',
> >              '*detach': 'bool', '*resume': 'bool' } }
> >
> > if 'migrate' is invoked with the '*config' data being non-nil,
> > then we should ignore *all* global state previously set with
> > migrate-set-XXXX, and exclusively use '*config'.
> >
> > That gives a clean semantic break between old and new approaches,
> > without us having to worry about removing the existing commands
> > quickly.
> >
> 
> Good idea. I will need to do something about the -global options because
> they also set the defaults for the various options. But we should be
> able to decouple setting defaults from -global. Or I could just apply
> -global again on top of what came in '*config'.

Would it still be possible that we allow whatever options attached to the
"migrate" command to only overwrite the globals (either set via -global or
migrate-set-* QMP commands), rather than ignoring them completely?

If so, we don't need to do anything with current -global and it'll keep
working.  It's the same to most existing way to set the migration options
(e.g., otherwise do we plan to disable HMP "migrate" usage?).

Making the above '*config' to only overwrite also do not stand against the
mgmt using it to ignore the default options, after all the mgmt can decide
to always set everything in the '*config' then it's the same as ignoring
the globals?

-- 
Peter Xu

On Mon, Apr 14, 2025 at 02:40:25PM -0300, Fabiano Rosas wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Mon, Apr 14, 2025 at 02:12:35PM -0300, Fabiano Rosas wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > On Fri, Apr 11, 2025 at 04:14:30PM -0300, Fabiano Rosas wrote:
> >> >> Open questions:
> >> >> ---------------
> >> >> 
> >> >> - Deprecations/compat?
> >> >> 
> >> >> I think we should deprecate migrate-set/query-capabilities and everything to do
> >> >> with capabilities (specifically the validation in the JSON at the end of the
> >> >> stream).
> >> >> 
> >> >> For migrate-set/query-parameters, we could probably keep it around indefinitely,
> >> >> but it'd be convenient to introduce new commands so we can give them new
> >> >> semantics.
> >> >> 
> >> >> - How to restrict the options that should not be set when the migration is in
> >> >> progress?
> >> >> 
> >> >> i.e.:
> >> >>   all options can be set before migration (initial config)
> >> >>   some options can be set during migration (runtime)
> >> >> 
> >> >> I thought of adding another type at the top of the hierarchy, with
> >> >> just the options allowed to change at runtime, but that doesn't really
> >> >> stop the others being also set at runtime. I'd need a way to have a
> >> >> set of options that are rejected 'if migration_is_running()', without
> >> >> adding more duplication all around.
> >> >> 
> >> >> - What about savevm?
> >> >> 
> >> >> None of this solves the issue of random caps/params being set before
> >> >> calling savevm. We still need to special-case savevm and reject
> >> >> everything. Unless we entirely deprecate setting initial options via
> >> >> set-parameters (or set-config) and require all options to be set as
> >> >> savevm (and migrate) arguments.
> >> >
> >> > I'd suggest we aim for a world where the commands take all options
> >> > as direct args and try to remove the global state eventually.
> >> >
> >> 
> >> Well, except the options that are adjusted during migration. But yes, I
> >> agree. It all depends on how we proceed with keeping the old commands
> >> around and for how long. If they're still around we can't stop people
> >> from using them and later invoking "savevm" for instance.
> >> 
> >> > For savevm/loadvm in particular it is very much a foot-gun that
> >> > 'migrate-set-*' will affect them, because savevm/loadvm aren't
> >> > obviously connected to 'migrate-*' commands unless you're aware
> >> > of how QEMU implements savevm internally.
> >> >
> >> 
> >> Yes, I could perhaps reset all options once savevm is called, maybe that
> >> would be acceptable, then we don't need to check and block every single
> >> one. Once we add support to migration options to savevm, then they'd be
> >> set in the savevm command-line from day 1 and those wouldn't be
> >> reset. We could also keep HMP restricted to savevm without any migration
> >> options. That's be easy to enforce. If the user wants fancy savevm, they
> >> can invoke via QMP.
> >
> > Can we make the two approaches mutually exclusive ? Taking your
> > 'migrate' command example addition:
> >
> >   { 'command': 'migrate',
> >     'data': {'*uri': 'str',
> >              '*channels': [ 'MigrationChannel' ],
> >   +          '*config': 'MigrationConfig',
> >              '*detach': 'bool', '*resume': 'bool' } }
> >
> > if 'migrate' is invoked with the '*config' data being non-nil,
> > then we should ignore *all* global state previously set with
> > migrate-set-XXXX, and exclusively use '*config'.
> >
> > That gives a clean semantic break between old and new approaches,
> > without us having to worry about removing the existing commands
> > quickly.
> >
> 
> Good idea. I will need to do something about the -global options because
> they also set the defaults for the various options. But we should be
> able to decouple setting defaults from -global. Or I could just apply
> -global again on top of what came in '*config'.
> 
> >
> >> >> - incoming defer?
> >> >> 
> >> >> It seems we cannot do the final step of removing
> >> >> migrate-set-capabilites before we have a form of handshake
> >> >> implemented. That would take the config from qmp_migrate on source and
> >> >> send it to the destination for negotiation.
> >> >
> >> > I'm not sure I understand why the QAPI design changes are tied
> >> > to the new protocol handshake ? I guess you're wanting to avoid
> >> > updating 'migrate_incoming' to accept the new parameters directly ?
> >> >
> >> 
> >> Yes, without migrate-set-capabilities, we'd need to pass an enormous
> >> command line to -incoming defer to be able to enable capabilities on the
> >> destination. With the handshake, we could transfer them over the wire
> >> somehow. Does that make sense?
> >
> > '-incoming defer' still gets paired with 'migrate-incoming' on the
> > target, so no matter what, there's no reason to ever pass parameters
> > on the CLI with '-incoming defer'.
> >
> 
> Oops, I misread the strcmp in vl.c. I mean -incoming uri is the one
> that'll need a huge cmdline.
> 
> But if we follow your suggestion above we could just tie -incoming URI
> to the existing commands and make the new format require defer.

Yes,  modern approach should require 'defer'.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|