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 <farosas@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);
+
+ if (major > tgt_major) {
+ return -1;
+ }
+ if (major < tgt_major) {
+ return 1;
+ }
+
+ 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;
+ }
+
/*
* Remove shmem file immediately to avoid memory leak in test failed case.
* It's valid because QEMU has already opened this file
--
2.35.3On 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 <farosas@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);
> }
If we provide a macro to encode the version to a single integer:
#define VERSION_NUMBER(maj, min, mic) ((maj * 1000000) + (min * 1000) + mic)
> +
> +int migration_vercmp(QTestState *who, const char *tgt_version)
This could be an "int 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, µ);
This could be nothing more than
return (tgt_version >= VERSION_NUMBER(major, minor, micro));
> +
> + tok = strtok(version, delim);
> + assert(tok);
> + tgt_major = atoi(tok);
> + assert(tgt_major);
> +
> + if (major > tgt_major) {
> + return -1;
> + }
> + if (major < tgt_major) {
> + return 1;
> + }
> +
> + 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;
This could be an 'int' too.
>
> /*
> @@ -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);
Hmm, the g_test_skip stuff is repeated in several places, which we could
avoid if we put it into a helper in libqtest.h
"qtest_require_version(QTestState *test, int version)"
> +
> + 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;
> + }
> +
> /*
> * 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
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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 <farosas@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.
In all cases, document this function with retval would be helpful too.
> + }
> + 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?
> +
> + 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.
> + }
> +
> /*
> * 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
>
--
Peter Xu
Peter Xu <peterx@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 <farosas@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
>>
On Mon, Jan 08, 2024 at 11:49:45AM -0300, Fabiano Rosas wrote:
> >> +
> >> + 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
Here if returns -1, then below [1] will skip the test?
>
> > 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.
To me QEMU_VER(8,2,0) is as easy to write and read, too. What Dan proposed
looks also good in the other thread.
I don't really have a strong opinion here especially for the test case. But
imho it'll be still nice to avoid string <-> int if the string is not required.
[...]
> >> @@ -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) {
[1]
> >> + 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;
> >> + }
--
Peter Xu
Peter Xu <peterx@redhat.com> writes:
> On Mon, Jan 08, 2024 at 11:49:45AM -0300, Fabiano Rosas wrote:
>> >> +
>> >> + 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
>
> Here if returns -1, then below [1] will skip the test?
>
Ah ok, code below is wrong.
>>
>> > 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.
>
> To me QEMU_VER(8,2,0) is as easy to write and read, too. What Dan proposed
> looks also good in the other thread.
>
> I don't really have a strong opinion here especially for the test case. But
> imho it'll be still nice to avoid string <-> int if the string is not required.
Ok, I'll change it to something else.
>
> [...]
>
>> >> @@ -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) {
>
> [1]
>
>> >> + 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;
>> >> + }
On Mon, Jan 08, 2024 at 04:13:10PM +0800, Peter Xu wrote:
> 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 <farosas@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.
>
> In all cases, document this function with retval would be helpful too.
>
> > + }
> > + 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 meant something like:
struct Version { int major, minor, micro; };
#define QEMU_VER(major, minor, micro) \
((struct Verion){.major = major, .minor = minor, .micro = micro})
Then:
...
struct Version since;
...
...
.since = QEMU_VER(8, 2, 0),
...
>
> > +
> > + 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.
>
> > + }
> > +
> > /*
> > * 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
> >
>
> --
> Peter Xu
--
Peter Xu
© 2016 - 2024 Red Hat, Inc.