[Qemu-devel] [PATCH] tests/boot-sector: Use mkstemp() to create a unique file name

[Qemu-devel] [PATCH] tests/boot-sector: Use mkstemp() to create a unique file name

[Thomas Huth]

The pxe-test is run for three different targets now (x86_64, i386
and ppc64), and the bios-tables-test is run for two targets (x86_64
and i386). But each of the tests is using an invariant name for the
disk image with the boot sector code - so if the tests are running
in parallel, there is a race condition that they destroy the disk
image of a parallel test program. Let's use mkstemp() to create
unique temporary files here instead.

Reported-by: Sascha Silbe <x-qemu@se-silbe.de>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/bios-tables-test.c | 2 +-
 tests/boot-sector.c      | 9 +++++++--
 tests/boot-sector.h      | 4 ++--
 tests/pxe-test.c         | 2 +-
 4 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 6ea2b6d..812f830 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -112,7 +112,7 @@ typedef struct {
     g_assert_cmpstr(ACPI_ASSERT_CMP_str, ==, expected); \
 } while (0)
 
-static const char *disk = "tests/acpi-test-disk.raw";
+static char disk[] = "tests/acpi-test-disk-XXXXXX";
 static const char *data_dir = "tests/acpi-test-data";
 #ifdef CONFIG_IASL
 static const char *iasl = stringify(CONFIG_IASL);
diff --git a/tests/boot-sector.c b/tests/boot-sector.c
index e3193c0..e75572f 100644
--- a/tests/boot-sector.c
+++ b/tests/boot-sector.c
@@ -69,10 +69,15 @@ static uint8_t boot_sector[0x7e000] = {
 };
 
 /* Create boot disk file.  */
