Re: [PATCH v3 30/51] tests/qtest/migration: Add temporary code to toggle usage of config

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> 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 <farosas@suse.de>
>> ---
>>  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

> 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.

>> +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
>>