| Message ID | 1476192722-22871-1-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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) > {
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
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
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 > >
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
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) {
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(-)