-int boot_sector_init(const char *fname)
+int boot_sector_init(char *fname)
 {
-    FILE *f = fopen(fname, "w");
+    FILE *f = NULL;
+    int fd;
 
+    fd = mkstemp(fname);
+    if (fd != -1) {
+        f = fdopen(fd, "w");
+    }
     if (!f) {
         fprintf(stderr, "Couldn't open \"%s\": %s", fname, strerror(errno));
         return 1;
diff --git a/tests/boot-sector.h b/tests/boot-sector.h
index f64b477..35d61c7 100644
--- a/tests/boot-sector.h
+++ b/tests/boot-sector.h
@@ -14,8 +14,8 @@
 #ifndef TEST_BOOT_SECTOR_H
 #define TEST_BOOT_SECTOR_H
 
-/* Create boot disk file.  */
-int boot_sector_init(const char *fname);
+/* Create boot disk file. fname must be a suitable string for mkstemp() */
+int boot_sector_init(char *fname);
 
 /* Loop until signature in memory is OK.  */
 void boot_sector_test(void);
diff --git a/tests/pxe-test.c b/tests/pxe-test.c
index 5d3ddbe..34282d3 100644
--- a/tests/pxe-test.c
+++ b/tests/pxe-test.c
@@ -19,7 +19,7 @@
 
 #define NETNAME "net0"
 
-static const char *disk = "tests/pxe-test-disk.raw";
+static char disk[] = "tests/pxe-test-disk-XXXXXX";
 
 static void test_pxe_one(const char *params, bool ipv6)
 {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] tests/boot-sector: Use mkstemp() to create a unique file name

[Peter Maydell]

On 11 October 2016 at 14:32, Thomas Huth <thuth@redhat.com> wrote:
> The pxe-test is run for three different targets now (x86_64, i386
> and ppc64), and the bios-tables-test is run for two targets (x86_64
> and i386). But each of the tests is using an invariant name for the
> disk image with the boot sector code - so if the tests are running
> in parallel, there is a race condition that they destroy the disk
> image of a parallel test program. Let's use mkstemp() to create
> unique temporary files here instead.
>
> Reported-by: Sascha Silbe <x-qemu@se-silbe.de>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/bios-tables-test.c | 2 +-
>  tests/boot-sector.c      | 9 +++++++--
>  tests/boot-sector.h      | 4 ++--
>  tests/pxe-test.c         | 2 +-
>  4 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 6ea2b6d..812f830 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -112,7 +112,7 @@ typedef struct {
>      g_assert_cmpstr(ACPI_ASSERT_CMP_str, ==, expected); \
>  } while (0)
>
> -static const char *disk = "tests/acpi-test-disk.raw";
> +static char disk[] = "tests/acpi-test-disk-XXXXXX";
>  static const char *data_dir = "tests/acpi-test-data";
>  #ifdef CONFIG_IASL
>  static const char *iasl = stringify(CONFIG_IASL);
> diff --git a/tests/boot-sector.c b/tests/boot-sector.c
> index e3193c0..e75572f 100644
> --- a/tests/boot-sector.c
> +++ b/tests/boot-sector.c
> @@ -69,10 +69,15 @@ static uint8_t boot_sector[0x7e000] = {
>  };
>
>  /* Create boot disk file.  */
> -int boot_sector_init(const char *fname)
> +int boot_sector_init(char *fname)
>  {
> -    FILE *f = fopen(fname, "w");
> +    FILE *f = NULL;
> +    int fd;
>
> +    fd = mkstemp(fname);
> +    if (fd != -1) {
> +        f = fdopen(fd, "w");
> +    }

Given that we're only writing a pile of bytes, we could
just use write() and close() in this function rather
than doing an fdopen() to use fwrite() and fclose().
But I can see the attraction of not doing that in this
patch for review purposes.

>      if (!f) {
>          fprintf(stderr, "Couldn't open \"%s\": %s", fname, strerror(errno));
>          return 1;
> diff --git a/tests/boot-sector.h b/tests/boot-sector.h
> index f64b477..35d61c7 100644
> --- a/tests/boot-sector.h
> +++ b/tests/boot-sector.h
> @@ -14,8 +14,8 @@
>  #ifndef TEST_BOOT_SECTOR_H
>  #define TEST_BOOT_SECTOR_H
>
> -/* Create boot disk file.  */
> -int boot_sector_init(const char *fname);
> +/* Create boot disk file. fname must be a suitable string for mkstemp() */
> +int boot_sector_init(char *fname);
>
>  /* Loop until signature in memory is OK.  */
>  void boot_sector_test(void);
> diff --git a/tests/pxe-test.c b/tests/pxe-test.c
> index 5d3ddbe..34282d3 100644
> --- a/tests/pxe-test.c
> +++ b/tests/pxe-test.c
> @@ -19,7 +19,7 @@
>
>  #define NETNAME "net0"
>
> -static const char *disk = "tests/pxe-test-disk.raw";
> +static char disk[] = "tests/pxe-test-disk-XXXXXX";
>
>  static void test_pxe_one(const char *params, bool ipv6)
>  {

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH] tests/boot-sector: Use mkstemp() to create a unique file name

[Greg Kurz]

On Tue, 11 Oct 2016 15:32:02 +0200
Thomas Huth <thuth@redhat.com> wrote:

> The pxe-test is run for three different targets now (x86_64, i386
> and ppc64), and the bios-tables-test is run for two targets (x86_64
> and i386). But each of the tests is using an invariant name for the
> disk image with the boot sector code - so if the tests are running
> in parallel, there is a race condition that they destroy the disk
> image of a parallel test program. Let's use mkstemp() to create
> unique temporary files here instead.
> 
> Reported-by: Sascha Silbe <x-qemu@se-silbe.de>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/bios-tables-test.c | 2 +-
>  tests/boot-sector.c      | 9 +++++++--
>  tests/boot-sector.h      | 4 ++--
>  tests/pxe-test.c         | 2 +-
>  4 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 6ea2b6d..812f830 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -112,7 +112,7 @@ typedef struct {
>      g_assert_cmpstr(ACPI_ASSERT_CMP_str, ==, expected); \
>  } while (0)
>  
> -static const char *disk = "tests/acpi-test-disk.raw";
> +static char disk[] = "tests/acpi-test-disk-XXXXXX";
>  static const char *data_dir = "tests/acpi-test-data";
>  #ifdef CONFIG_IASL
>  static const char *iasl = stringify(CONFIG_IASL);
> diff --git a/tests/boot-sector.c b/tests/boot-sector.c
> index e3193c0..e75572f 100644
> --- a/tests/boot-sector.c
> +++ b/tests/boot-sector.c
> @@ -69,10 +69,15 @@ static uint8_t boot_sector[0x7e000] = {
>  };
>  
>  /* Create boot disk file.  */
> -int boot_sector_init(const char *fname)
> +int boot_sector_init(char *fname)
>  {
> -    FILE *f = fopen(fname, "w");
> +    FILE *f = NULL;
> +    int fd;
>  
> +    fd = mkstemp(fname);
> +    if (fd != -1) {
> +        f = fdopen(fd, "w");
> +    }

Unless we really want to control how the temporary file is named (maybe for
debugging purpose?), I have the impression that using tmpfile(3) would result
in a simpler patch.

Cheers.

--
Greg

>      if (!f) {
>          fprintf(stderr, "Couldn't open \"%s\": %s", fname, strerror(errno));
>          return 1;
> diff --git a/tests/boot-sector.h b/tests/boot-sector.h
> index f64b477..35d61c7 100644
> --- a/tests/boot-sector.h
> +++ b/tests/boot-sector.h
> @@ -14,8 +14,8 @@
>  #ifndef TEST_BOOT_SECTOR_H
>  #define TEST_BOOT_SECTOR_H
>  
> -/* Create boot disk file.  */
> -int boot_sector_init(const char *fname);
> +/* Create boot disk file. fname must be a suitable string for mkstemp() */
> +int boot_sector_init(char *fname);
>  
>  /* Loop until signature in memory is OK.  */
>  void boot_sector_test(void);
> diff --git a/tests/pxe-test.c b/tests/pxe-test.c
> index 5d3ddbe..34282d3 100644
> --- a/tests/pxe-test.c
> +++ b/tests/pxe-test.c
> @@ -19,7 +19,7 @@
>  
>  #define NETNAME "net0"
>  
> -static const char *disk = "tests/pxe-test-disk.raw";
> +static char disk[] = "tests/pxe-test-disk-XXXXXX";
>  
>  static void test_pxe_one(const char *params, bool ipv6)
>  {

Re: [Qemu-devel] [PATCH] tests/boot-sector: Use mkstemp() to create a unique file name

[Thomas Huth]

On 11.10.2016 15:55, Greg Kurz wrote:
> On Tue, 11 Oct 2016 15:32:02 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> The pxe-test is run for three different targets now (x86_64, i386
>> and ppc64), and the bios-tables-test is run for two targets (x86_64
>> and i386). But each of the tests is using an invariant name for the
>> disk image with the boot sector code - so if the tests are running
>> in parallel, there is a race condition that they destroy the disk
>> image of a parallel test program. Let's use mkstemp() to create
>> unique temporary files here instead.
>>
>> Reported-by: Sascha Silbe <x-qemu@se-silbe.de>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  tests/bios-tables-test.c | 2 +-
>>  tests/boot-sector.c      | 9 +++++++--
>>  tests/boot-sector.h      | 4 ++--
>>  tests/pxe-test.c         | 2 +-
>>  4 files changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
>> index 6ea2b6d..812f830 100644
>> --- a/tests/bios-tables-test.c
>> +++ b/tests/bios-tables-test.c
>> @@ -112,7 +112,7 @@ typedef struct {
>>      g_assert_cmpstr(ACPI_ASSERT_CMP_str, ==, expected); \
>>  } while (0)
>>  
>> -static const char *disk = "tests/acpi-test-disk.raw";
>> +static char disk[] = "tests/acpi-test-disk-XXXXXX";
>>  static const char *data_dir = "tests/acpi-test-data";
>>  #ifdef CONFIG_IASL
>>  static const char *iasl = stringify(CONFIG_IASL);
>> diff --git a/tests/boot-sector.c b/tests/boot-sector.c
>> index e3193c0..e75572f 100644
>> --- a/tests/boot-sector.c
>> +++ b/tests/boot-sector.c
>> @@ -69,10 +69,15 @@ static uint8_t boot_sector[0x7e000] = {
>>  };
>>  
>>  /* Create boot disk file.  */
>> -int boot_sector_init(const char *fname)
>> +int boot_sector_init(char *fname)
>>  {
>> -    FILE *f = fopen(fname, "w");
>> +    FILE *f = NULL;
>> +    int fd;
>>  
>> +    fd = mkstemp(fname);
>> +    if (fd != -1) {
>> +        f = fdopen(fd, "w");
>> +    }
> 
> Unless we really want to control how the temporary file is named (maybe for
> debugging purpose?), I have the impression that using tmpfile(3) would result
> in a simpler patch.

That unfortunately does not work here - the file name is needed for
starting the QEMU process later (see test_acpi_one() or test_pxe_one()
for details).

 Thomas

Re: [Qemu-devel] [PATCH] tests/boot-sector: Use mkstemp() to create a unique file name

[Thomas Huth]

On 11.10.2016 15:38, Peter Maydell wrote:
> On 11 October 2016 at 14:32, Thomas Huth <thuth@redhat.com> wrote:
>> The pxe-test is run for three different targets now (x86_64, i386
>> and ppc64), and the bios-tables-test is run for two targets (x86_64
>> and i386). But each of the tests is using an invariant name for the
>> disk image with the boot sector code - so if the tests are running
>> in parallel, there is a race condition that they destroy the disk
>> image of a parallel test program. Let's use mkstemp() to create
>> unique temporary files here instead.
>>
>> Reported-by: Sascha Silbe <x-qemu@se-silbe.de>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  tests/bios-tables-test.c | 2 +-
>>  tests/boot-sector.c      | 9 +++++++--
>>  tests/boot-sector.h      | 4 ++--
>>  tests/pxe-test.c         | 2 +-
>>  4 files changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
>> index 6ea2b6d..812f830 100644
>> --- a/tests/bios-tables-test.c
>> +++ b/tests/bios-tables-test.c
>> @@ -112,7 +112,7 @@ typedef struct {
>>      g_assert_cmpstr(ACPI_ASSERT_CMP_str, ==, expected); \
>>  } while (0)
>>
>> -static const char *disk = "tests/acpi-test-disk.raw";
>> +static char disk[] = "tests/acpi-test-disk-XXXXXX";
>>  static const char *data_dir = "tests/acpi-test-data";
>>  #ifdef CONFIG_IASL
>>  static const char *iasl = stringify(CONFIG_IASL);
>> diff --git a/tests/boot-sector.c b/tests/boot-sector.c
>> index e3193c0..e75572f 100644
>> --- a/tests/boot-sector.c
>> +++ b/tests/boot-sector.c
>> @@ -69,10 +69,15 @@ static uint8_t boot_sector[0x7e000] = {
>>  };
>>
>>  /* Create boot disk file.  */
>> -int boot_sector_init(const char *fname)
>> +int boot_sector_init(char *fname)
>>  {
>> -    FILE *f = fopen(fname, "w");
>> +    FILE *f = NULL;
>> +    int fd;
>>
>> +    fd = mkstemp(fname);
>> +    if (fd != -1) {
>> +        f = fdopen(fd, "w");
>> +    }
> 
> Given that we're only writing a pile of bytes, we could
> just use write() and close() in this function rather
> than doing an fdopen() to use fwrite() and fclose().

You're right, that sounds nicer, so please ignore this version, I'll
send an update.

I think we might also need to increase the default timeout in
boot_sector_test() a little bit, since the pxe test is really rather
slow on ppc64 ... by increasing the timeout, we can make sure that it
also still passes on machines with high CPU load, so I'll add a patch to
do that, too.

 Thomas

Re: [Qemu-devel] [PATCH] tests/boot-sector: Use mkstemp() to create a unique file name

[Fam Zheng]

On Tue, 10/11 15:32, Thomas Huth wrote:
> The pxe-test is run for three different targets now (x86_64, i386
> and ppc64), and the bios-tables-test is run for two targets (x86_64
> and i386). But each of the tests is using an invariant name for the
> disk image with the boot sector code - so if the tests are running
> in parallel, there is a race condition that they destroy the disk
> image of a parallel test program. Let's use mkstemp() to create
> unique temporary files here instead.
> 
> Reported-by: Sascha Silbe <x-qemu@se-silbe.de>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Nice, this does help fix the failure that patchew hits with "make
docker-test-clang@ubuntu J=8".

Tested-by: Fam Zheng <famz@redhat.com>

> ---
>  tests/bios-tables-test.c | 2 +-
>  tests/boot-sector.c      | 9 +++++++--
>  tests/boot-sector.h      | 4 ++--
>  tests/pxe-test.c         | 2 +-
>  4 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 6ea2b6d..812f830 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -112,7 +112,7 @@ typedef struct {
>      g_assert_cmpstr(ACPI_ASSERT_CMP_str, ==, expected); \
>  } while (0)
>  
> -static const char *disk = "tests/acpi-test-disk.raw";
> +static char disk[] = "tests/acpi-test-disk-XXXXXX";
>  static const char *data_dir = "tests/acpi-test-data";
>  #ifdef CONFIG_IASL
>  static const char *iasl = stringify(CONFIG_IASL);
> diff --git a/tests/boot-sector.c b/tests/boot-sector.c
> index e3193c0..e75572f 100644
> --- a/tests/boot-sector.c
> +++ b/tests/boot-sector.c
> @@ -69,10 +69,15 @@ static uint8_t boot_sector[0x7e000] = {
>  };
>  
>  /* Create boot disk file.  */
> -int boot_sector_init(const char *fname)
> +int boot_sector_init(char *fname)
>  {
> -    FILE *f = fopen(fname, "w");
> +    FILE *f = NULL;
> +    int fd;
>  
> +    fd = mkstemp(fname);
> +    if (fd != -1) {
> +        f = fdopen(fd, "w");
> +    }
>      if (!f) {
>          fprintf(stderr, "Couldn't open \"%s\": %s", fname, strerror(errno));
>          return 1;
> diff --git a/tests/boot-sector.h b/tests/boot-sector.h
> index f64b477..35d61c7 100644
> --- a/tests/boot-sector.h
> +++ b/tests/boot-sector.h
> @@ -14,8 +14,8 @@
>  #ifndef TEST_BOOT_SECTOR_H
>  #define TEST_BOOT_SECTOR_H
>  
> -/* Create boot disk file.  */
> -int boot_sector_init(const char *fname);
> +/* Create boot disk file. fname must be a suitable string for mkstemp() */
> +int boot_sector_init(char *fname);
>  
>  /* Loop until signature in memory is OK.  */
>  void boot_sector_test(void);
> diff --git a/tests/pxe-test.c b/tests/pxe-test.c
> index 5d3ddbe..34282d3 100644
> --- a/tests/pxe-test.c
> +++ b/tests/pxe-test.c
> @@ -19,7 +19,7 @@
>  
>  #define NETNAME "net0"
>  
> -static const char *disk = "tests/pxe-test-disk.raw";
> +static char disk[] = "tests/pxe-test-disk-XXXXXX";
>  
>  static void test_pxe_one(const char *params, bool ipv6)
>  {
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH] tests/boot-sector: Use mkstemp() to create a unique file name

[Greg Kurz]

On Tue, 11 Oct 2016 16:06:55 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 11.10.2016 15:55, Greg Kurz wrote:
> > On Tue, 11 Oct 2016 15:32:02 +0200
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> The pxe-test is run for three different targets now (x86_64, i386
> >> and ppc64), and the bios-tables-test is run for two targets (x86_64
> >> and i386). But each of the tests is using an invariant name for the
> >> disk image with the boot sector code - so if the tests are running
> >> in parallel, there is a race condition that they destroy the disk
> >> image of a parallel test program. Let's use mkstemp() to create
> >> unique temporary files here instead.
> >>
> >> Reported-by: Sascha Silbe <x-qemu@se-silbe.de>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >>  tests/bios-tables-test.c | 2 +-
> >>  tests/boot-sector.c      | 9 +++++++--
> >>  tests/boot-sector.h      | 4 ++--
> >>  tests/pxe-test.c         | 2 +-
> >>  4 files changed, 11 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> >> index 6ea2b6d..812f830 100644
> >> --- a/tests/bios-tables-test.c
> >> +++ b/tests/bios-tables-test.c
> >> @@ -112,7 +112,7 @@ typedef struct {
> >>      g_assert_cmpstr(ACPI_ASSERT_CMP_str, ==, expected); \
> >>  } while (0)
> >>  
> >> -static const char *disk = "tests/acpi-test-disk.raw";
> >> +static char disk[] = "tests/acpi-test-disk-XXXXXX";
> >>  static const char *data_dir = "tests/acpi-test-data";
> >>  #ifdef CONFIG_IASL
> >>  static const char *iasl = stringify(CONFIG_IASL);
> >> diff --git a/tests/boot-sector.c b/tests/boot-sector.c
> >> index e3193c0..e75572f 100644
> >> --- a/tests/boot-sector.c
> >> +++ b/tests/boot-sector.c
> >> @@ -69,10 +69,15 @@ static uint8_t boot_sector[0x7e000] = {
> >>  };
> >>  
> >>  /* Create boot disk file.  */
> >> -int boot_sector_init(const char *fname)
> >> +int boot_sector_init(char *fname)
> >>  {
> >> -    FILE *f = fopen(fname, "w");
> >> +    FILE *f = NULL;
> >> +    int fd;
> >>  
> >> +    fd = mkstemp(fname);
> >> +    if (fd != -1) {
> >> +        f = fdopen(fd, "w");
> >> +    }  
> > 
> > Unless we really want to control how the temporary file is named (maybe for
> > debugging purpose?), I have the impression that using tmpfile(3) would result
> > in a simpler patch.  
> 
> That unfortunately does not work here - the file name is needed for
> starting the QEMU process later (see test_acpi_one() or test_pxe_one()
> for details).
> 

Oops you're right *of course*.

Then I guess Peter's remark on fwrite()/fclose() should also be addressed, but I
see you already sent an update for that :)

>  Thomas
> 

Cheers.

--
Greg