Fabiano Rosas <faro...@suse.de> writes: > Daniel P. Berrangé <berra...@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/zvm5xmsae41wj...@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. >>> Add a compatibility check routine and reject the snapshot command if >>> an incompatible capability is enabled. For now only act on the the >>> two that actually cause a crash: multifd and mapped-ram. >>> >>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2818 >> >> Issue is 2881 not 2818 ^^^^^^^ >> > > It seem that was your own C-t =) > >>> Signed-off-by: Fabiano Rosas <faro...@suse.de> >>> --- >>> migration/options.c | 26 ++++++++++++++++++++++++++ >>> migration/options.h | 1 + >>> migration/savevm.c | 8 ++++++++ >>> 3 files changed, 35 insertions(+) >> >> With regards, >> Daniel
