[Qemu-devel] [PATCH 0/2] Sort tests by architecture and add a test for serial output

[Qemu-devel] [PATCH 0/2] Sort tests by architecture and add a test for serial output

[Thomas Huth]

Since some of the machines that we support in QEMU sometimes get
broken by other changes, and it then takes a while 'till somebody
notices the breakage, it would be nice to get some more automatic
test coverage for various machines during "make check". The
second patch adds such a test for the machines where we've got
a firmware image for and thus can test for some magic strings
in the serial output of the firmware.

The first patch is just a clean-up for the Makefile to avoid that
new tests get added with "=" instead of "+=" (and thus completely
overwrite the correspond variable by accident).

Thomas Huth (2):
  tests: Resort check-qtest entries in Makefile.include
  tests: Check serial output of firmware boot of some machines

 tests/Makefile.include   |  40 +++++++++++------
 tests/boot-serial-test.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 138 insertions(+), 12 deletions(-)
 create mode 100644 tests/boot-serial-test.c

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] tests: Resort check-qtest entries in Makefile.include

[Thomas Huth]

The rather random list of check-qtest-xxx entries caused some
confusion in the past, where to use "=" and where to use "+="
(see commits 0ccac16f59462b8e2b9afbc1 and 1f5c1cfbaec0792cd2e5da
for example).
Sorting the check-qtest-xxx entries by architecure instead and
using some empty lines inbetween should help to ease this
situation a little bit, so that it is hopefully now obvious
that new tests should be added with "+=" instead of "=".

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/Makefile.include | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 2010b11..b7784d3 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -240,32 +240,40 @@ check-qtest-i386-y += tests/postcopy-test$(EXESUF)
 check-qtest-x86_64-y += $(check-qtest-i386-y)
 gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
 gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
+
 check-qtest-mips-y = tests/endianness-test$(EXESUF)
 check-qtest-mips64-y = tests/endianness-test$(EXESUF)
 check-qtest-mips64el-y = tests/endianness-test$(EXESUF)
+
 check-qtest-ppc-y = tests/endianness-test$(EXESUF)
+check-qtest-ppc-y += tests/boot-order-test$(EXESUF)
+check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
+
 check-qtest-ppc64-y = tests/endianness-test$(EXESUF)
+check-qtest-ppc64-y += tests/boot-order-test$(EXESUF)
+check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF)
+gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c
+check-qtest-ppc64-y += tests/prom-env-test$(EXESUF)
+
 check-qtest-sh4-y = tests/endianness-test$(EXESUF)
 check-qtest-sh4eb-y = tests/endianness-test$(EXESUF)
+
+check-qtest-sparc-y = tests/prom-env-test$(EXESUF)
+#check-qtest-sparc-y += tests/m48t59-test$(EXESUF)
+#gcov-files-sparc-y = hw/timer/m48t59.c
+
 check-qtest-sparc64-y = tests/endianness-test$(EXESUF)
-#check-qtest-sparc-y = tests/m48t59-test$(EXESUF)
 #check-qtest-sparc64-y += tests/m48t59-test$(EXESUF)
-gcov-files-sparc-y += hw/timer/m48t59.c
-gcov-files-sparc64-y += hw/timer/m48t59.c
+#gcov-files-sparc64-y += hw/timer/m48t59.c
+#Disabled for now, triggers a TCG bug on 32-bit hosts
+#check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
+
 check-qtest-arm-y = tests/tmp105-test$(EXESUF)
 check-qtest-arm-y += tests/ds1338-test$(EXESUF)
 gcov-files-arm-y += hw/misc/tmp105.c
 check-qtest-arm-y += tests/virtio-blk-test$(EXESUF)
 gcov-files-arm-y += arm-softmmu/hw/block/virtio-blk.c
-check-qtest-ppc-y += tests/boot-order-test$(EXESUF)
-check-qtest-ppc64-y += tests/boot-order-test$(EXESUF)
-check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF)
-gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c
-check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
-check-qtest-ppc64-y += tests/prom-env-test$(EXESUF)
-check-qtest-sparc-y += tests/prom-env-test$(EXESUF)
-#Disabled for now, triggers a TCG bug on 32-bit hosts
-#check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
+
 check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
 check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] tests: Check serial output of firmware boot of some machines

[Thomas Huth]

Some of the machines that we have got a firmware image for write
some output to the serial console while booting up. We can use
this output to make sure that the machine is basically working,
so this adds a test that checks the output of these machines
for some well-known "magic" strings.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/Makefile.include   |   8 ++++
 tests/boot-serial-test.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 118 insertions(+)
 create mode 100644 tests/boot-serial-test.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index b7784d3..ba1cc8d 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -194,6 +194,7 @@ check-qtest-i386-y += tests/hd-geo-test$(EXESUF)
 gcov-files-i386-y += hw/block/hd-geometry.c
 check-qtest-i386-y += tests/boot-order-test$(EXESUF)
 check-qtest-i386-y += tests/bios-tables-test$(EXESUF)
+check-qtest-i386-y += tests/boot-serial-test$(EXESUF)
 check-qtest-i386-y += tests/pxe-test$(EXESUF)
 check-qtest-i386-y += tests/rtc-test$(EXESUF)
 check-qtest-i386-y += tests/ipmi-kcs-test$(EXESUF)
@@ -241,6 +242,8 @@ check-qtest-x86_64-y += $(check-qtest-i386-y)
 gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
 gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
 
+check-qtest-alpha-y = tests/boot-serial-test$(EXESUF)
+
 check-qtest-mips-y = tests/endianness-test$(EXESUF)
 check-qtest-mips64-y = tests/endianness-test$(EXESUF)
 check-qtest-mips64el-y = tests/endianness-test$(EXESUF)
@@ -248,12 +251,14 @@ check-qtest-mips64el-y = tests/endianness-test$(EXESUF)
 check-qtest-ppc-y = tests/endianness-test$(EXESUF)
 check-qtest-ppc-y += tests/boot-order-test$(EXESUF)
 check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
+check-qtest-ppc-y += tests/boot-serial-test$(EXESUF)
 
 check-qtest-ppc64-y = tests/endianness-test$(EXESUF)
 check-qtest-ppc64-y += tests/boot-order-test$(EXESUF)
 check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF)
 gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c
 check-qtest-ppc64-y += tests/prom-env-test$(EXESUF)
+check-qtest-ppc64-y += tests/boot-serial-test$(EXESUF)
 
 check-qtest-sh4-y = tests/endianness-test$(EXESUF)
 check-qtest-sh4eb-y = tests/endianness-test$(EXESUF)
