Peter Xu <pet...@redhat.com> writes:
> On Fri, Jan 05, 2024 at 03:04:47PM -0300, Fabiano Rosas wrote:
>> We can run the migration tests with two different QEMU binaries to
>> test migration compatibility between QEMU versions. This means we'll
>> be running the tests with an older QEMU in either source or
>> destination.
>>
>> We need to avoid trying to test functionality that is unknown to the
>> older QEMU. This could mean new features, bug fixes, error message
>> changes, QEMU command line changes, migration API changes, etc.
>>
>> Add a 'since' argument to the tests that inform when the functionality
>> that is being test has been added to QEMU so we can skip the test on
>> older versions.
>>
>> Also add a version comparison function so we can adapt test code
>> depending on the QEMU binary version being used.
>>
>> Signed-off-by: Fabiano Rosas <faro...@suse.de>
>> ---
>> tests/qtest/migration-helpers.h | 1 +
>> tests/qtest/migration-helpers.c | 48 +++++++++++++++++++++++++++++++++
>> tests/qtest/migration-test.c | 29 ++++++++++++++++++++
>> 3 files changed, 78 insertions(+)
>>
>> diff --git a/tests/qtest/migration-helpers.h
>> b/tests/qtest/migration-helpers.h
>> index e31dc85cc7..7b4f8e851e 100644
>> --- a/tests/qtest/migration-helpers.h
>> +++ b/tests/qtest/migration-helpers.h
>> @@ -47,4 +47,5 @@ char *find_common_machine_version(const char *mtype, const
>> char *var1,
>> const char *var2);
>> char *resolve_machine_version(const char *alias, const char *var1,
>> const char *var2);
>> +int migration_vercmp(QTestState *who, const char *tgt_version);
>> #endif /* MIGRATION_HELPERS_H */
>> diff --git a/tests/qtest/migration-helpers.c
>> b/tests/qtest/migration-helpers.c
>> index 24fb7b3525..bc4fd93e8c 100644
>> --- a/tests/qtest/migration-helpers.c
>> +++ b/tests/qtest/migration-helpers.c
>> @@ -292,3 +292,51 @@ char *resolve_machine_version(const char *alias, const
>> char *var1,
>>
>> return find_common_machine_version(machine_name, var1, var2);
>> }
>> +
>> +int migration_vercmp(QTestState *who, const char *tgt_version)
>> +{
>> + g_autofree char *version = g_strdup(tgt_version);
>> + int major = 0, minor = 0, micro = 0;
>> + int tgt_major = 0, tgt_minor = 0, tgt_micro = 0;
>> + const char *delim = ".";
>> + char *tok;
>> +
>> + qtest_query_version(who, &major, &minor, µ);
>> +
>> + tok = strtok(version, delim);
>> + assert(tok);
>> + tgt_major = atoi(tok);
>> + assert(tgt_major);
>
> I'd consider dropping this. But I don't think "since: 0.8" is valid. :)
> More like a nitpick.
>
>> +
>> + if (major > tgt_major) {
>> + return -1;
>
> This means the QEMU version is newer, the function will return negative.
> Is this what we want? It seems it's inverted.
The return "points" to which once is the more recent:
QEMU version | since: version
-1 0 1
> In all cases, document this function with retval would be helpful too.
>
Ok.
>> + }
>> + if (major < tgt_major) {
>> + return 1;
>> + }
>
> Instead of all these, I'm wondering whether we can allow "since" to be an
> array of integers, like [8, 2, 0]. Would that be much easier?
I don't see why push the complexity towards the person writing the
tests. The string is much more natural to specify.
>> +
>> + tok = strtok(NULL, delim);
>> + assert(tok);
>> + tgt_minor = atoi(tok);
>> +
>> + if (minor > tgt_minor) {
>> + return -1;
>> + }
>> + if (minor < tgt_minor) {
>> + return 1;
>> + }
>> +
>> + tok = strtok(NULL, delim);
>> + if (tok) {
>> + tgt_micro = atoi(tok);
>> + }
>> +
>> + if (micro > tgt_micro) {
>> + return -1;
>> + }
>> + if (micro < tgt_micro) {
>> + return 1;
>> + }
>> +
>> + return 0;
>> +}
>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> index d520c587f7..001470238b 100644
>> --- a/tests/qtest/migration-test.c
>> +++ b/tests/qtest/migration-test.c
>> @@ -637,6 +637,12 @@ typedef struct {
>> bool use_dirty_ring;
>> const char *opts_source;
>> const char *opts_target;
>> + /*
>> + * If a test checks against new functionality that might not be
>> + * present in older QEMUs this needs to be set so we can skip
>> + * running it when doing compatibility testing.
>> + */
>> + const char *since;
>> } MigrateStart;
>>
>> /*
>> @@ -850,6 +856,17 @@ static int test_migrate_start(QTestState **from,
>> QTestState **to,
>> qtest_qmp_set_event_callback(*from,
>> migrate_watch_for_stop,
>> &got_src_stop);
>> +
>> + if (args->since && migration_vercmp(*from, args->since) < 0) {
>> + g_autofree char *msg = NULL;
>> +
>> + msg = g_strdup_printf("Test requires at least QEMU version %s",
>> + args->since);
>> + g_test_skip(msg);
>> + qtest_quit(*from);
>> +
>> + return -1;
>> + }
>> }
>>
>> cmd_target = g_strdup_printf("-accel kvm%s -accel tcg "
>> @@ -872,6 +889,18 @@ static int test_migrate_start(QTestState **from,
>> QTestState **to,
>> migrate_watch_for_resume,
>> &got_dst_resume);
>>
>> + if (args->since && migration_vercmp(*to, args->since) < 0) {
>> + g_autofree char *msg = NULL;
>> +
>> + msg = g_strdup_printf("Test requires at least QEMU version %s",
>> + args->since);
>> + g_test_skip(msg);
>> + qtest_quit(*to);
>> + qtest_quit(*from);
>> +
>> + return -1;
>
> Nitpick: you can do both check here, then avoid duplicating some code, and
> always free from+to.
>
Hm.. I think I had a good reason for doing it this way. I don't remember
now. I'll double check and apply your suggestion if it works.
>> + }
>> +
>> /*
>> * Remove shmem file immediately to avoid memory leak in test failed
>> case.
>> * It's valid because QEMU has already opened this file
>> --
>> 2.35.3
>>
