Re: [PATCH v3 2/4] tests/qtest/migration: Add infrastructure to skip tests on older QEMUs

[Daniel P . Berrangé]

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);
>  }

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, &micro);


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