@@ -277,6 +282,8 @@ gcov-files-arm-y += arm-softmmu/hw/block/virtio-blk.c
 check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
 check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
 
+check-qtest-s390x-y = tests/boot-serial-test$(EXESUF)
+
 check-qtest-generic-y += tests/qom-test$(EXESUF)
 
 qapi-schema += alternate-any.json
@@ -575,6 +582,7 @@ tests/ipmi-kcs-test$(EXESUF): tests/ipmi-kcs-test.o
 tests/ipmi-bt-test$(EXESUF): tests/ipmi-bt-test.o
 tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o
 tests/boot-order-test$(EXESUF): tests/boot-order-test.o $(libqos-obj-y)
+tests/boot-serial-test$(EXESUF): tests/boot-serial-test.o $(libqos-obj-y)
 tests/bios-tables-test$(EXESUF): tests/bios-tables-test.o \
 	tests/boot-sector.o $(libqos-obj-y)
 tests/pxe-test$(EXESUF): tests/pxe-test.o tests/boot-sector.o $(libqos-obj-y)
diff --git a/tests/boot-serial-test.c b/tests/boot-serial-test.c
new file mode 100644
index 0000000..3263dcd
--- /dev/null
+++ b/tests/boot-serial-test.c
@@ -0,0 +1,110 @@
+/*
+ * Test serial output of some machines.
+ *
+ * Copyright 2016 Thomas Huth, Red Hat Inc.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2
+ * or later. See the COPYING file in the top-level directory.
+ *
+ * This test is used to check that the serial output of the firmware
+ * (that we provide for some machines) contains an expected string.
+ * Thus we check that the firmware still boots at least to a certain
+ * point and so we know that the machine is not completely broken.
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+
+typedef struct testdef {
+    const char *arch;       /* Target architecture */
+    const char *machine;    /* Name of the machine */
+    const char *extra;      /* Additional parameters */
+    const char *expect;     /* Expected string in the serial output */
+} testdef_t;
+
+static testdef_t tests[] = {
+    { "alpha", "clipper", "", "PCI:" },
+    { "ppc", "ppce500", "", "U-Boot" },
+    { "ppc", "prep", "", "Open Hack'Ware BIOS" },
+    { "ppc64", "ppce500", "", "U-Boot" },
+    { "ppc64", "prep", "", "Open Hack'Ware BIOS" },
+    { "ppc64", "pseries", "", "Open Firmware" },
+    { "i386", "isapc", "-device sga", "SGABIOS" },
+    { "i386", "pc", "-device sga", "SGABIOS" },
+    { "i386", "q35", "-device sga", "SGABIOS" },
+    { "x86_64", "isapc", "-device sga", "SGABIOS" },
+    { "x86_64", "pc", "-device sga", "SGABIOS" },
+    { "x86_64", "q35", "-device sga", "SGABIOS" },
+    { "s390x", "s390-ccw-virtio",
+      "-nodefaults -device sclpconsole,chardev=serial0", "virtio device" },
+    { NULL }
+};
+
+static void check_guest_output(const testdef_t *test, int fd)
+{
+    bool output_ok = false;
+    int i, pos = 0;
+    char ch;
+
+    /* Poll serial output... Wait at most 30 seconds */
+    for (i = 0; i < 3000; ++i) {
+        while (read(fd, &ch, 1) == 1) {
+            if (ch == test->expect[pos]) {
+                pos += 1;
+                if (test->expect[pos] == '\0') {
+                    /* We've reached the end of the expected string! */
+                    output_ok = true;
+                    goto done;
+                }
+            } else {
+                pos = 0;
+            }
+        }
+        g_usleep(10000);
+    }
+
+done:
+    g_assert(output_ok);
+}
+
+static void test_machine(const void *data)
+{
+    const testdef_t *test = data;
+    char *args;
+    char tmpname[] = "/tmp/qtest-boot-serial-XXXXXX";
+    int fd;
+
+    fd = mkstemp(tmpname);
+    g_assert(fd != -1);
+
+    args = g_strdup_printf("-M %s,accel=tcg -chardev file,id=serial0,path=%s"
+                           " -serial chardev:serial0 %s", test->machine,
+                           tmpname, test->extra);
+
+    qtest_start(args);
+    unlink(tmpname);
+
+    check_guest_output(test, fd);
+    qtest_quit(global_qtest);
+
+    g_free(args);
+    close(fd);
+}
+
+int main(int argc, char *argv[])
+{
+    const char *arch = qtest_get_arch();
+    int i;
+
+    g_test_init(&argc, &argv, NULL);
+
+    for (i = 0; tests[i].arch != NULL; i++) {
+        if (strcmp(arch, tests[i].arch) == 0) {
+            char *name = g_strdup_printf("boot-serial/%s", tests[i].machine);
+            qtest_add_data_func(name, &tests[i], test_machine);
+            g_free(name);
+        }
+    }
+
+    return g_test_run();
+}
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/2] tests: Resort check-qtest entries in Makefile.include

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4065 bytes --]

On Thu, Jul 14, 2016 at 11:57:45AM +0200, Thomas Huth wrote:
> The rather random list of check-qtest-xxx entries caused some
> confusion in the past, where to use "=" and where to use "+="
> (see commits 0ccac16f59462b8e2b9afbc1 and 1f5c1cfbaec0792cd2e5da
> for example).
> Sorting the check-qtest-xxx entries by architecure instead and
> using some empty lines inbetween should help to ease this
> situation a little bit, so that it is hopefully now obvious
> that new tests should be added with "+=" instead of "=".
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/Makefile.include | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 2010b11..b7784d3 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -240,32 +240,40 @@ check-qtest-i386-y += tests/postcopy-test$(EXESUF)
>  check-qtest-x86_64-y += $(check-qtest-i386-y)
>  gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
>  gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
> +

Hm, seems to me like there's still potential for confusion in the
mixing of check-qtest lines and gcov-files lines.

>  check-qtest-mips-y = tests/endianness-test$(EXESUF)
>  check-qtest-mips64-y = tests/endianness-test$(EXESUF)
>  check-qtest-mips64el-y = tests/endianness-test$(EXESUF)
> +
>  check-qtest-ppc-y = tests/endianness-test$(EXESUF)
> +check-qtest-ppc-y += tests/boot-order-test$(EXESUF)
> +check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
> +
>  check-qtest-ppc64-y = tests/endianness-test$(EXESUF)
> +check-qtest-ppc64-y += tests/boot-order-test$(EXESUF)
> +check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF)
> +gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c

