Skip to contentSkip to conversation

Add new CD-ROM related qtests

PATCH Superseded Thomas Huth 6 participants 14 messages Mar 15, 2018

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Thomas Huth

author
#0 lore raw

cover Add new CD-ROM related qtests

With one of my clean-up patches (see commit 1454509726719e0933c800), I recently accidentially broke the "-cdrom" parameter (more precisely "-drive if=scsi") on a couple of boards, since there was no error detected during the "make check" regression testing. This is clearly an indication that we are lacking tests in this area.
So this small patch series now introduces some tests for CD-ROM drives:
The first two patches introduce the possibility to check that booting from CD-ROM drives still works fine for x86 and s390x, and the third patch adds a test that certain machines can at least still be started with the "-cdrom" parameter (i.e. that test would have catched the mistake that I did with my SCSI cleanup patch).

Thomas Huth (3):
tests/boot-sector: Add magic bytes to s390x boot code header tests/cdboot: Test booting from CD-ROM ISO image file tests/cdrom-test: Test that -cdrom parameter is working

 tests/Makefile.include |  10 ++-
 tests/boot-sector.c    |   9 ++-
 tests/cdrom-test.c     | 216 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 231 insertions(+), 4 deletions(-)
 create mode 100644 tests/cdrom-test.c
Signature · 1 line
1.8.3.1
author
#1 lore raw

PATCH 1/3 tests/boot-sector: Add magic bytes to s390x boot code header

We're going to use the s390x boot code for testing CD-ROM booting. But the ISO loader of the s390-ccw bios is a little bit more picky than the network loader and expects some magic bytes in the header of the file (see linux_s390_magic in pc-bios/s390-ccw/bootmap.c), so we've got to add them in our boot code here, too.

Signed-off-by Thomas Huth <[email protected]>
---
 tests/boot-sector.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
tests/boot-sector.c +6 -3
index c373f0e..04721c9 100644
@@ -68,8 +68,11 @@ static uint8_t x86_boot_sector[512] = {
 };
 
 /* For s390x, use a mini "kernel" with the appropriate signature */
