Re: [Qemu-devel] [PATCH 2/4] tests/pxe-test: Use table of testcases rather than open-coding

[David Gibson]

On Fri, Dec 15, 2017 at 12:08:13PM +0100, Thomas Huth wrote:
> On 15.12.2017 11:16, David Gibson wrote:
> > Currently pxe-tests open codes the list of tests for each architecture.
> > This changes it to use tables of test parameters, somewhat similar to
> > boot-serial-test.
> > 
> > This adds the machine type into the table as well, giving us the ability
> > to perform tests on multiple machine types for architectures where there's
> > more than one machine type that matters.
> > 
> > NOTE: This changes the names of the tests in the output, to include the
> >       machine type and IPv4 vs. IPv6.  I'm not sure if this has the
> >       potential to break existing tooling.
> > 
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> >  tests/pxe-test.c | 94 
> > +++++++++++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 72 insertions(+), 22 deletions(-)
> > 
> > diff --git a/tests/pxe-test.c b/tests/pxe-test.c
> > index eb70aa2bc6..f9bca8976d 100644
> > --- a/tests/pxe-test.c
> > +++ b/tests/pxe-test.c
> > @@ -22,29 +22,86 @@
> >  
> >  static char disk[] = "tests/pxe-test-disk-XXXXXX";
> >  
> > -static void test_pxe_one(const char *params, bool ipv6)
> > +typedef struct testdef {
> > +    const char *machine;    /* Machine type */
> > +    const char *model;      /* NIC device model (human readable)*/
> > +    const char *extra;      /* Extra parameters, overriding default */
> > +} testdef_t;
> 
> Not sure whether it's nicer, but you could also add a "is_slow" field to
> the struct and then merge the fast and slow tables below...?

I had a draft version like that.  I thought the separate tables was a
bit nicer than having a bunch of true/false entries in the table that
you have to look elsewhere to interpret.