For example, AFAICT this is the first line setting gcov-files-ppc64-y,
so shouldn't it be '=' instead of '+='?

> +check-qtest-ppc64-y += tests/prom-env-test$(EXESUF)
> +
>  check-qtest-sh4-y = tests/endianness-test$(EXESUF)
>  check-qtest-sh4eb-y = tests/endianness-test$(EXESUF)

Should be another blank line between sh4 and sh4eb, no?

> +
> +check-qtest-sparc-y = tests/prom-env-test$(EXESUF)
> +#check-qtest-sparc-y += tests/m48t59-test$(EXESUF)
> +#gcov-files-sparc-y = hw/timer/m48t59.c

You've added a comment here, haven't you?  Which changes something
real, not just order.

> +
>  check-qtest-sparc64-y = tests/endianness-test$(EXESUF)
> -#check-qtest-sparc-y = tests/m48t59-test$(EXESUF)
>  #check-qtest-sparc64-y += tests/m48t59-test$(EXESUF)
> -gcov-files-sparc-y += hw/timer/m48t59.c
> -gcov-files-sparc64-y += hw/timer/m48t59.c
> +#gcov-files-sparc64-y += hw/timer/m48t59.c

And here?

> +#Disabled for now, triggers a TCG bug on 32-bit hosts
> +#check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
> +
>  check-qtest-arm-y = tests/tmp105-test$(EXESUF)
>  check-qtest-arm-y += tests/ds1338-test$(EXESUF)
>  gcov-files-arm-y += hw/misc/tmp105.c
>  check-qtest-arm-y += tests/virtio-blk-test$(EXESUF)
>  gcov-files-arm-y += arm-softmmu/hw/block/virtio-blk.c
> -check-qtest-ppc-y += tests/boot-order-test$(EXESUF)
> -check-qtest-ppc64-y += tests/boot-order-test$(EXESUF)
> -check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF)
> -gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c
> -check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
> -check-qtest-ppc64-y += tests/prom-env-test$(EXESUF)
> -check-qtest-sparc-y += tests/prom-env-test$(EXESUF)
> -#Disabled for now, triggers a TCG bug on 32-bit hosts
> -#check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
> +
>  check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
>  check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)

And there should be a blank betweem microblazeel and xtensaeb too, no?

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 2/2] tests: Check serial output of firmware boot of some machines

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 7627 bytes --]

On Thu, Jul 14, 2016 at 11:57:46AM +0200, Thomas Huth wrote:
> Some of the machines that we have got a firmware image for write
> some output to the serial console while booting up. We can use
> this output to make sure that the machine is basically working,
> so this adds a test that checks the output of these machines
> for some well-known "magic" strings.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

I love this idea.  A couple of queries about the implemntation.

> ---
>  tests/Makefile.include   |   8 ++++
>  tests/boot-serial-test.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 118 insertions(+)
>  create mode 100644 tests/boot-serial-test.c
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index b7784d3..ba1cc8d 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -194,6 +194,7 @@ check-qtest-i386-y += tests/hd-geo-test$(EXESUF)
>  gcov-files-i386-y += hw/block/hd-geometry.c
>  check-qtest-i386-y += tests/boot-order-test$(EXESUF)
>  check-qtest-i386-y += tests/bios-tables-test$(EXESUF)
> +check-qtest-i386-y += tests/boot-serial-test$(EXESUF)
>  check-qtest-i386-y += tests/pxe-test$(EXESUF)
>  check-qtest-i386-y += tests/rtc-test$(EXESUF)
>  check-qtest-i386-y += tests/ipmi-kcs-test$(EXESUF)
> @@ -241,6 +242,8 @@ check-qtest-x86_64-y += $(check-qtest-i386-y)
>  gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
>  gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
>  
> +check-qtest-alpha-y = tests/boot-serial-test$(EXESUF)
> +
>  check-qtest-mips-y = tests/endianness-test$(EXESUF)
>  check-qtest-mips64-y = tests/endianness-test$(EXESUF)
>  check-qtest-mips64el-y = tests/endianness-test$(EXESUF)
> @@ -248,12 +251,14 @@ check-qtest-mips64el-y = tests/endianness-test$(EXESUF)
>  check-qtest-ppc-y = tests/endianness-test$(EXESUF)
>  check-qtest-ppc-y += tests/boot-order-test$(EXESUF)
>  check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
> +check-qtest-ppc-y += tests/boot-serial-test$(EXESUF)
>  
>  check-qtest-ppc64-y = tests/endianness-test$(EXESUF)
>  check-qtest-ppc64-y += tests/boot-order-test$(EXESUF)
>  check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF)
>  gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c
>  check-qtest-ppc64-y += tests/prom-env-test$(EXESUF)
> +check-qtest-ppc64-y += tests/boot-serial-test$(EXESUF)
>  
>  check-qtest-sh4-y = tests/endianness-test$(EXESUF)
>  check-qtest-sh4eb-y = tests/endianness-test$(EXESUF)
> @@ -277,6 +282,8 @@ gcov-files-arm-y += arm-softmmu/hw/block/virtio-blk.c
>  check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
>  check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
>  
> +check-qtest-s390x-y = tests/boot-serial-test$(EXESUF)
> +
>  check-qtest-generic-y += tests/qom-test$(EXESUF)
>  
>  qapi-schema += alternate-any.json
> @@ -575,6 +582,7 @@ tests/ipmi-kcs-test$(EXESUF): tests/ipmi-kcs-test.o
>  tests/ipmi-bt-test$(EXESUF): tests/ipmi-bt-test.o
>  tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o
>  tests/boot-order-test$(EXESUF): tests/boot-order-test.o $(libqos-obj-y)
> +tests/boot-serial-test$(EXESUF): tests/boot-serial-test.o $(libqos-obj-y)
>  tests/bios-tables-test$(EXESUF): tests/bios-tables-test.o \
>  	tests/boot-sector.o $(libqos-obj-y)
>  tests/pxe-test$(EXESUF): tests/pxe-test.o tests/boot-sector.o $(libqos-obj-y)
> diff --git a/tests/boot-serial-test.c b/tests/boot-serial-test.c
> new file mode 100644
> index 0000000..3263dcd
> --- /dev/null
> +++ b/tests/boot-serial-test.c
> @@ -0,0 +1,110 @@
> +/*
> + * Test serial output of some machines.
> + *
> + * Copyright 2016 Thomas Huth, Red Hat Inc.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2
> + * or later. See the COPYING file in the top-level directory.
> + *
> + * This test is used to check that the serial output of the firmware
> + * (that we provide for some machines) contains an expected string.
> + * Thus we check that the firmware still boots at least to a certain
> + * point and so we know that the machine is not completely broken.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +
> +typedef struct testdef {
> +    const char *arch;       /* Target architecture */
> +    const char *machine;    /* Name of the machine */
> +    const char *extra;      /* Additional parameters */
> +    const char *expect;     /* Expected string in the serial output */
> +} testdef_t;
> +
> +static testdef_t tests[] = {
> +    { "alpha", "clipper", "", "PCI:" },
> +    { "ppc", "ppce500", "", "U-Boot" },
> +    { "ppc", "prep", "", "Open Hack'Ware BIOS" },
> +    { "ppc64", "ppce500", "", "U-Boot" },
> +    { "ppc64", "prep", "", "Open Hack'Ware BIOS" },
> +    { "ppc64", "pseries", "", "Open Firmware" },
> +    { "i386", "isapc", "-device sga", "SGABIOS" },
> +    { "i386", "pc", "-device sga", "SGABIOS" },
> +    { "i386", "q35", "-device sga", "SGABIOS" },
> +    { "x86_64", "isapc", "-device sga", "SGABIOS" },
> +    { "x86_64", "pc", "-device sga", "SGABIOS" },
> +    { "x86_64", "q35", "-device sga", "SGABIOS" },
> +    { "s390x", "s390-ccw-virtio",
> +      "-nodefaults -device sclpconsole,chardev=serial0", "virtio device" },
> +    { NULL }
> +};
> +
> +static void check_guest_output(const testdef_t *test, int fd)
> +{
> +    bool output_ok = false;
> +    int i, pos = 0;
> +    char ch;
> +
> +    /* Poll serial output... Wait at most 30 seconds */
> +    for (i = 0; i < 3000; ++i) {
> +        while (read(fd, &ch, 1) == 1) {

It might be a good idea to check for actual errors from read() here.

> +            if (ch == test->expect[pos]) {
> +                pos += 1;
> +                if (test->expect[pos] == '\0') {
> +                    /* We've reached the end of the expected string! */
> +                    output_ok = true;
> +                    goto done;
> +                }
> +            } else {
> +                pos = 0;
> +            }
> +        }
> +        g_usleep(10000);
> +    }
> +
> +done:
> +    g_assert(output_ok);
> +}
> +
> +static void test_machine(const void *data)
> +{
> +    const testdef_t *test = data;
> +    char *args;
> +    char tmpname[] = "/tmp/qtest-boot-serial-XXXXXX";
> +    int fd;
> +
> +    fd = mkstemp(tmpname);
> +    g_assert(fd != -1);
> +
> +    args = g_strdup_printf("-M %s,accel=tcg -chardev file,id=serial0,path=%s"
> +                           " -serial chardev:serial0 %s", test->machine,
> +                           tmpname, test->extra);
> +
> +    qtest_start(args);
> +    unlink(tmpname);
> +
> +    check_guest_output(test, fd);
> +    qtest_quit(global_qtest);
> +
> +    g_free(args);
> +    close(fd);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +    const char *arch = qtest_get_arch();
> +    int i;
> +
> +    g_test_init(&argc, &argv, NULL);
> +
> +    for (i = 0; tests[i].arch != NULL; i++) {
> +        if (strcmp(arch, tests[i].arch) == 0) {
> +            char *name = g_strdup_printf("boot-serial/%s", tests[i].machine);
> +            qtest_add_data_func(name, &tests[i], test_machine);

Do we need some conditionals in case certain machine types are
configured out?

> +            g_free(name);
> +        }
> +    }
> +
> +    return g_test_run();
> +}

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 1/2] tests: Resort check-qtest entries in Makefile.include

