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 <da...@gibson.dropbear.id.au>
> ---
> 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...?
> +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.
> + { 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.:
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