-static const uint8_t s390x_psw[] = {
-    0x00, 0x08, 0x00, 0x00, 0x80, 0x01, 0x00, 0x00
+static const uint8_t s390x_psw_and_magic[] = {
+    0x00, 0x08, 0x00, 0x00, 0x80, 0x01, 0x00, 0x00,
+    0x02, 0x00, 0x00, 0x18, 0x60, 0x00, 0x00, 0x50,
+    0x02, 0x00, 0x00, 0x68, 0x60, 0x00, 0x00, 0x50,
+    0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40
 };
 static const uint8_t s390x_code[] = {
     0xa7, 0xf4, 0x00, 0x0a,                                /* j 0x10010 */

@@ -110,7 +113,7 @@ int boot_sector_init(char *fname)
     } else if (g_str_equal(arch, "s390x")) {
         len = 0x10000 + sizeof(s390x_code);
         boot_code = g_malloc0(len);
-        memcpy(boot_code, s390x_psw, sizeof(s390x_psw));
+        memcpy(boot_code, s390x_psw_and_magic, sizeof(s390x_psw_and_magic));
         memcpy(&boot_code[0x10000], s390x_code, sizeof(s390x_code));
     } else {
         g_assert_not_reached();
Signature · 1 line
1.8.3.1
Reviewed-by on v1 1/3
#2 lore raw

Re: [Qemu-devel] [qemu-s390x] [PATCH 1/3] tests/boot-sector: Add magic bytes to s390x boot code header

Quoted 7 lines · Thomas Huth

We're going to use the s390x boot code for testing CD-ROM booting. But the ISO loader of the s390-ccw bios is a little bit more picky than the network loader and expects some magic bytes in the header of the file (see linux_s390_magic in pc-bios/s390-ccw/bootmap.c), so we've got to add them in our boot code here, too.

Signed-off-by Thomas Huth <[email protected]>
Reviewed-by Christian Borntraeger <[email protected]>
Quoted 32 lines
---
 tests/boot-sector.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
tests/boot-sector.c +6 -3
index c373f0e..04721c9 100644
@@ -68,8 +68,11 @@ static uint8_t x86_boot_sector[512] = {
 };
 
 /* For s390x, use a mini "kernel" with the appropriate signature */
-static const uint8_t s390x_psw[] = {
-    0x00, 0x08, 0x00, 0x00, 0x80, 0x01, 0x00, 0x00
+static const uint8_t s390x_psw_and_magic[] = {
+    0x00, 0x08, 0x00, 0x00, 0x80, 0x01, 0x00, 0x00,
+    0x02, 0x00, 0x00, 0x18, 0x60, 0x00, 0x00, 0x50,
+    0x02, 0x00, 0x00, 0x68, 0x60, 0x00, 0x00, 0x50,
+    0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40
 };
 static const uint8_t s390x_code[] = {
     0xa7, 0xf4, 0x00, 0x0a,                                /* j 0x10010 */

@@ -110,7 +113,7 @@ int boot_sector_init(char *fname)
     } else if (g_str_equal(arch, "s390x")) {
         len = 0x10000 + sizeof(s390x_code);
         boot_code = g_malloc0(len);
-        memcpy(boot_code, s390x_psw, sizeof(s390x_psw));
+        memcpy(boot_code, s390x_psw_and_magic, sizeof(s390x_psw_and_magic));
         memcpy(&boot_code[0x10000], s390x_code, sizeof(s390x_code));
     } else {
         g_assert_not_reached();
#3 lore raw

Re: [Qemu-devel] [PATCH 1/3] tests/boot-sector: Add magic bytes to s390x boot code header

In reply to Thomas Huth
Quoted 16 lines · Thomas Huth

We're going to use the s390x boot code for testing CD-ROM booting. But the ISO loader of the s390-ccw bios is a little bit more picky than the network loader and expects some magic bytes in the header of the file (see linux_s390_magic in pc-bios/s390-ccw/bootmap.c), so we've got to add them in our boot code here, too.

Signed-off-by Thomas Huth <[email protected]>
---
 tests/boot-sector.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
tests/boot-sector.c +0 -0
index c373f0e..04721c9 100644

tests/boot-sector.c static uint8_t x86_boot_sector[512] = { · line 71

Quoted 6 lines
tests/boot-sector.c +1 -2
@@ -68,8 +68,11 @@ static uint8_t x86_boot_sector[512] = {
 };
 
 /* For s390x, use a mini "kernel" with the appropriate signature */
-static const uint8_t s390x_psw[] = {
-    0x00, 0x08, 0x00, 0x00, 0x80, 0x01, 0x00, 0x00
+static const uint8_t s390x_psw_and_magic[] = {

Can you add a comment such "see linux_s390_magic in pc-bios/s390-ccw/bootmap.c"?

Quoted 17 lines
tests/boot-sector.c +5 -1
+    0x00, 0x08, 0x00, 0x00, 0x80, 0x01, 0x00, 0x00,
+    0x02, 0x00, 0x00, 0x18, 0x60, 0x00, 0x00, 0x50,
+    0x02, 0x00, 0x00, 0x68, 0x60, 0x00, 0x00, 0x50,
+    0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40
 };
 static const uint8_t s390x_code[] = {
     0xa7, 0xf4, 0x00, 0x0a,                                /* j 0x10010 */

@@ -110,7 +113,7 @@ int boot_sector_init(char *fname)
     } else if (g_str_equal(arch, "s390x")) {
         len = 0x10000 + sizeof(s390x_code);
         boot_code = g_malloc0(len);
-        memcpy(boot_code, s390x_psw, sizeof(s390x_psw));
+        memcpy(boot_code, s390x_psw_and_magic, sizeof(s390x_psw_and_magic));
         memcpy(&boot_code[0x10000], s390x_code, sizeof(s390x_code));
     } else {
         g_assert_not_reached();
author
#4 lore raw

Re: [Qemu-devel] [PATCH 1/3] tests/boot-sector: Add magic bytes to s390x boot code header

Quoted 19 lines · Philippe Mathieu-Daudé
Quoted 18 lines · Thomas Huth

We're going to use the s390x boot code for testing CD-ROM booting. But the ISO loader of the s390-ccw bios is a little bit more picky than the network loader and expects some magic bytes in the header of the file (see linux_s390_magic in pc-bios/s390-ccw/bootmap.c), so we've got to add them in our boot code here, too.

Signed-off-by Thomas Huth <[email protected]>
---
 tests/boot-sector.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
tests/boot-sector.c +0 -0
index c373f0e..04721c9 100644
@@ -68,8 +68,11 @@ static uint8_t x86_boot_sector[512] = {
 };
Quoted 7 lines

tests/boot-sector.c static uint8_t x86_boot_sector[512] = { · line 71

Quoted 4 lines
tests/boot-sector.c +1 -2
 /* For s390x, use a mini "kernel" with the appropriate signature */
-static const uint8_t s390x_psw[] = {
-    0x00, 0x08, 0x00, 0x00, 0x80, 0x01, 0x00, 0x00
+static const uint8_t s390x_psw_and_magic[] = {

Can you add a comment such "see linux_s390_magic in pc-bios/s390-ccw/bootmap.c"?

Sure, I'll add a comment.

Thomas

#5 lore raw

Re: [Qemu-devel] [PATCH 1/3] tests/boot-sector: Add magic bytes to s390x boot code header

In reply to Philippe Mathieu-Daudé
Quoted 19 lines · Philippe Mathieu-Daudé
Quoted 18 lines · Thomas Huth

We're going to use the s390x boot code for testing CD-ROM booting. But the ISO loader of the s390-ccw bios is a little bit more picky than the network loader and expects some magic bytes in the header of the file (see linux_s390_magic in pc-bios/s390-ccw/bootmap.c), so we've got to add them in our boot code here, too.

Signed-off-by Thomas Huth <[email protected]>
---
 tests/boot-sector.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
tests/boot-sector.c +0 -0
index c373f0e..04721c9 100644
@@ -68,8 +68,11 @@ static uint8_t x86_boot_sector[512] = {
 };
Quoted 7 lines

tests/boot-sector.c static uint8_t x86_boot_sector[512] = { · line 71

Quoted 4 lines
tests/boot-sector.c +1 -2
 /* For s390x, use a mini "kernel" with the appropriate signature */
-static const uint8_t s390x_psw[] = {
-    0x00, 0x08, 0x00, 0x00, 0x80, 0x01, 0x00, 0x00
+static const uint8_t s390x_psw_and_magic[] = {

Can you add a comment such "see linux_s390_magic in pc-bios/s390-ccw/bootmap.c"?

Or better yet, copy the code comment from there.

Quoted 17 lines
Quoted 17 lines
tests/boot-sector.c +5 -1
+    0x00, 0x08, 0x00, 0x00, 0x80, 0x01, 0x00, 0x00,
+    0x02, 0x00, 0x00, 0x18, 0x60, 0x00, 0x00, 0x50,
+    0x02, 0x00, 0x00, 0x68, 0x60, 0x00, 0x00, 0x50,
+    0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40
 };
 static const uint8_t s390x_code[] = {
     0xa7, 0xf4, 0x00, 0x0a,                                /* j 0x10010 */

@@ -110,7 +113,7 @@ int boot_sector_init(char *fname)
     } else if (g_str_equal(arch, "s390x")) {
         len = 0x10000 + sizeof(s390x_code);
         boot_code = g_malloc0(len);
-        memcpy(boot_code, s390x_psw, sizeof(s390x_psw));
+        memcpy(boot_code, s390x_psw_and_magic, sizeof(s390x_psw_and_magic));
         memcpy(&boot_code[0x10000], s390x_code, sizeof(s390x_code));
     } else {
         g_assert_not_reached();
author
#6 lore raw

PATCH 2/3 tests/cdboot: Test booting from CD-ROM ISO image file

In reply to Thomas Huth

We already have the code for a boot file in tests/boot-sector.c, so if the genisoimage program is available, we can easily create a bootable CD ISO image that we can use for testing whether our CD-ROM emulation and the BIOS CD-ROM boot works correctly.

Signed-off-by Thomas Huth <[email protected]>
---
 tests/Makefile.include |   3 +
 tests/cdrom-test.c     | 155 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 158 insertions(+)
 create mode 100644 tests/cdrom-test.c
tests/Makefile.include +3 -0
index ef9b88c..a104222 100644
@@ -304,6 +304,7 @@ check-qtest-i386-$(CONFIG_POSIX) += tests/test-filter-redirector$(EXESUF)
 check-qtest-i386-y += tests/migration-test$(EXESUF)
 check-qtest-i386-y += tests/test-x86-cpuid-compat$(EXESUF)
 check-qtest-i386-y += tests/numa-test$(EXESUF)
+check-qtest-i386-y += tests/cdrom-test$(EXESUF)
 check-qtest-x86_64-y += $(check-qtest-i386-y)
 check-qtest-x86_64-y += tests/sdhci-test$(EXESUF)
 gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c

@@ -398,6 +399,7 @@ check-qtest-s390x-y += tests/virtio-balloon-test$(EXESUF)
 check-qtest-s390x-y += tests/virtio-console-test$(EXESUF)
 check-qtest-s390x-y += tests/virtio-serial-test$(EXESUF)
 check-qtest-s390x-y += tests/cpu-plug-test$(EXESUF)
+check-qtest-s390x-y += tests/cdrom-test$(EXESUF)
 
 check-qtest-generic-y += tests/qom-test$(EXESUF)
 check-qtest-generic-y += tests/test-hmp$(EXESUF)

@@ -829,6 +831,7 @@ tests/test-qapi-util$(EXESUF): tests/test-qapi-util.o $(test-util-obj-y)
 tests/numa-test$(EXESUF): tests/numa-test.o
 tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o tests/boot-sector.o tests/acpi-utils.o
 tests/sdhci-test$(EXESUF): tests/sdhci-test.o $(libqos-pc-obj-y)
+tests/cdrom-test$(EXESUF): tests/cdrom-test.o tests/boot-sector.o $(libqos-obj-y)
 
 tests/migration/stress$(EXESUF): tests/migration/stress.o
 	$(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< ,"LINK","$(TARGET_DIR)$@")
tests/cdrom-test.c new file +155 -0

155 lines not on this page · raw message

Signature · 1 line
1.8.3.1
#7 lore raw

Re: [Qemu-devel] [PATCH 2/3] tests/cdboot: Test booting from CD-ROM ISO image file

Quoted 72 lines · Thomas Huth

We already have the code for a boot file in tests/boot-sector.c, so if the genisoimage program is available, we can easily create a bootable CD ISO image that we can use for testing whether our CD-ROM emulation and the BIOS CD-ROM boot works correctly.

Signed-off-by Thomas Huth <[email protected]>
---
 tests/Makefile.include |   3 +
 tests/cdrom-test.c     | 155 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 158 insertions(+)
 create mode 100644 tests/cdrom-test.c
tests/Makefile.include +3 -0
index ef9b88c..a104222 100644
@@ -304,6 +304,7 @@ check-qtest-i386-$(CONFIG_POSIX) += tests/test-filter-redirector$(EXESUF)
 check-qtest-i386-y += tests/migration-test$(EXESUF)
 check-qtest-i386-y += tests/test-x86-cpuid-compat$(EXESUF)
 check-qtest-i386-y += tests/numa-test$(EXESUF)
+check-qtest-i386-y += tests/cdrom-test$(EXESUF)
 check-qtest-x86_64-y += $(check-qtest-i386-y)
 check-qtest-x86_64-y += tests/sdhci-test$(EXESUF)
 gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c

@@ -398,6 +399,7 @@ check-qtest-s390x-y += tests/virtio-balloon-test$(EXESUF)
 check-qtest-s390x-y += tests/virtio-console-test$(EXESUF)
 check-qtest-s390x-y += tests/virtio-serial-test$(EXESUF)
 check-qtest-s390x-y += tests/cpu-plug-test$(EXESUF)
+check-qtest-s390x-y += tests/cdrom-test$(EXESUF)
 
 check-qtest-generic-y += tests/qom-test$(EXESUF)
 check-qtest-generic-y += tests/test-hmp$(EXESUF)

@@ -829,6 +831,7 @@ tests/test-qapi-util$(EXESUF): tests/test-qapi-util.o $(test-util-obj-y)
 tests/numa-test$(EXESUF): tests/numa-test.o
 tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o tests/boot-sector.o tests/acpi-utils.o
 tests/sdhci-test$(EXESUF): tests/sdhci-test.o $(libqos-pc-obj-y)
+tests/cdrom-test$(EXESUF): tests/cdrom-test.o tests/boot-sector.o $(libqos-obj-y)
 
 tests/migration/stress$(EXESUF): tests/migration/stress.o
 	$(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< ,"LINK","$(TARGET_DIR)$@")
tests/cdrom-test.c new file +26 -0
new file mode 100644
index 0000000..1fc5230
@@ -0,0 +1,155 @@
+/*
+ * Various tests for emulated CD-ROM drives.
+ *
+ * Copyright (c) 2018 Red Hat Inc.
+ *
+ * Author:
+ *    Thomas Huth <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2
+ * or later. See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "boot-sector.h"
+
+static char isoimage[] = "cdrom-boot-iso-XXXXXX";
+
+static int gen_iso(const char *fmt, ...)
+{
+    char *params, *command;
+    va_list args;
+    pid_t pid;
+    int status;
+
+    pid = fork();

tests/cdrom-test.c line 36

Quoted 10 lines
tests/cdrom-test.c new file +10 -0
+    if (pid == 0) {
+        va_start(args, fmt);
+        params = g_strdup_vprintf(fmt, args);
+        va_end(args);
+        command = g_strdup_printf("exec genisoimage %s", params);
+        g_free(params);
+        execlp("/bin/sh", "sh", "-c", command, NULL);
+        exit(1);
+    }
+    wait(&status);

IMHO this should just use g_spawn_sync(), also the use of shell seems rather unneccessary - why not just run genisoimage directly ?

https://developer.gnome.org/glib/stable/glib-Spawning-Processes.html#g-spawn-sync

Quoted 23 lines
tests/cdrom-test.c new file +23 -0
+
+    return WEXITSTATUS(status);
+}
+
+static int prepare_image(const char *arch, char *isoimage)
+{
+    char srcdir[] = "cdrom-test-dir-XXXXXX";
+    char *codefile = NULL;
+    int ifh, ret = -1;
+
+    ifh = mkstemp(isoimage);
+    if (ifh < 0) {
+        perror("Error creating temporary iso image file");
+        return -1;
+    }
+    if (!mkdtemp(srcdir)) {
+        perror("Error creating temporary directory");
+        goto cleanup;
+    }
+
+    if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64") ||
+        g_str_equal(arch, "s390x")) {
+        codefile = g_strdup_printf("%s/bootcode-XXXXXX", srcdir);

tests/cdrom-test.c line 69

Quoted 10 lines
tests/cdrom-test.c new file +10 -0
+        ret = boot_sector_init(codefile);
+        if (ret) {
+            goto cleanup;
+        }
+    } else {
+        g_assert_not_reached();
+    }
+
+    ret = gen_iso("-quiet -l -no-emul-boot -b %s -o %s %s",
+                  strrchr(codefile, '/') + 1, isoimage, srcdir);

It would be easy to just declare the args as an array here

  char *genisoargv[] = {
    "genisoimage", "-quiet", "-l", "-no-emul-boot", "-b",
    strrchr(codefile, "/") + 1, "-o", isoimage, srcdir,
    NULL,

};

to then pass to g_spawn_sync

tests/cdrom-test.c line 82

Quoted 13 lines
tests/cdrom-test.c new file +13 -0
+    if (ret) {
+        fprintf(stderr, "genisoimage failed: %i\n", ret);
+    }
+
+    unlink(codefile);
+
+cleanup:
+    g_free(codefile);
+    rmdir(srcdir);
+    close(ifh);
+
+    return ret;
+}

Regards,
Daniel

Signature · 3 lines
|: 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 :|
author
#8 lore raw

Re: [Qemu-devel] [PATCH 2/3] tests/cdboot: Test booting from CD-ROM ISO image file

Quoted 79 lines · Daniel P. Berrangé
Quoted 78 lines · Thomas Huth

We already have the code for a boot file in tests/boot-sector.c, so if the genisoimage program is available, we can easily create a bootable CD ISO image that we can use for testing whether our CD-ROM emulation and the BIOS CD-ROM boot works correctly.

Signed-off-by Thomas Huth <[email protected]>
---
 tests/Makefile.include |   3 +
 tests/cdrom-test.c     | 155 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 158 insertions(+)
 create mode 100644 tests/cdrom-test.c
tests/Makefile.include +3 -0
index ef9b88c..a104222 100644
@@ -304,6 +304,7 @@ check-qtest-i386-$(CONFIG_POSIX) += tests/test-filter-redirector$(EXESUF)
 check-qtest-i386-y += tests/migration-test$(EXESUF)
 check-qtest-i386-y += tests/test-x86-cpuid-compat$(EXESUF)
 check-qtest-i386-y += tests/numa-test$(EXESUF)
+check-qtest-i386-y += tests/cdrom-test$(EXESUF)
 check-qtest-x86_64-y += $(check-qtest-i386-y)
 check-qtest-x86_64-y += tests/sdhci-test$(EXESUF)
 gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c

@@ -398,6 +399,7 @@ check-qtest-s390x-y += tests/virtio-balloon-test$(EXESUF)
 check-qtest-s390x-y += tests/virtio-console-test$(EXESUF)
 check-qtest-s390x-y += tests/virtio-serial-test$(EXESUF)
 check-qtest-s390x-y += tests/cpu-plug-test$(EXESUF)
+check-qtest-s390x-y += tests/cdrom-test$(EXESUF)
 
 check-qtest-generic-y += tests/qom-test$(EXESUF)
 check-qtest-generic-y += tests/test-hmp$(EXESUF)

@@ -829,6 +831,7 @@ tests/test-qapi-util$(EXESUF): tests/test-qapi-util.o $(test-util-obj-y)
 tests/numa-test$(EXESUF): tests/numa-test.o
 tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o tests/boot-sector.o tests/acpi-utils.o
 tests/sdhci-test$(EXESUF): tests/sdhci-test.o $(libqos-pc-obj-y)
+tests/cdrom-test$(EXESUF): tests/cdrom-test.o tests/boot-sector.o $(libqos-obj-y)
 
 tests/migration/stress$(EXESUF): tests/migration/stress.o
 	$(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< ,"LINK","$(TARGET_DIR)$@")
tests/cdrom-test.c new file +32 -0
new file mode 100644
index 0000000..1fc5230
@@ -0,0 +1,155 @@
+/*
+ * Various tests for emulated CD-ROM drives.
+ *
+ * Copyright (c) 2018 Red Hat Inc.
+ *
+ * Author:
+ *    Thomas Huth <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2
+ * or later. See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "boot-sector.h"
+
+static char isoimage[] = "cdrom-boot-iso-XXXXXX";
+
+static int gen_iso(const char *fmt, ...)
+{
+    char *params, *command;
+    va_list args;
+    pid_t pid;
+    int status;
+
+    pid = fork();
+    if (pid == 0) {
+        va_start(args, fmt);
+        params = g_strdup_vprintf(fmt, args);
+        va_end(args);
+        command = g_strdup_printf("exec genisoimage %s", params);
+        g_free(params);
Quoted 8 lines

tests/cdrom-test.c line 36

Quoted 4 lines
tests/cdrom-test.c new file +4 -0
+        execlp("/bin/sh", "sh", "-c", command, NULL);
+        exit(1);
+    }
+    wait(&status);

IMHO this should just use g_spawn_sync(), also the use of shell seems rather unneccessary - why not just run genisoimage directly ?

That code was simply "inspired" from the execlp() code in qtest_init_without_qmp_handshake() ... but g_spawn_sync() sounds like a good alternative here, so I'll try to switch to that one instead.

Thanks,
Thomas

#9 lore raw

Re: [Qemu-devel] [PATCH 2/3] tests/cdboot: Test booting from CD-ROM ISO image file

Quoted 12 lines · Thomas Huth
Quoted 12 lines
Quoted 11 lines
tests/cdrom-test.c new file +11 -0
+    pid = fork();
+    if (pid == 0) {
+        va_start(args, fmt);
+        params = g_strdup_vprintf(fmt, args);
+        va_end(args);
+        command = g_strdup_printf("exec genisoimage %s", params);
+        g_free(params);
+        execlp("/bin/sh", "sh", "-c", command, NULL);
+        exit(1);
+    }
+    wait(&status);
Quoted 2 lines
Quoted 2 lines

IMHO this should just use g_spawn_sync(), also the use of shell seems rather unneccessary

and potentially dangerous - if we aren't absolutely positive that we aren't going to improperly expand shell metacharacters embedded in params.

Quoted 5 lines
Quoted 2 lines

- why not just run genisoimage
directly ?

That code was simply "inspired" from the execlp() code in qtest_init_without_qmp_handshake()

Sounds like a good idea for a future cleanup patch ;)

Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
author
#10 lore raw

PATCH 3/3 tests/cdrom-test: Test that -cdrom parameter is working

In reply to Thomas Huth

Commit 1454509726719e0933c800 recently broke the "-cdrom" parameter on a couple of boards without that we noticed it immediately. Thus add a test which checks that "-cdrom" can at least be used to start QEMU with certain machine types.

Signed-off-by Thomas Huth <[email protected]>
---
 tests/Makefile.include |  7 +++++-
 tests/cdrom-test.c     | 63 +++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 68 insertions(+), 2 deletions(-)
tests/Makefile.include +6 -1
index a104222..b744fea 100644
@@ -321,8 +321,9 @@ check-qtest-microblaze-y = tests/boot-serial-test$(EXESUF)
 check-qtest-mips-y = tests/endianness-test$(EXESUF)
 
 check-qtest-mips64-y = tests/endianness-test$(EXESUF)
+check-qtest-mips64-y += tests/cdrom-test$(EXESUF)
 
-check-qtest-mips64el-y = tests/endianness-test$(EXESUF)
+check-qtest-mips64el-y = $(check-qtest-mips64-y)
 
 check-qtest-moxie-y = tests/boot-serial-test$(EXESUF)
 

@@ -356,6 +357,7 @@ check-qtest-ppc64-y += tests/display-vga-test$(EXESUF)
 check-qtest-ppc64-y += tests/numa-test$(EXESUF)
 check-qtest-ppc64-$(CONFIG_IVSHMEM) += tests/ivshmem-test$(EXESUF)
 check-qtest-ppc64-y += tests/cpu-plug-test$(EXESUF)
+check-qtest-ppc64-y += tests/cdrom-test$(EXESUF)
 
 check-qtest-sh4-y = tests/endianness-test$(EXESUF)
 

@@ -365,10 +367,12 @@ 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-sparc-y += tests/boot-serial-test$(EXESUF)
+check-qtest-sparc-y += tests/cdrom-test$(EXESUF)
 
 check-qtest-sparc64-y = tests/endianness-test$(EXESUF)
 check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
 check-qtest-sparc64-y += tests/boot-serial-test$(EXESUF)
+check-qtest-sparc64-y += tests/cdrom-test$(EXESUF)
 
 check-qtest-arm-y = tests/tmp105-test$(EXESUF)
 check-qtest-arm-y += tests/ds1338-test$(EXESUF)

@@ -384,6 +388,7 @@ check-qtest-arm-y += tests/sdhci-test$(EXESUF)
 check-qtest-aarch64-y = tests/numa-test$(EXESUF)
 check-qtest-aarch64-y += tests/sdhci-test$(EXESUF)
 check-qtest-aarch64-y += tests/boot-serial-test$(EXESUF)
+check-qtest-aarch64-y += tests/cdrom-test$(EXESUF)
 
 check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
tests/cdrom-test.c +62 -1

87 lines not on this page · raw message

Signature · 1 line
1.8.3.1
Reviewed-by on v1 3/3
#11 lore raw

Re: [Qemu-devel] [PATCH 3/3] tests/cdrom-test: Test that -cdrom parameter is working

Hi Thomas,

Quoted 48 lines · Thomas Huth

Commit 1454509726719e0933c800 recently broke the "-cdrom" parameter on a couple of boards without that we noticed it immediately. Thus add a test which checks that "-cdrom" can at least be used to start QEMU with certain machine types.

Signed-off-by Thomas Huth <[email protected]>
---
 tests/Makefile.include |  7 +++++-
 tests/cdrom-test.c     | 63 +++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 68 insertions(+), 2 deletions(-)
tests/Makefile.include +5 -1
index a104222..b744fea 100644
@@ -321,8 +321,9 @@ check-qtest-microblaze-y = tests/boot-serial-test$(EXESUF)
 check-qtest-mips-y = tests/endianness-test$(EXESUF)
 
 check-qtest-mips64-y = tests/endianness-test$(EXESUF)
+check-qtest-mips64-y += tests/cdrom-test$(EXESUF)
 
-check-qtest-mips64el-y = tests/endianness-test$(EXESUF)
+check-qtest-mips64el-y = $(check-qtest-mips64-y)
 
 check-qtest-moxie-y = tests/boot-serial-test$(EXESUF)
 

@@ -356,6 +357,7 @@ check-qtest-ppc64-y += tests/display-vga-test$(EXESUF)
 check-qtest-ppc64-y += tests/numa-test$(EXESUF)
 check-qtest-ppc64-$(CONFIG_IVSHMEM) += tests/ivshmem-test$(EXESUF)
 check-qtest-ppc64-y += tests/cpu-plug-test$(EXESUF)
+check-qtest-ppc64-y += tests/cdrom-test$(EXESUF)
 
 check-qtest-sh4-y = tests/endianness-test$(EXESUF)
 

@@ -365,10 +367,12 @@ 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-sparc-y += tests/boot-serial-test$(EXESUF)
+check-qtest-sparc-y += tests/cdrom-test$(EXESUF)
 
 check-qtest-sparc64-y = tests/endianness-test$(EXESUF)
 check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
 check-qtest-sparc64-y += tests/boot-serial-test$(EXESUF)
+check-qtest-sparc64-y += tests/cdrom-test$(EXESUF)
 
 check-qtest-arm-y = tests/tmp105-test$(EXESUF)
 check-qtest-arm-y += tests/ds1338-test$(EXESUF)

tests/Makefile.include check-qtest-arm-y += tests/sdhci-test$(EXESUF) · line 391

Quoted 4 lines
tests/Makefile.include +1 -0
@@ -384,6 +388,7 @@ check-qtest-arm-y += tests/sdhci-test$(EXESUF)
 check-qtest-aarch64-y = tests/numa-test$(EXESUF)
 check-qtest-aarch64-y += tests/sdhci-test$(EXESUF)
 check-qtest-aarch64-y += tests/boot-serial-test$(EXESUF)
+check-qtest-aarch64-y += tests/cdrom-test$(EXESUF)

Since you use g_str_equal(arch,), maybe this test can be added regardless the arch in check-qtest-generic.

Quoted 89 lines
tests/Makefile.include +0 -0
 check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
tests/cdrom-test.c +55 -1
index 1fc5230..5e516dd 100644
@@ -13,6 +13,7 @@
 #include "qemu/osdep.h"
 #include "libqtest.h"
 #include "boot-sector.h"
+#include "qapi/qmp/qdict.h"
 
 static char isoimage[] = "cdrom-boot-iso-XXXXXX";
 

@@ -62,7 +63,13 @@ static int prepare_image(const char *arch, char *isoimage)
             goto cleanup;
         }
     } else {
-        g_assert_not_reached();
+        /* Just create a dummy file */
+        char txt[] = "empty disc";
+        codefile = g_strdup_printf("%s/readme.txt", srcdir);
+        if (!g_file_set_contents(codefile, txt, sizeof(txt) - 1, NULL)) {
+            fprintf(stderr, "Failed to create '%s'\n", codefile);
+            goto cleanup;
+        }
     }
 
     ret = gen_iso("-quiet -l -no-emul-boot -b %s -o %s %s",

@@ -81,6 +88,32 @@ cleanup:
     return ret;
 }
 
+/**
+ * Check that at least the -cdrom parameter is basically working, i.e. we can
+ * see the filename of the ISO image in the output of "info block" afterwards
+ */
+static void test_cdrom_param(gconstpointer data)
+{
+    QTestState *qts;
+    char *resp;
+
+    qts = qtest_startf("-M %s -cdrom %s", (const char *)data, isoimage);
+    resp = qtest_hmp(qts, "info block");
+    g_assert(strstr(resp, isoimage) != 0);
+    g_free(resp);
+    qtest_quit(qts);
+}
+
+static void add_cdrom_param_tests(const char **machines)
+{
+    while (*machines) {
+        char *testname = g_strdup_printf("cdrom/param/%s", *machines);
+        qtest_add_data_func(testname, *machines, test_cdrom_param);
+        g_free(testname);
+        machines++;
+    }
+}
+
 static void test_cdboot(gconstpointer data)
 {
     QTestState *qts;

@@ -145,6 +178,34 @@ int main(int argc, char **argv)
         add_x86_tests();
     } else if (g_str_equal(arch, "s390x")) {
         add_s390x_tests();
+    } else if (g_str_equal(arch, "ppc64")) {
+        const char *ppcmachines[] = {
+            "pseries", "mac99", "g3beige", "40p", "prep", NULL
+        };
+        add_cdrom_param_tests(ppcmachines);
+    } else if (g_str_equal(arch, "sparc")) {
+        const char *sparcmachines[] = {
+            "LX", "SPARCClassic", "SPARCbook", "SS-10", "SS-20", "SS-4",
+            "SS-5", "SS-600MP", "Voyager", "leon3_generic", NULL
+        };
+        add_cdrom_param_tests(sparcmachines);
+    } else if (g_str_equal(arch, "sparc64")) {
+        const char *sparc64machines[] = {
+            "niagara", "sun4u", "sun4v", NULL
+        };
+        add_cdrom_param_tests(sparc64machines);
+    } else if (!strncmp(arch, "mips64", 6)) {
+        const char *mips64machines[] = {
+            "magnum", "malta", "mips", "mipssim", "pica61", NULL
+        };
+        add_cdrom_param_tests(mips64machines);

tests/cdrom-test.c int main(int argc, char **argv) · line 211

Quoted 10 lines
tests/cdrom-test.c +7 -0
+    } else if (g_str_equal(arch, "aarch64")) {
+        const char *aarch64machines[] = {
+            "realview-eb", "realview-eb-mpcore", "realview-pb-a8",
+            "realview-pbx-a9", "versatileab", "versatilepb", "vexpress-a15",
+            "vexpress-a9", "virt", NULL
+        };
+        add_cdrom_param_tests(aarch64machines);
     }
 
     ret = g_test_run();
Reviewed-by Philippe Mathieu-Daudé <[email protected]>
author
#12 lore raw

Re: [Qemu-devel] [PATCH 3/3] tests/cdrom-test: Test that -cdrom parameter is working

Quoted 48 lines · Philippe Mathieu-Daudé

Hi Thomas,

Quoted 45 lines · Thomas Huth

Commit 1454509726719e0933c800 recently broke the "-cdrom" parameter on a couple of boards without that we noticed it immediately. Thus add a test which checks that "-cdrom" can at least be used to start QEMU with certain machine types.

Signed-off-by Thomas Huth <[email protected]>
---
 tests/Makefile.include |  7 +++++-
 tests/cdrom-test.c     | 63 +++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 68 insertions(+), 2 deletions(-)
tests/Makefile.include +5 -1
index a104222..b744fea 100644
@@ -321,8 +321,9 @@ check-qtest-microblaze-y = tests/boot-serial-test$(EXESUF)
 check-qtest-mips-y = tests/endianness-test$(EXESUF)
 
 check-qtest-mips64-y = tests/endianness-test$(EXESUF)
+check-qtest-mips64-y += tests/cdrom-test$(EXESUF)
 
-check-qtest-mips64el-y = tests/endianness-test$(EXESUF)
+check-qtest-mips64el-y = $(check-qtest-mips64-y)
 
 check-qtest-moxie-y = tests/boot-serial-test$(EXESUF)
 

@@ -356,6 +357,7 @@ check-qtest-ppc64-y += tests/display-vga-test$(EXESUF)
 check-qtest-ppc64-y += tests/numa-test$(EXESUF)
 check-qtest-ppc64-$(CONFIG_IVSHMEM) += tests/ivshmem-test$(EXESUF)
 check-qtest-ppc64-y += tests/cpu-plug-test$(EXESUF)
+check-qtest-ppc64-y += tests/cdrom-test$(EXESUF)
 
 check-qtest-sh4-y = tests/endianness-test$(EXESUF)
 

@@ -365,10 +367,12 @@ 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-sparc-y += tests/boot-serial-test$(EXESUF)
+check-qtest-sparc-y += tests/cdrom-test$(EXESUF)
 
 check-qtest-sparc64-y = tests/endianness-test$(EXESUF)
 check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
 check-qtest-sparc64-y += tests/boot-serial-test$(EXESUF)
+check-qtest-sparc64-y += tests/cdrom-test$(EXESUF)
Quoted 10 lines

tests/Makefile.include check-qtest-arm-y += tests/sdhci-test$(EXESUF) · line 391

Quoted 7 lines
tests/Makefile.include +1 -0
 check-qtest-arm-y = tests/tmp105-test$(EXESUF)
 check-qtest-arm-y += tests/ds1338-test$(EXESUF)

@@ -384,6 +388,7 @@ check-qtest-arm-y += tests/sdhci-test$(EXESUF)
 check-qtest-aarch64-y = tests/numa-test$(EXESUF)
 check-qtest-aarch64-y += tests/sdhci-test$(EXESUF)
 check-qtest-aarch64-y += tests/boot-serial-test$(EXESUF)
+check-qtest-aarch64-y += tests/cdrom-test$(EXESUF)

Since you use g_str_equal(arch,), maybe this test can be added regardless the arch in check-qtest-generic.

Yes, sounds like a good idea. I think the test can even be run on other architectures by using the default machine or "-M none" ... I'll give it a try ...

Quoted 4 lines

tests/Makefile.include check-qtest-arm-y += tests/sdhci-test$(EXESUF) · line 393

Quoted 2 lines
tests/Makefile.include +0 -0
     ret = g_test_run();
Reviewed-by Philippe Mathieu-Daudé <[email protected]>

Thanks!
Thomas

#13 lore raw

Re: [Qemu-devel] [PATCH 3/3] tests/cdrom-test: Test that -cdrom parameter is working

In reply to Thomas Huth
Quoted 2 lines · Thomas Huth

Commit 1454509726719e0933c800 recently broke the "-cdrom" parameter on a couple of boards without that we noticed it immediately. Thus

s/without that we noticed/without us noticing/

Quoted 9 lines

add a test which checks that "-cdrom" can at least be used to start QEMU with certain machine types.

Signed-off-by Thomas Huth <[email protected]>
---
  tests/Makefile.include |  7 +++++-
  tests/cdrom-test.c     | 63 +++++++++++++++++++++++++++++++++++++++++++++++++-
  2 files changed, 68 insertions(+), 2 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org