[Thomas Huth]

[-- Attachment #1: Type: text/plain, Size: 4614 bytes --]

On 15.07.2016 05:12, David Gibson wrote:
> On Thu, Jul 14, 2016 at 11:57:45AM +0200, Thomas Huth wrote:
>> The rather random list of check-qtest-xxx entries caused some
>> confusion in the past, where to use "=" and where to use "+="
>> (see commits 0ccac16f59462b8e2b9afbc1 and 1f5c1cfbaec0792cd2e5da
>> for example).
>> Sorting the check-qtest-xxx entries by architecure instead and
>> using some empty lines inbetween should help to ease this
>> situation a little bit, so that it is hopefully now obvious
>> that new tests should be added with "+=" instead of "=".
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  tests/Makefile.include | 32 ++++++++++++++++++++------------
>>  1 file changed, 20 insertions(+), 12 deletions(-)
>>
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index 2010b11..b7784d3 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -240,32 +240,40 @@ check-qtest-i386-y += tests/postcopy-test$(EXESUF)
>>  check-qtest-x86_64-y += $(check-qtest-i386-y)
>>  gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
>>  gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
>> +
> 
> Hm, seems to me like there's still potential for confusion in the
> mixing of check-qtest lines and gcov-files lines.
> 
>>  check-qtest-mips-y = tests/endianness-test$(EXESUF)
>>  check-qtest-mips64-y = tests/endianness-test$(EXESUF)
>>  check-qtest-mips64el-y = tests/endianness-test$(EXESUF)
>> +
>>  check-qtest-ppc-y = tests/endianness-test$(EXESUF)
>> +check-qtest-ppc-y += tests/boot-order-test$(EXESUF)
>> +check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
>> +
>>  check-qtest-ppc64-y = tests/endianness-test$(EXESUF)
>> +check-qtest-ppc64-y += tests/boot-order-test$(EXESUF)
>> +check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF)
>> +gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c
> 
> For example, AFAICT this is the first line setting gcov-files-ppc64-y,
> so shouldn't it be '=' instead of '+='?

True. It currently doesn't hurt because the variable is empty initially,
but it's likely cleaner to start with "=" here, too, for the first
entry... I'll change it in v2.

>> +check-qtest-ppc64-y += tests/prom-env-test$(EXESUF)
>> +
>>  check-qtest-sh4-y = tests/endianness-test$(EXESUF)
>>  check-qtest-sh4eb-y = tests/endianness-test$(EXESUF)
> 
> Should be another blank line between sh4 and sh4eb, no?

I didn't mind it here yet since there is currently only one test for
these architectures, but yes, it's likely cleaner to also add a space
here right from the start, too.

