Fabiano Rosas <faro...@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 <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. >
