Peter Xu <[email protected]> writes:
> On Thu, Dec 18, 2025 at 04:41:46PM -0300, Fabiano Rosas wrote:
>> Peter Xu <[email protected]> writes:
>>
>> > On Mon, Dec 15, 2025 at 07:00:16PM -0300, Fabiano Rosas wrote:
>> >> The tests are being refactored to pass migration options to QEMU using
>> >> the new API of passing a JSON object as argument the migration
>> >> commands instead of using several calls to the
>> >> migrate_set_capabilities|parameters commands.
>> >>
>> >> Since multiple tests share common infrastructure (framework.c,
>> >> migration-utils.c, migration-qmp.c), it's cumbersome to convert tests
>> >> in small chunks, which would require changes to every common function
>> >> to accept both the new and old ways.
>> >>
>> >> After some tinkering, an easier way to do this transition is to allow
>> >> the tests to set a key in the config dict itself telling whether the
>> >> config is supported. With this, the common functions can be fully
>> >> altered to support the config object, as long as they check this
>> >> temporary key and do the right thing.
>> >>
>> >> QEMU doesn't know about this hack, so some code is needed to hide it
>> >> when issuing QMP commands with the config object.
>> >>
>> >> This will all be removed once tests are fully converted.
>> >>
>> >> Signed-off-by: Fabiano Rosas <[email protected]>
>> >> ---
>> >> tests/qtest/migration/migration-qmp.h | 1 -
>> >> tests/qtest/migration/migration-util.c | 1 +
>> >> tests/qtest/migration/migration-util.h | 34 ++++++++++++++++++++++++++
>> >> 3 files changed, 35 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/tests/qtest/migration/migration-qmp.h
>> >> b/tests/qtest/migration/migration-qmp.h
>> >> index 940ffd5950..9a36a677ba 100644
>> >> --- a/tests/qtest/migration/migration-qmp.h
>> >> +++ b/tests/qtest/migration/migration-qmp.h
>> >> @@ -47,5 +47,4 @@ void migrate_recover(QTestState *who, const char *uri);
>> >> void migrate_cancel(QTestState *who);
>> >> void migrate_postcopy_start(QTestState *from, QTestState *to,
>> >> QTestMigrationState *src_state);
>> >> -
>> >> #endif /* MIGRATION_QMP_H */
>> >> diff --git a/tests/qtest/migration/migration-util.c
>> >> b/tests/qtest/migration/migration-util.c
>> >> index 416dd10ef8..e702f00896 100644
>> >> --- a/tests/qtest/migration/migration-util.c
>> >> +++ b/tests/qtest/migration/migration-util.c
>> >> @@ -255,6 +255,7 @@ static void migration_test_wrapper(const void *data)
>> >>
>> >> test->data = g_new0(MigrateCommon, 1);
>> >> test->data->start.config = qdict_new();
>> >> + qdict_put_bool(test->data->start.config, "use-config", false);
>> >>
>> >> g_test_message("Running /%s%s", qtest_get_arch(), test->name);
>> >> test->func(test->name, test->data);
>> >> diff --git a/tests/qtest/migration/migration-util.h
>> >> b/tests/qtest/migration/migration-util.h
>> >> index e73d69bab0..3c3b5a8777 100644
>> >> --- a/tests/qtest/migration/migration-util.h
>> >> +++ b/tests/qtest/migration/migration-util.h
>> >> @@ -60,4 +60,38 @@ void migration_test_add_suffix(const char *path, const
>> >> char *suffix,
>> >> char *migrate_get_connect_uri(QTestState *who);
>> >> void migrate_set_ports(QTestState *to, QList *channel_list);
>> >>
>> >> +/*
>> >> + * Scaffolding to allow the framework _common functions and _qmp
>> >> + * functions to use the config object while some tests are still using
>> >> + * migrate_set_*. Tests that have been converted will set use-config =
>> >> + * true on the config dict.
>> >> + */
>> >> +static bool has_key;
>> >> +static bool use_config;
>> >
>> > Looks like this is temp measure, so no strong opinions.. said that, it
>> > looks tricky to have the two globals shared between all the tests, and
>> > having magic keys in the qdict.
>> >
>>
>> It is tricky, but it works. The other options all require "passing
>> something" in, which ends up touching good code and causing a mess with
>> rebases and the overall clarity of the patches. But let me read about
>> your suggestions below...
>>
>> > Can we pass in MigrateStart* for config_load() and config_put()? Then at
>> > least we can change globals into per-test flags of MigrateStart.
>> >
>> > Btw, AFAIU the two helpers should always used in a pair but load() and
>> > put() do not look like a pair..
>> >
>>
>> My mind went to vcpu_load/vcpu_put from kvm code. =D
>
> Fair enough. :) Personally I'd use load/unload in new codes.
>
>>
>> > If we can have args->use_config as a bool, having tests opt-in config
>> > setups by setting it, then I wonder if we can do that like:
>> >
>>
>> The migrate_qmp commands don't take args. So I'd have to alter their
>> signature just for this temporary state. That's why I put the flag in
>> the dict itself.
>>
>> > if (args->use_config) {
>> > // do whatever with args->config...
>> > } else {
>> > // covered by other migrate-set-parameters QMP commands..
>> > }
>> >
>> > Do we really need config_put()? I'll keep reading, but please evaluate..
>> >
>>
>> Because of the migrate_incoming_qmp and -incoming calls, we need to take
>> the key out of the dict to hide it. Then put it back so the rest of the
>> code, e.g. migrate_qmp can use it.
>
> Can we introduce migrate_qmp_args() wrapper which takes the *args, then
> only pass in config if args->use_config for migrate_qmp()?
>
> I still want to know if we can have better way to do this, so that the
> qdict should only and always be the real configs to be applied. That can
> remove a major confusion I had when reading this series.
>
Ok, I can try harder.
> Another example is, I see that you also reused the qdict keys for storing
> different tls-creds for client/server, which needs tweak as well before
> applying. I wonder if those can be done with config, config_src,
> config_dst, hence whatever passed into migrate_qmp should be
> "config+config_src", whilist "config+config_dst" for migrate_incoming_qmp.
>
Hm, but then we'd have these args->config_src and args->config_dst for
people to misuse. The config reaches migrate_qmp via the _common
functions:
test_foobar(args)
{
args->config.multifd = true;
args->config.tls_creds = "abc";
test_foobar_common(args);
}
test_foobar_common(args)
{
migrate_start(args);
...
migrate_qmp_incoming(args->config);
...
migrate_qmp(args->config);
}
It would be weird:
test_foobar(args)
{
args->config.multifd = true;
args->config_src.tls_creds = "abc";
args->config_dst.tls_creds = "def";
test_foobar_common(args);
}
test_foobar_common(args)
{
migrate_start(args);
...
migrate_qmp_incoming(args->config, args->config_dst);
...
migrate_qmp(args->config, args->config_src);
}
If there's code that needs to check the config for an option, how does
it know which of the three to look at?
> IMHO if no way to work it out, one last request is for all special keys we
> should have somewhere document them explicitly (maybe above the "config"
> var?). We could also make all special keys to be prefixed with "__" (or
> something else) so as to be very clear they are special. We can even
> assert in a config_load() making sure no special keys after loaded.
>
> So in general, if there's way to not introduce special keys I'll consider
> voting for them first.. Said that, please go choose whatever way you
> finally decide. It can take into account of how easy it is to impl the idea
> based on your current version, to not make this series drag you too much.
> Not a big deal, IMHO we can work on top especially with tests.
>
>>
>> >> +static inline QDict *config_load(QDict *config)
>> >> +{
>> >> + if (!config) {
>> >> + return NULL;
>> >> + }
>> >> +
>> >> + has_key = qdict_haskey(config, "use-config");
>> >> + if (has_key) {
>> >> + use_config = qdict_get_try_bool(config, "use-config", false);
>> >> + qdict_del(config, "use-config");
>> >> + }
>> >> +
>> >> + if (use_config) {
>> >> + return config;
>> >> + }
>> >> +
>> >> + return NULL;
>> >> +}
>> >> +
>> >> +static inline void config_put(QDict *config)
>> >> +{
>> >> + if (config && has_key) {
>> >> + qdict_put_bool(config, "use-config", use_config);
>> >> + }
>> >> +}
>> >> +
>> >> #endif /* MIGRATION_UTIL_H */
>> >> --
>> >> 2.51.0
>> >>
>>