>> +
>> +check-qtest-sparc-y = tests/prom-env-test$(EXESUF)
>> +#check-qtest-sparc-y += tests/m48t59-test$(EXESUF)
>> +#gcov-files-sparc-y = hw/timer/m48t59.c
> 
> You've added a comment here, haven't you?  Which changes something
> real, not just order.

Yeah, I've commented it out since the test is disabled, ... I guess I
should either leave it uncommented or write a proper sentence about this
in the commit description...

>> +
>>  check-qtest-sparc64-y = tests/endianness-test$(EXESUF)
>> -#check-qtest-sparc-y = tests/m48t59-test$(EXESUF)
>>  #check-qtest-sparc64-y += tests/m48t59-test$(EXESUF)
>> -gcov-files-sparc-y += hw/timer/m48t59.c
>> -gcov-files-sparc64-y += hw/timer/m48t59.c
>> +#gcov-files-sparc64-y += hw/timer/m48t59.c
> 
> And here?
> 
>> +#Disabled for now, triggers a TCG bug on 32-bit hosts
>> +#check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
>> +
>>  check-qtest-arm-y = tests/tmp105-test$(EXESUF)
>>  check-qtest-arm-y += tests/ds1338-test$(EXESUF)
>>  gcov-files-arm-y += hw/misc/tmp105.c
>>  check-qtest-arm-y += tests/virtio-blk-test$(EXESUF)
>>  gcov-files-arm-y += arm-softmmu/hw/block/virtio-blk.c
>> -check-qtest-ppc-y += tests/boot-order-test$(EXESUF)
>> -check-qtest-ppc64-y += tests/boot-order-test$(EXESUF)
>> -check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF)
>> -gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c
>> -check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
>> -check-qtest-ppc64-y += tests/prom-env-test$(EXESUF)
>> -check-qtest-sparc-y += tests/prom-env-test$(EXESUF)
>> -#Disabled for now, triggers a TCG bug on 32-bit hosts
>> -#check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
>> +
>>  check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
>>  check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
> 
> And there should be a blank betweem microblazeel and xtensaeb too, no?

Yes, I'll change in v2.

Thanks for the review!

 Thomas




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH 1/2] tests: Resort check-qtest entries in Makefile.include

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5475 bytes --]

On Fri, Jul 15, 2016 at 08:46:47AM +0200, Thomas Huth wrote:
> On 15.07.2016 05:12, David Gibson wrote:
> > On Thu, Jul 14, 2016 at 11:57:45AM +0200, Thomas Huth wrote:
> >> The rather random list of check-qtest-xxx entries caused some
> >> confusion in the past, where to use "=" and where to use "+="
> >> (see commits 0ccac16f59462b8e2b9afbc1 and 1f5c1cfbaec0792cd2e5da
> >> for example).
> >> Sorting the check-qtest-xxx entries by architecure instead and
> >> using some empty lines inbetween should help to ease this
> >> situation a little bit, so that it is hopefully now obvious
> >> that new tests should be added with "+=" instead of "=".
> >>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >>  tests/Makefile.include | 32 ++++++++++++++++++++------------
> >>  1 file changed, 20 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/tests/Makefile.include b/tests/Makefile.include
> >> index 2010b11..b7784d3 100644
> >> --- a/tests/Makefile.include
> >> +++ b/tests/Makefile.include
> >> @@ -240,32 +240,40 @@ check-qtest-i386-y += tests/postcopy-test$(EXESUF)
> >>  check-qtest-x86_64-y += $(check-qtest-i386-y)
> >>  gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
> >>  gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
> >> +
> > 
> > Hm, seems to me like there's still potential for confusion in the
> > mixing of check-qtest lines and gcov-files lines.
> > 
> >>  check-qtest-mips-y = tests/endianness-test$(EXESUF)
> >>  check-qtest-mips64-y = tests/endianness-test$(EXESUF)
> >>  check-qtest-mips64el-y = tests/endianness-test$(EXESUF)
> >> +
> >>  check-qtest-ppc-y = tests/endianness-test$(EXESUF)
> >> +check-qtest-ppc-y += tests/boot-order-test$(EXESUF)
> >> +check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
> >> +
> >>  check-qtest-ppc64-y = tests/endianness-test$(EXESUF)
> >> +check-qtest-ppc64-y += tests/boot-order-test$(EXESUF)
> >> +check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF)
> >> +gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c
> > 
> > For example, AFAICT this is the first line setting gcov-files-ppc64-y,
> > so shouldn't it be '=' instead of '+='?
> 
> True. It currently doesn't hurt because the variable is empty initially,
> but it's likely cleaner to start with "=" here, too, for the first
> entry... I'll change it in v2.

Hm, given that += on a previously undeclared variable treats it as
empty, it seems like it would be safer to just use += everywhere.  I
don't believe there are any cases where we're trying to remove tests

> 
> >> +check-qtest-ppc64-y += tests/prom-env-test$(EXESUF)
> >> +
> >>  check-qtest-sh4-y = tests/endianness-test$(EXESUF)
> >>  check-qtest-sh4eb-y = tests/endianness-test$(EXESUF)
> > 
> > Should be another blank line between sh4 and sh4eb, no?
> 
> I didn't mind it here yet since there is currently only one test for
> these architectures, but yes, it's likely cleaner to also add a space
> here right from the start, too.
> 
> >> +
> >> +check-qtest-sparc-y = tests/prom-env-test$(EXESUF)
> >> +#check-qtest-sparc-y += tests/m48t59-test$(EXESUF)
> >> +#gcov-files-sparc-y = hw/timer/m48t59.c
> > 
> > You've added a comment here, haven't you?  Which changes something
> > real, not just order.
> 
> Yeah, I've commented it out since the test is disabled, ... I guess I
> should either leave it uncommented or write a proper sentence about this
> in the commit description...

Ah.. yes, I see.  I hadn't connected the coverage to the fact the
corresponding test is disabled.  Yeah, I think that needs a mention in
the description.

