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

[Thomas Huth <thuth@redhat.com>]

On 15.03.2018 10:21, Daniel P. Berrangé wrote:
> On Thu, Mar 15, 2018 at 08:49:04AM +0100, Thomas Huth wrote:
>> 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 <thuth@redhat.com>
>> ---
>>  tests/Makefile.include |   3 +
>>  tests/cdrom-test.c     | 155 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 158 insertions(+)
>>  create mode 100644 tests/cdrom-test.c
>>
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index ef9b88c..a104222 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -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)$@")
>> diff --git a/tests/cdrom-test.c b/tests/cdrom-test.c
>> new file mode 100644
>> index 0000000..1fc5230
>> --- /dev/null
>> +++ b/tests/cdrom-test.c
>> @@ -0,0 +1,155 @@
>> +/*
>> + * Various tests for emulated CD-ROM drives.
>> + *
>> + * Copyright (c) 2018 Red Hat Inc.
>> + *
>> + * Author:
>> + *    Thomas Huth <thuth@redhat.com>
>> + *
>> + * 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);
>> +        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