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

[Peter Xu <peterx@redhat.com>]

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