> 
> >> +
> >>  check-qtest-sparc64-y = tests/endianness-test$(EXESUF)
> >> -#check-qtest-sparc-y = tests/m48t59-test$(EXESUF)
> >>  #check-qtest-sparc64-y += tests/m48t59-test$(EXESUF)
> >> -gcov-files-sparc-y += hw/timer/m48t59.c
> >> -gcov-files-sparc64-y += hw/timer/m48t59.c
> >> +#gcov-files-sparc64-y += hw/timer/m48t59.c
> > 
> > And here?
> > 
> >> +#Disabled for now, triggers a TCG bug on 32-bit hosts
> >> +#check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
> >> +
> >>  check-qtest-arm-y = tests/tmp105-test$(EXESUF)
> >>  check-qtest-arm-y += tests/ds1338-test$(EXESUF)
> >>  gcov-files-arm-y += hw/misc/tmp105.c
> >>  check-qtest-arm-y += tests/virtio-blk-test$(EXESUF)
> >>  gcov-files-arm-y += arm-softmmu/hw/block/virtio-blk.c
> >> -check-qtest-ppc-y += tests/boot-order-test$(EXESUF)
> >> -check-qtest-ppc64-y += tests/boot-order-test$(EXESUF)
> >> -check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF)
> >> -gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c
> >> -check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
> >> -check-qtest-ppc64-y += tests/prom-env-test$(EXESUF)
> >> -check-qtest-sparc-y += tests/prom-env-test$(EXESUF)
> >> -#Disabled for now, triggers a TCG bug on 32-bit hosts
> >> -#check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
> >> +
> >>  check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
> >>  check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
> > 
> > And there should be a blank betweem microblazeel and xtensaeb too, no?
> 
> Yes, I'll change in v2.
> 
> Thanks for the review!
> 
>  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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 2/2] tests: Check serial output of firmware boot of some machines

[Thomas Huth]

