diff mbox series

[1/2] tests/9pfs: fix test dir for parallel tests

Message ID 73a2acc5ca29f95d6d2e5ce60ec69c62bd55f637.1604046404.git.qemu_oss@crudebyte.com
State New
Headers show
Series [1/2] tests/9pfs: fix test dir for parallel tests | expand

Commit Message

Christian Schoenebeck Oct. 30, 2020, 8:19 a.m. UTC
Use mkdtemp() to generate a unique directory for the 9p 'local' tests.

This fixes occasional 9p test failures when running 'make check -jN' if
QEMU was compiled for multiple target architectures, because the individual
architecture's test suites would run in parallel and interfere with each
other's data as the test directory was previously hard coded and hence the
same directory was used by all of them simultaniously.

This also requires a change how the test directory is created and deleted:
As the test path is now randomized and virtio_9p_register_nodes() being
called in a somewhat undeterministic way, that's no longer an appropriate
place to create and remove the test directory. Use a constructor and
destructor function for creating and removing the test directory instead.
Unfortunately libqos currently does not support setup/teardown callbacks
to handle this more cleanly.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/libqos/virtio-9p.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

Comments

Greg Kurz Oct. 30, 2020, 11:32 a.m. UTC | #1
On Fri, 30 Oct 2020 09:19:46 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> Use mkdtemp() to generate a unique directory for the 9p 'local' tests.

> 

> This fixes occasional 9p test failures when running 'make check -jN' if

> QEMU was compiled for multiple target architectures, because the individual

> architecture's test suites would run in parallel and interfere with each

> other's data as the test directory was previously hard coded and hence the

> same directory was used by all of them simultaniously.

> 

> This also requires a change how the test directory is created and deleted:

> As the test path is now randomized and virtio_9p_register_nodes() being

> called in a somewhat undeterministic way, that's no longer an appropriate

> place to create and remove the test directory. Use a constructor and

> destructor function for creating and removing the test directory instead.

> Unfortunately libqos currently does not support setup/teardown callbacks

> to handle this more cleanly.

> 

> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

> ---


LGTM

I've been running 'make check-qtest -j' with 4 archs for 2 hours without
hitting the issue.

Tested-by: Greg Kurz <groug@kaod.org>


>  tests/qtest/libqos/virtio-9p.c | 25 +++++++++++++++++++------

>  1 file changed, 19 insertions(+), 6 deletions(-)

> 

> diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c

> index d43647b3b7..6b22fa0e9a 100644

> --- a/tests/qtest/libqos/virtio-9p.c

> +++ b/tests/qtest/libqos/virtio-9p.c

> @@ -35,7 +35,12 @@ static char *concat_path(const char* a, const char* b)

>  static void init_local_test_path(void)

>  {

>      char *pwd = g_get_current_dir();

> -    local_test_path = concat_path(pwd, "qtest-9p-local");

> +    char *template = concat_path(pwd, "qtest-9p-local-XXXXXX");

> +    local_test_path = mkdtemp(template);

> +    if (!local_test_path) {

> +        g_test_message("mkdtemp('%s') failed: %s", template, strerror(errno));


Just per curiosity, is there a preferred way to output error messages ?

Cc'ing Thomas and Laurent.

[...]
Thomas Huth Nov. 9, 2020, 11:51 a.m. UTC | #2
On 30/10/2020 12.32, Greg Kurz wrote:
> On Fri, 30 Oct 2020 09:19:46 +0100

> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

[...]
>> diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c

>> index d43647b3b7..6b22fa0e9a 100644

>> --- a/tests/qtest/libqos/virtio-9p.c

>> +++ b/tests/qtest/libqos/virtio-9p.c

>> @@ -35,7 +35,12 @@ static char *concat_path(const char* a, const char* b)

>>  static void init_local_test_path(void)

>>  {

>>      char *pwd = g_get_current_dir();

>> -    local_test_path = concat_path(pwd, "qtest-9p-local");

>> +    char *template = concat_path(pwd, "qtest-9p-local-XXXXXX");

>> +    local_test_path = mkdtemp(template);

>> +    if (!local_test_path) {

>> +        g_test_message("mkdtemp('%s') failed: %s", template, strerror(errno));

> 

> Just per curiosity, is there a preferred way to output error messages ?


I don't think that we've ever agreed on a common way here... but as far as I
can see, most tests rather use fprintf(stderr, ...) for error messages,
while g_test_message() is rather for normal log messages?

 Thomas
diff mbox series

Patch

diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index d43647b3b7..6b22fa0e9a 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -35,7 +35,12 @@  static char *concat_path(const char* a, const char* b)
 static void init_local_test_path(void)
 {
     char *pwd = g_get_current_dir();
-    local_test_path = concat_path(pwd, "qtest-9p-local");
+    char *template = concat_path(pwd, "qtest-9p-local-XXXXXX");
+    local_test_path = mkdtemp(template);
+    if (!local_test_path) {
+        g_test_message("mkdtemp('%s') failed: %s", template, strerror(errno));
+    }
+    g_assert(local_test_path);
     g_free(pwd);
 }
 
@@ -246,11 +251,6 @@  static void virtio_9p_register_nodes(void)
     const char *str_simple = "fsdev=fsdev0,mount_tag=" MOUNT_TAG;
     const char *str_addr = "fsdev=fsdev0,addr=04.0,mount_tag=" MOUNT_TAG;
 
-    /* make sure test dir for the 'local' tests exists and is clean */
-    init_local_test_path();
-    remove_local_test_dir();
-    create_local_test_dir();
-
     QPCIAddress addr = {
         .devfn = QPCI_DEVFN(4, 0),
     };
@@ -278,3 +278,16 @@  static void virtio_9p_register_nodes(void)
 }
 
 libqos_init(virtio_9p_register_nodes);
+
+static void __attribute__((constructor)) construct_virtio_9p(void)
+{
+    /* make sure test dir for the 'local' tests exists */
+    init_local_test_path();
+    create_local_test_dir();
+}
+
+static void __attribute__((destructor)) destruct_virtio_9p(void)
+{
+    /* remove previously created test dir when test suite completed */
+    remove_local_test_dir();
+}