Re: [PATCH 1/4] migration/savevm: Add a compatibility check for capabilities

[Fabiano Rosas <farosas@suse.de>]

Fabiano Rosas <farosas@suse.de> writes:

> Fabiano Rosas <farosas@suse.de> writes:
>
> +Cc Markus
>
> context:
> This series was trying to stop savevm from crashing when arbitrary
> migration capabilities are enabled. Daniel brought up the previous
> discussion around unifying capabilities + parameters and passing it all
> via the migrate (or snapshot in this case) command arguments. I'm
> looking into that now.
>
>> Fabiano Rosas <farosas@suse.de> writes:
>>
>>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>>
>>>> On Thu, Mar 27, 2025 at 11:39:31AM -0300, Fabiano Rosas wrote:
>>>>> It has always been possible to enable arbitrary migration capabilities
>>>>> and attempt to take a snapshot of the VM with the savevm/loadvm
>>>>> commands as well as their QMP counterparts
>>>>> snapshot-save/snapshot-load.
>>>>> 
>>>>> Most migration capabilities are not meant to be used with snapshots
>>>>> and there's a risk of crashing QEMU or producing incorrect
>>>>> behavior. Ideally, every migration capability would either be
>>>>> implemented for savevm or explicitly rejected.
>>>>
>>>> IMHO, this a prime example of why migration config shouldn't be held
>>>> as global state, and instead passed as parameters to the commands
>>>> that need them.  The snapshot-save/load commands would then only
>>>> be able to accept what few settings are actually relevant, instead
>>>> of inheriting any/all global migration state.
>>>>
>>>
>>> Right, I remember we got caught around the fact that some migration
>>> options are needed during runtime as well... but I don't remember the
>>> details, let try to find that thread.
>>>
>>
>> Found it: https://lore.kernel.org/r/ZVM5xmsaE41WJYgb@redhat.com
>>
>> I don't think it's *too* hard to start passing the configuration to
>> qmp_migrate & friends. We just need to figure out a path for the
>> compatibility.
>>
>> I'm thiking of:
>>
>> 1) Unifying capabilities and parameters in a MigrationConfig
>> structure. We take the opportunity to fix the tls options to 'str'
>> instead of StrOrNull.
>>
>> 2) Deprecate migrate-set-capabilities. There are no capabilities
>> anymore.
>>
>> 3) Deprecate migate-set-parameters. There are no parameters
>> anymore. Alternatively, reuse the existing command, but have it take the
>> additional capabilities as optional (everything else is already
>> optional).
>>
>> 4) Depending on what we do on (3), add a new migrate-set-config command
>> that sets every option. All as optional. This would be nice because we
>> wouldn't need to worry about breaking compat on the tls options, we just
>> define the new command in the correct way.
>>
>> 5) Add a {'*config': MigrationConfig} entry to qmp_migrate and
>> migrate_set_{config|parameters}. Here is where I have questions, because
>> ideally we'd have a way to limit the migrate_set_config command to only
>> the options that can be set at runtime. But I can't see a way of doing
>> that without the duplication of the options in the QAPI .json file. I'm
>> inclined to allow the whole set of options and do some tracking on the
>> side in options.c in the migration code.
>>
>> (same issue for savevm really. To allow it to (say) work with
>> mapped-ram, we'd need a duplicate mapped-ram entry in migration.json)
>>
>> About (2) and (3). If we use this unified MigrationConfig, I can keep
>> the old commands working (along with the query_* variants), by defining
>> a compat function that converts from those commands specific format into
>> the new format. But then there's the question of what do we do when a
>> new capability/parameter comes along? Can we declare that the old
>> commands will not see the new data and that's it? If there's no
>> distinction between caps and params anymore, there isn't even a way to
>> decide which command to use.
>>

Hi all, please disregard my messages on this thread. I've posted a
series which has a cover letter that explains the situation better:

[RFC PATCH 00/13] migration: Unify capabilities and parameters
https://lore.kernel.org/r/20250411191443.22565-1-farosas@suse.de