[-- Attachment #1: Type: text/plain, Size: 7907 bytes --]

On 15.07.2016 05:21, David Gibson wrote:
> On Thu, Jul 14, 2016 at 11:57:46AM +0200, Thomas Huth wrote:
>> Some of the machines that we have got a firmware image for write
>> some output to the serial console while booting up. We can use
>> this output to make sure that the machine is basically working,
>> so this adds a test that checks the output of these machines
>> for some well-known "magic" strings.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> I love this idea.  A couple of queries about the implemntation.
> 
>> ---
>>  tests/Makefile.include   |   8 ++++
>>  tests/boot-serial-test.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 118 insertions(+)
>>  create mode 100644 tests/boot-serial-test.c
>>
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index b7784d3..ba1cc8d 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -194,6 +194,7 @@ check-qtest-i386-y += tests/hd-geo-test$(EXESUF)
>>  gcov-files-i386-y += hw/block/hd-geometry.c
>>  check-qtest-i386-y += tests/boot-order-test$(EXESUF)
>>  check-qtest-i386-y += tests/bios-tables-test$(EXESUF)
>> +check-qtest-i386-y += tests/boot-serial-test$(EXESUF)
>>  check-qtest-i386-y += tests/pxe-test$(EXESUF)
>>  check-qtest-i386-y += tests/rtc-test$(EXESUF)
>>  check-qtest-i386-y += tests/ipmi-kcs-test$(EXESUF)
>> @@ -241,6 +242,8 @@ check-qtest-x86_64-y += $(check-qtest-i386-y)
>>  gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
>>  gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
>>  
>> +check-qtest-alpha-y = tests/boot-serial-test$(EXESUF)
>> +
>>  check-qtest-mips-y = tests/endianness-test$(EXESUF)
>>  check-qtest-mips64-y = tests/endianness-test$(EXESUF)
>>  check-qtest-mips64el-y = tests/endianness-test$(EXESUF)
>> @@ -248,12 +251,14 @@ check-qtest-mips64el-y = tests/endianness-test$(EXESUF)
>>  check-qtest-ppc-y = tests/endianness-test$(EXESUF)
>>  check-qtest-ppc-y += tests/boot-order-test$(EXESUF)
>>  check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
>> +check-qtest-ppc-y += tests/boot-serial-test$(EXESUF)
>>  
>>  check-qtest-ppc64-y = tests/endianness-test$(EXESUF)
>>  check-qtest-ppc64-y += tests/boot-order-test$(EXESUF)
>>  check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF)
>>  gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c
>>  check-qtest-ppc64-y += tests/prom-env-test$(EXESUF)
>> +check-qtest-ppc64-y += tests/boot-serial-test$(EXESUF)
>>  
>>  check-qtest-sh4-y = tests/endianness-test$(EXESUF)
>>  check-qtest-sh4eb-y = tests/endianness-test$(EXESUF)
>> @@ -277,6 +282,8 @@ gcov-files-arm-y += arm-softmmu/hw/block/virtio-blk.c
>>  check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
>>  check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
>>  
>> +check-qtest-s390x-y = tests/boot-serial-test$(EXESUF)
>> +
>>  check-qtest-generic-y += tests/qom-test$(EXESUF)
>>  
>>  qapi-schema += alternate-any.json
>> @@ -575,6 +582,7 @@ tests/ipmi-kcs-test$(EXESUF): tests/ipmi-kcs-test.o
>>  tests/ipmi-bt-test$(EXESUF): tests/ipmi-bt-test.o
>>  tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o
>>  tests/boot-order-test$(EXESUF): tests/boot-order-test.o $(libqos-obj-y)
>> +tests/boot-serial-test$(EXESUF): tests/boot-serial-test.o $(libqos-obj-y)
>>  tests/bios-tables-test$(EXESUF): tests/bios-tables-test.o \
>>  	tests/boot-sector.o $(libqos-obj-y)
>>  tests/pxe-test$(EXESUF): tests/pxe-test.o tests/boot-sector.o $(libqos-obj-y)
>> diff --git a/tests/boot-serial-test.c b/tests/boot-serial-test.c
>> new file mode 100644
>> index 0000000..3263dcd
>> --- /dev/null
>> +++ b/tests/boot-serial-test.c
>> @@ -0,0 +1,110 @@
>> +/*
>> + * Test serial output of some machines.
>> + *
>> + * Copyright 2016 Thomas Huth, Red Hat Inc.
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2
>> + * or later. See the COPYING file in the top-level directory.
>> + *
>> + * This test is used to check that the serial output of the firmware
>> + * (that we provide for some machines) contains an expected string.
>> + * Thus we check that the firmware still boots at least to a certain
>> + * point and so we know that the machine is not completely broken.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "libqtest.h"
>> +
>> +typedef struct testdef {
>> +    const char *arch;       /* Target architecture */
>> +    const char *machine;    /* Name of the machine */
>> +    const char *extra;      /* Additional parameters */
>> +    const char *expect;     /* Expected string in the serial output */
>> +} testdef_t;
>> +
>> +static testdef_t tests[] = {
>> +    { "alpha", "clipper", "", "PCI:" },
>> +    { "ppc", "ppce500", "", "U-Boot" },
>> +    { "ppc", "prep", "", "Open Hack'Ware BIOS" },
>> +    { "ppc64", "ppce500", "", "U-Boot" },
>> +    { "ppc64", "prep", "", "Open Hack'Ware BIOS" },
>> +    { "ppc64", "pseries", "", "Open Firmware" },
>> +    { "i386", "isapc", "-device sga", "SGABIOS" },
>> +    { "i386", "pc", "-device sga", "SGABIOS" },
>> +    { "i386", "q35", "-device sga", "SGABIOS" },
>> +    { "x86_64", "isapc", "-device sga", "SGABIOS" },
>> +    { "x86_64", "pc", "-device sga", "SGABIOS" },
>> +    { "x86_64", "q35", "-device sga", "SGABIOS" },
>> +    { "s390x", "s390-ccw-virtio",
>> +      "-nodefaults -device sclpconsole,chardev=serial0", "virtio device" },
>> +    { NULL }
>> +};
>> +
>> +static void check_guest_output(const testdef_t *test, int fd)
>> +{
>> +    bool output_ok = false;
>> +    int i, pos = 0;
>> +    char ch;
>> +
>> +    /* Poll serial output... Wait at most 30 seconds */
>> +    for (i = 0; i < 3000; ++i) {
>> +        while (read(fd, &ch, 1) == 1) {
> 
> It might be a good idea to check for actual errors from read() here.

Ok.

>> +            if (ch == test->expect[pos]) {
>> +                pos += 1;
>> +                if (test->expect[pos] == '\0') {
>> +                    /* We've reached the end of the expected string! */
>> +                    output_ok = true;
>> +                    goto done;
>> +                }
>> +            } else {
>> +                pos = 0;
>> +            }
>> +        }
>> +        g_usleep(10000);
>> +    }
>> +
>> +done:
>> +    g_assert(output_ok);
>> +}
>> +
>> +static void test_machine(const void *data)
>> +{
>> +    const testdef_t *test = data;
>> +    char *args;
>> +    char tmpname[] = "/tmp/qtest-boot-serial-XXXXXX";
>> +    int fd;
>> +
>> +    fd = mkstemp(tmpname);
>> +    g_assert(fd != -1);
>> +
>> +    args = g_strdup_printf("-M %s,accel=tcg -chardev file,id=serial0,path=%s"
>> +                           " -serial chardev:serial0 %s", test->machine,
>> +                           tmpname, test->extra);
>> +
>> +    qtest_start(args);
>> +    unlink(tmpname);
>> +
>> +    check_guest_output(test, fd);
>> +    qtest_quit(global_qtest);
>> +
>> +    g_free(args);
>> +    close(fd);
>> +}
>> +
>> +int main(int argc, char *argv[])
>> +{
>> +    const char *arch = qtest_get_arch();
>> +    int i;
>> +
>> +    g_test_init(&argc, &argv, NULL);
>> +
>> +    for (i = 0; tests[i].arch != NULL; i++) {
>> +        if (strcmp(arch, tests[i].arch) == 0) {
>> +            char *name = g_strdup_printf("boot-serial/%s", tests[i].machine);
>> +            qtest_add_data_func(name, &tests[i], test_machine);
> 
> Do we need some conditionals in case certain machine types are
> configured out?

You mean something like using "#ifdef CONFIG_PREP" ? That unfortunately
does not work since it's not exported as a pre-processor definition,
it's only a Makefile variable. So other tests like boot-order-test.c can
also not check for the available machine types and assume that they are
simply always there.

 Thomas




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH 2/2] tests: Check serial output of firmware boot of some machines

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 8708 bytes --]

On Fri, Jul 15, 2016 at 09:25:16AM +0200, Thomas Huth wrote:
> On 15.07.2016 05:21, David Gibson wrote:
> > On Thu, Jul 14, 2016 at 11:57:46AM +0200, Thomas Huth wrote:
> >> Some of the machines that we have got a firmware image for write
> >> some output to the serial console while booting up. We can use
> >> this output to make sure that the machine is basically working,
> >> so this adds a test that checks the output of these machines
> >> for some well-known "magic" strings.
> >>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> > 
> > I love this idea.  A couple of queries about the implemntation.
> > 
> >> ---
> >>  tests/Makefile.include   |   8 ++++
> >>  tests/boot-serial-test.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 118 insertions(+)
> >>  create mode 100644 tests/boot-serial-test.c
> >>
> >> diff --git a/tests/Makefile.include b/tests/Makefile.include
> >> index b7784d3..ba1cc8d 100644
> >> --- a/tests/Makefile.include
> >> +++ b/tests/Makefile.include
> >> @@ -194,6 +194,7 @@ check-qtest-i386-y += tests/hd-geo-test$(EXESUF)
> >>  gcov-files-i386-y += hw/block/hd-geometry.c
> >>  check-qtest-i386-y += tests/boot-order-test$(EXESUF)
> >>  check-qtest-i386-y += tests/bios-tables-test$(EXESUF)
> >> +check-qtest-i386-y += tests/boot-serial-test$(EXESUF)
> >>  check-qtest-i386-y += tests/pxe-test$(EXESUF)
> >>  check-qtest-i386-y += tests/rtc-test$(EXESUF)
> >>  check-qtest-i386-y += tests/ipmi-kcs-test$(EXESUF)
> >> @@ -241,6 +242,8 @@ check-qtest-x86_64-y += $(check-qtest-i386-y)
> >>  gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
> >>  gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
> >>  
> >> +check-qtest-alpha-y = tests/boot-serial-test$(EXESUF)
> >> +
> >>  check-qtest-mips-y = tests/endianness-test$(EXESUF)
> >>  check-qtest-mips64-y = tests/endianness-test$(EXESUF)
> >>  check-qtest-mips64el-y = tests/endianness-test$(EXESUF)
> >> @@ -248,12 +251,14 @@ check-qtest-mips64el-y = tests/endianness-test$(EXESUF)
> >>  check-qtest-ppc-y = tests/endianness-test$(EXESUF)
> >>  check-qtest-ppc-y += tests/boot-order-test$(EXESUF)
> >>  check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
> >> +check-qtest-ppc-y += tests/boot-serial-test$(EXESUF)
> >>  
> >>  check-qtest-ppc64-y = tests/endianness-test$(EXESUF)
> >>  check-qtest-ppc64-y += tests/boot-order-test$(EXESUF)
> >>  check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF)
> >>  gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c
> >>  check-qtest-ppc64-y += tests/prom-env-test$(EXESUF)
> >> +check-qtest-ppc64-y += tests/boot-serial-test$(EXESUF)
> >>  
> >>  check-qtest-sh4-y = tests/endianness-test$(EXESUF)
> >>  check-qtest-sh4eb-y = tests/endianness-test$(EXESUF)
> >> @@ -277,6 +282,8 @@ gcov-files-arm-y += arm-softmmu/hw/block/virtio-blk.c
> >>  check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
> >>  check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
> >>  
> >> +check-qtest-s390x-y = tests/boot-serial-test$(EXESUF)
> >> +
> >>  check-qtest-generic-y += tests/qom-test$(EXESUF)
> >>  
> >>  qapi-schema += alternate-any.json
> >> @@ -575,6 +582,7 @@ tests/ipmi-kcs-test$(EXESUF): tests/ipmi-kcs-test.o
> >>  tests/ipmi-bt-test$(EXESUF): tests/ipmi-bt-test.o
> >>  tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o
> >>  tests/boot-order-test$(EXESUF): tests/boot-order-test.o $(libqos-obj-y)
> >> +tests/boot-serial-test$(EXESUF): tests/boot-serial-test.o $(libqos-obj-y)
> >>  tests/bios-tables-test$(EXESUF): tests/bios-tables-test.o \
> >>  	tests/boot-sector.o $(libqos-obj-y)
> >>  tests/pxe-test$(EXESUF): tests/pxe-test.o tests/boot-sector.o $(libqos-obj-y)
> >> diff --git a/tests/boot-serial-test.c b/tests/boot-serial-test.c
> >> new file mode 100644
> >> index 0000000..3263dcd
> >> --- /dev/null
> >> +++ b/tests/boot-serial-test.c
> >> @@ -0,0 +1,110 @@
> >> +/*
> >> + * Test serial output of some machines.
> >> + *
> >> + * Copyright 2016 Thomas Huth, Red Hat Inc.
> >> + *
> >> + * This work is licensed under the terms of the GNU GPL, version 2
> >> + * or later. See the COPYING file in the top-level directory.
> >> + *
> >> + * This test is used to check that the serial output of the firmware
> >> + * (that we provide for some machines) contains an expected string.
> >> + * Thus we check that the firmware still boots at least to a certain
> >> + * point and so we know that the machine is not completely broken.
> >> + */
> >> +
> >> +#include "qemu/osdep.h"
> >> +#include "libqtest.h"
> >> +
> >> +typedef struct testdef {
> >> +    const char *arch;       /* Target architecture */
> >> +    const char *machine;    /* Name of the machine */
> >> +    const char *extra;      /* Additional parameters */
> >> +    const char *expect;     /* Expected string in the serial output */
> >> +} testdef_t;
> >> +
> >> +static testdef_t tests[] = {
> >> +    { "alpha", "clipper", "", "PCI:" },
> >> +    { "ppc", "ppce500", "", "U-Boot" },
> >> +    { "ppc", "prep", "", "Open Hack'Ware BIOS" },
> >> +    { "ppc64", "ppce500", "", "U-Boot" },
> >> +    { "ppc64", "prep", "", "Open Hack'Ware BIOS" },
> >> +    { "ppc64", "pseries", "", "Open Firmware" },
> >> +    { "i386", "isapc", "-device sga", "SGABIOS" },
> >> +    { "i386", "pc", "-device sga", "SGABIOS" },
> >> +    { "i386", "q35", "-device sga", "SGABIOS" },
> >> +    { "x86_64", "isapc", "-device sga", "SGABIOS" },
> >> +    { "x86_64", "pc", "-device sga", "SGABIOS" },
> >> +    { "x86_64", "q35", "-device sga", "SGABIOS" },
> >> +    { "s390x", "s390-ccw-virtio",
> >> +      "-nodefaults -device sclpconsole,chardev=serial0", "virtio device" },
> >> +    { NULL }
> >> +};
> >> +
> >> +static void check_guest_output(const testdef_t *test, int fd)
> >> +{
> >> +    bool output_ok = false;
> >> +    int i, pos = 0;
> >> +    char ch;
> >> +
> >> +    /* Poll serial output... Wait at most 30 seconds */
> >> +    for (i = 0; i < 3000; ++i) {
> >> +        while (read(fd, &ch, 1) == 1) {
> > 
> > It might be a good idea to check for actual errors from read() here.
> 
> Ok.
> 
> >> +            if (ch == test->expect[pos]) {
> >> +                pos += 1;
> >> +                if (test->expect[pos] == '\0') {
> >> +                    /* We've reached the end of the expected string! */
> >> +                    output_ok = true;
> >> +                    goto done;
> >> +                }
> >> +            } else {
> >> +                pos = 0;
> >> +            }
> >> +        }
> >> +        g_usleep(10000);
> >> +    }
> >> +
> >> +done:
> >> +    g_assert(output_ok);
> >> +}
> >> +
> >> +static void test_machine(const void *data)
> >> +{
> >> +    const testdef_t *test = data;
> >> +    char *args;
> >> +    char tmpname[] = "/tmp/qtest-boot-serial-XXXXXX";
> >> +    int fd;
> >> +
> >> +    fd = mkstemp(tmpname);
> >> +    g_assert(fd != -1);
> >> +
> >> +    args = g_strdup_printf("-M %s,accel=tcg -chardev file,id=serial0,path=%s"
> >> +                           " -serial chardev:serial0 %s", test->machine,
> >> +                           tmpname, test->extra);
> >> +
> >> +    qtest_start(args);
> >> +    unlink(tmpname);
> >> +
> >> +    check_guest_output(test, fd);
> >> +    qtest_quit(global_qtest);
> >> +
> >> +    g_free(args);
> >> +    close(fd);
> >> +}
> >> +
> >> +int main(int argc, char *argv[])
> >> +{
> >> +    const char *arch = qtest_get_arch();
> >> +    int i;
> >> +
> >> +    g_test_init(&argc, &argv, NULL);
> >> +
> >> +    for (i = 0; tests[i].arch != NULL; i++) {
> >> +        if (strcmp(arch, tests[i].arch) == 0) {
> >> +            char *name = g_strdup_printf("boot-serial/%s", tests[i].machine);
> >> +            qtest_add_data_func(name, &tests[i], test_machine);
> > 
> > Do we need some conditionals in case certain machine types are
> > configured out?
> 
> You mean something like using "#ifdef CONFIG_PREP" ? That unfortunately
> does not work since it's not exported as a pre-processor definition,
> it's only a Makefile variable. So other tests like boot-order-test.c can
> also not check for the available machine types and assume that they are
> simply always there.

I was thinking more of actually querying qemu -M ?, but yeah other
tests assume the necessary machines are there, so I think it's
probably ok to do so here as well.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]