> > +static testdef_t x86_tests[] = {
> > +    { "pc", "e1000" },
> > +    { "pc", "virtio", "-device virtio-net-pci,netdev=" NETNAME },
> 
> I think I'd rather rather use "virtio-net-pci" as model name here and
> drop the "extra" parameter.

I guesss.  Originally I was trying to maintain the test names, but now
that I've ended up changing them for other reasons, I guess that
doesn't matter.

> > +    { NULL },
> > +};
> > +
> > +static testdef_t x86_tests_slow[] = {
> > +    { "pc", "ne2000", "-device ne2k_pci,netdev=" NETNAME },
> > +    { "pc", "eepro100", "-device i82550,netdev=" NETNAME },
> 
> dito.
> 
> > +    { "pc", "rtl8139" },
> > +    { "pc", "vmxnet3" },
> > +    { NULL },
> > +};
> > +
> > +static testdef_t ppc64_tests[] = {
> > +    { "pseries", "spapr-vlan" },
> > +    { NULL },
> > +};
> > +
> > +static testdef_t ppc64_tests_slow[] = {
> > +    { "pseries", "virtio", "-device virtio-net-pci,netdev=" NETNAME },
> 
> dito.
> 
> > +    { "pseries", "e1000" },
> > +    { NULL },
> > +};
> > +
> > +static testdef_t s390x_tests[] = {
> > +    { "s390-ccw-virtio", "virtio-ccw",
> > +      "-device virtio-net-ccw,bootindex=1,netdev=" NETNAME },
> > +    { NULL },
> > +};
> > +
> > +static void test_pxe_one(const testdef_t *test, bool ipv6)
> >  {
> >      char *args;
> > +    char *defextra;
> > +    const char *extra = test->extra;
> > +
> > +    defextra = g_strdup_printf("-device %s,netdev=" NETNAME, test->model);
> > +    if (!extra) {
> > +        extra = defextra;
> > +    }
> 
> I'd be nicer to do the g_strdup_printf() only if it is really necessary,
> e.g.:

Well, I was trying to avoid having conditionals at the end as well as
the beginning.  But I just realised g_free(NULL) is safe, so that can
still be done.  I'll update.

>     const char *extra = test->extra;
> 
>     if (!extra) {
>         extra = g_strdup_printf(...);
>     }
> 
> > -    args = g_strdup_printf("-machine accel=kvm:tcg -nodefaults -boot 
> > order=n "
> > -                           "-netdev user,id=" NETNAME 
> > ",tftp=./,bootfile=%s,"
> > -                           "ipv4=%s,ipv6=%s %s", disk, ipv6 ? "off" : "on",
> > -                           ipv6 ? "on" : "off", params);
> > +    args = g_strdup_printf(
> > +        "-machine %s,accel=kvm:tcg -nodefaults -boot order=n "
> > +        "-netdev user,id=" NETNAME ",tftp=./,bootfile=%s,ipv4=%s,ipv6=%s 
> > %s",
> > +        test->machine, disk, ipv6 ? "off" : "on", ipv6 ? "on" : "off", 
> > extra);
> >  
> >      qtest_start(args);
> >      boot_sector_test();
> >      qtest_quit(global_qtest);
> >      g_free(args);
> > +    g_free(defextra);
> 
> ... and then:
> 
>     if (!test->extra) {
>         g_free(extra);
>     }
> 
> >  }
> >  
> >  static void test_pxe_ipv4(gconstpointer data)
> >  {
> > -    const char *model = data;
> > -    char *dev_arg;
> > +    const testdef_t *test = data;
> > +
> > +    test_pxe_one(test, false);
> > +}
> >  
> > -    dev_arg = g_strdup_printf("-device %s,netdev=" NETNAME, model);
> > -    test_pxe_one(dev_arg, false);
> > -    g_free(dev_arg);
> > +static void test_batch(const testdef_t *tests)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; tests[i].machine; i++) {
> > +        const testdef_t *test = &tests[i];
> > +        char *testname;
> > +
> > +        testname = g_strdup_printf("pxe/ipv4/%s/%s",
> > +                                   test->machine, test->model);
> > +        qtest_add_data_func(testname, test, test_pxe_ipv4);
> > +        g_free(testname);
> > +    }
> >  }
> >  
> >  int main(int argc, char *argv[])
> > @@ -59,24 +116,17 @@ int main(int argc, char *argv[])
> >      g_test_init(&argc, &argv, NULL);
> >  
> >      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> > -        qtest_add_data_func("pxe/e1000", "e1000", test_pxe_ipv4);
> > -        qtest_add_data_func("pxe/virtio", "virtio-net-pci", test_pxe_ipv4);
> > +        test_batch(x86_tests);
> >          if (g_test_slow()) {
> > -            qtest_add_data_func("pxe/ne2000", "ne2k_pci", test_pxe_ipv4);
> > -            qtest_add_data_func("pxe/eepro100", "i82550", test_pxe_ipv4);
> > -            qtest_add_data_func("pxe/pcnet", "pcnet", test_pxe_ipv4);
> > -            qtest_add_data_func("pxe/rtl8139", "rtl8139", test_pxe_ipv4);
> > -            qtest_add_data_func("pxe/vmxnet3", "vmxnet3", test_pxe_ipv4);
> > +            test_batch(x86_tests_slow);
> >          }
> >      } else if (strcmp(arch, "ppc64") == 0) {
> > -        qtest_add_data_func("pxe/spapr-vlan", "spapr-vlan", test_pxe_ipv4);
> > +        test_batch(ppc64_tests);
> >          if (g_test_slow()) {
> > -            qtest_add_data_func("pxe/virtio", "virtio-net-pci", 
> > test_pxe_ipv4);
> > -            qtest_add_data_func("pxe/e1000", "e1000", test_pxe_ipv4);
> > +            test_batch(ppc64_tests_slow);
> >          }
> >      } else if (g_str_equal(arch, "s390x")) {
> > -        qtest_add_data_func("pxe/virtio-ccw",
> > -                            "virtio-net-ccw,bootindex=1", test_pxe_ipv4);
> > +        test_batch(s390x_tests);
> >      }
> >      ret = g_test_run();
> >      boot_sector_cleanup(disk);
> > 
> 
>  Thomas
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature