qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 1/2] tests/9pfs: make create/remove test dir public
  2020-11-01 15:12 [PATCH v3 0/2] 9pfs: test suite fixes Christian Schoenebeck
@ 2020-11-01 14:25 ` Christian Schoenebeck
  2020-11-01 17:03   ` Greg Kurz
  2020-11-01 14:37 ` [PATCH v3 2/2] tests/9pfs: fix test dir for parallel tests Christian Schoenebeck
  2020-11-01 18:02 ` [PATCH v3 0/2] 9pfs: test suite fixes Mark Cave-Ayland
  2 siblings, 1 reply; 10+ messages in thread
From: Christian Schoenebeck @ 2020-11-01 14:25 UTC (permalink / raw)
  To: qemu-devel; +Cc: Greg Kurz

Make functions create_local_test_dir() and remove_local_test_dir()
public. They're going to be used in the next patch.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/libqos/virtio-9p.c | 10 ++++------
 tests/qtest/libqos/virtio-9p.h | 10 ++++++++++
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index d43647b3b7..2736e9ae2a 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -39,8 +39,7 @@ static void init_local_test_path(void)
     g_free(pwd);
 }
 
-/* Creates the directory for the 9pfs 'local' filesystem driver to access. */
-static void create_local_test_dir(void)
+void virtio_9p_create_local_test_dir(void)
 {
     struct stat st;
 
@@ -53,8 +52,7 @@ static void create_local_test_dir(void)
     g_assert((st.st_mode & S_IFMT) == S_IFDIR);
 }
 
-/* Deletes directory previously created by create_local_test_dir(). */
-static void remove_local_test_dir(void)
+void virtio_9p_remove_local_test_dir(void)
 {
     g_assert(local_test_path != NULL);
     char *cmd = g_strdup_printf("rm -r '%s'\n", local_test_path);
@@ -248,8 +246,8 @@ static void virtio_9p_register_nodes(void)
 
     /* make sure test dir for the 'local' tests exists and is clean */
     init_local_test_path();
-    remove_local_test_dir();
-    create_local_test_dir();
+    virtio_9p_remove_local_test_dir();
+    virtio_9p_create_local_test_dir();
 
     QPCIAddress addr = {
         .devfn = QPCI_DEVFN(4, 0),
diff --git a/tests/qtest/libqos/virtio-9p.h b/tests/qtest/libqos/virtio-9p.h
index 19a4d97454..480727120e 100644
--- a/tests/qtest/libqos/virtio-9p.h
+++ b/tests/qtest/libqos/virtio-9p.h
@@ -44,6 +44,16 @@ struct QVirtio9PDevice {
     QVirtio9P v9p;
 };
 
+/**
+ * Creates the directory for the 9pfs 'local' filesystem driver to access.
+ */
+void virtio_9p_create_local_test_dir(void);
+
+/**
+ * Deletes directory previously created by virtio_9p_create_local_test_dir().
+ */
+void virtio_9p_remove_local_test_dir(void);
+
 /**
  * Prepares QEMU command line for 9pfs tests using the 'local' fs driver.
  */
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 2/2] tests/9pfs: fix test dir for parallel tests
  2020-11-01 15:12 [PATCH v3 0/2] 9pfs: test suite fixes Christian Schoenebeck
  2020-11-01 14:25 ` [PATCH v3 1/2] tests/9pfs: make create/remove test dir public Christian Schoenebeck
@ 2020-11-01 14:37 ` Christian Schoenebeck
  2020-11-01 17:44   ` Greg Kurz
  2020-11-01 18:02 ` [PATCH v3 0/2] 9pfs: test suite fixes Mark Cave-Ayland
  2 siblings, 1 reply; 10+ messages in thread
From: Christian Schoenebeck @ 2020-11-01 14:37 UTC (permalink / raw)
  To: qemu-devel; +Cc: Greg Kurz

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.

The constructor functions needs to be in virtio-9p-test.c, not in
virtio-9p.c, because in the latter location it would cause all apps that
link to libqos (i.e. entirely unrelated test suites) to create a 9pfs
test directory as well, which would even break other test suites.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/libqos/virtio-9p.c | 14 ++++++++------
 tests/qtest/virtio-9p-test.c   | 12 ++++++++++++
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index 2736e9ae2a..586e700b24 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);
 }
 
@@ -43,6 +48,8 @@ void virtio_9p_create_local_test_dir(void)
 {
     struct stat st;
 
+    init_local_test_path();
+
     g_assert(local_test_path != NULL);
     mkdir(local_test_path, 0777);
 
@@ -244,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();
-    virtio_9p_remove_local_test_dir();
-    virtio_9p_create_local_test_dir();
-
     QPCIAddress addr = {
         .devfn = QPCI_DEVFN(4, 0),
     };
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index c15908f27b..6401d4f564 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -1076,3 +1076,15 @@ static void register_virtio_9p_test(void)
 }
 
 libqos_init(register_virtio_9p_test);
+
+static void __attribute__((constructor)) construct_9p_test(void)
+{
+    /* make sure test dir for the 'local' tests exists */
+    virtio_9p_create_local_test_dir();
+}
+
+static void __attribute__((destructor)) destruct_9p_test(void)
+{
+    /* remove previously created test dir when test suite completed */
+    virtio_9p_remove_local_test_dir();
+}
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 0/2] 9pfs: test suite fixes
@ 2020-11-01 15:12 Christian Schoenebeck
  2020-11-01 14:25 ` [PATCH v3 1/2] tests/9pfs: make create/remove test dir public Christian Schoenebeck
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Christian Schoenebeck @ 2020-11-01 15:12 UTC (permalink / raw)
  To: qemu-devel; +Cc: Greg Kurz

Fixes test failures with the 9pfs 'local' tests as discussed with latest
9P PR. See the discussion of that PR v2 (Fri, Oct 30th) for details.

In conjunction with Peter Xu's two migration patches (fixing occasional
lockups of migration tests) overall situation appears to be smooth now:
https://lore.kernel.org/qemu-devel/20201030135350.GA588069@xz-x1/

v2->v3:

  - Make the two functions for creating and removing the 9pfs test directory
    public [NEW patch 1].

  - Place the constructor and destructor functions in virtio-9p-test.c, not
    in virtio-9p.c, because the latter location would cause the constructor
    to be executed whenever libqos is loaded, which would break other,
    completely unrelated tests suites that just link to libqos [patch 2].

  - Previous patch 2 (coverity fix) is already queued, no changes, hence
    omitted in this v3.

v1->v2:

  - Added Greg's tested-by tag [patch 1].

  - Log an info-level message if mkdir() failed [patch 2].

  - Update commit log message about coverity being the reporter and
    details of the coverity report [patch 2].

Christian Schoenebeck (2):
  tests/9pfs: make create/remove test dir public
  tests/9pfs: fix test dir for parallel tests

 tests/qtest/libqos/virtio-9p.c | 20 ++++++++++----------
 tests/qtest/libqos/virtio-9p.h | 10 ++++++++++
 tests/qtest/virtio-9p-test.c   | 12 ++++++++++++
 3 files changed, 32 insertions(+), 10 deletions(-)

-- 
2.20.1



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 1/2] tests/9pfs: make create/remove test dir public
  2020-11-01 14:25 ` [PATCH v3 1/2] tests/9pfs: make create/remove test dir public Christian Schoenebeck
@ 2020-11-01 17:03   ` Greg Kurz
  0 siblings, 0 replies; 10+ messages in thread
From: Greg Kurz @ 2020-11-01 17:03 UTC (permalink / raw)
  To: Christian Schoenebeck; +Cc: qemu-devel

On Sun, 1 Nov 2020 15:25:14 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> Make functions create_local_test_dir() and remove_local_test_dir()
> public. They're going to be used in the next patch.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---

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

>  tests/qtest/libqos/virtio-9p.c | 10 ++++------
>  tests/qtest/libqos/virtio-9p.h | 10 ++++++++++
>  2 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
> index d43647b3b7..2736e9ae2a 100644
> --- a/tests/qtest/libqos/virtio-9p.c
> +++ b/tests/qtest/libqos/virtio-9p.c
> @@ -39,8 +39,7 @@ static void init_local_test_path(void)
>      g_free(pwd);
>  }
>  
> -/* Creates the directory for the 9pfs 'local' filesystem driver to access. */
> -static void create_local_test_dir(void)
> +void virtio_9p_create_local_test_dir(void)
>  {
>      struct stat st;
>  
> @@ -53,8 +52,7 @@ static void create_local_test_dir(void)
>      g_assert((st.st_mode & S_IFMT) == S_IFDIR);
>  }
>  
> -/* Deletes directory previously created by create_local_test_dir(). */
> -static void remove_local_test_dir(void)
> +void virtio_9p_remove_local_test_dir(void)
>  {
>      g_assert(local_test_path != NULL);
>      char *cmd = g_strdup_printf("rm -r '%s'\n", local_test_path);
> @@ -248,8 +246,8 @@ static void virtio_9p_register_nodes(void)
>  
>      /* make sure test dir for the 'local' tests exists and is clean */
>      init_local_test_path();
> -    remove_local_test_dir();
> -    create_local_test_dir();
> +    virtio_9p_remove_local_test_dir();
> +    virtio_9p_create_local_test_dir();
>  
>      QPCIAddress addr = {
>          .devfn = QPCI_DEVFN(4, 0),
> diff --git a/tests/qtest/libqos/virtio-9p.h b/tests/qtest/libqos/virtio-9p.h
> index 19a4d97454..480727120e 100644
> --- a/tests/qtest/libqos/virtio-9p.h
> +++ b/tests/qtest/libqos/virtio-9p.h
> @@ -44,6 +44,16 @@ struct QVirtio9PDevice {
>      QVirtio9P v9p;
>  };
>  
> +/**
> + * Creates the directory for the 9pfs 'local' filesystem driver to access.
> + */
> +void virtio_9p_create_local_test_dir(void);
> +
> +/**
> + * Deletes directory previously created by virtio_9p_create_local_test_dir().
> + */
> +void virtio_9p_remove_local_test_dir(void);
> +
>  /**
>   * Prepares QEMU command line for 9pfs tests using the 'local' fs driver.
>   */



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 2/2] tests/9pfs: fix test dir for parallel tests
  2020-11-01 14:37 ` [PATCH v3 2/2] tests/9pfs: fix test dir for parallel tests Christian Schoenebeck
@ 2020-11-01 17:44   ` Greg Kurz
  2020-11-01 19:14     ` Christian Schoenebeck
  0 siblings, 1 reply; 10+ messages in thread
From: Greg Kurz @ 2020-11-01 17:44 UTC (permalink / raw)
  To: Christian Schoenebeck; +Cc: qemu-devel

On Sun, 1 Nov 2020 15:37:12 +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.
> 
> The constructor functions needs to be in virtio-9p-test.c, not in
> virtio-9p.c, because in the latter location it would cause all apps that
> link to libqos (i.e. entirely unrelated test suites) to create a 9pfs
> test directory as well, which would even break other test suites.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---

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

I could run 'make check -j' with 4 archs (ppc64, x86_64, aarch64, s390x)
on a POWER9 system with 128 cpus, for ~1 hour without seeing any failure.

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

>  tests/qtest/libqos/virtio-9p.c | 14 ++++++++------
>  tests/qtest/virtio-9p-test.c   | 12 ++++++++++++
>  2 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
> index 2736e9ae2a..586e700b24 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);
>  }
>  
> @@ -43,6 +48,8 @@ void virtio_9p_create_local_test_dir(void)
>  {
>      struct stat st;
>  
> +    init_local_test_path();
> +
>      g_assert(local_test_path != NULL);
>      mkdir(local_test_path, 0777);
>  
> @@ -244,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();
> -    virtio_9p_remove_local_test_dir();
> -    virtio_9p_create_local_test_dir();
> -
>      QPCIAddress addr = {
>          .devfn = QPCI_DEVFN(4, 0),
>      };
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index c15908f27b..6401d4f564 100644
> --- a/tests/qtest/virtio-9p-test.c
> +++ b/tests/qtest/virtio-9p-test.c
> @@ -1076,3 +1076,15 @@ static void register_virtio_9p_test(void)
>  }
>  
>  libqos_init(register_virtio_9p_test);
> +
> +static void __attribute__((constructor)) construct_9p_test(void)
> +{
> +    /* make sure test dir for the 'local' tests exists */
> +    virtio_9p_create_local_test_dir();
> +}
> +
> +static void __attribute__((destructor)) destruct_9p_test(void)
> +{
> +    /* remove previously created test dir when test suite completed */
> +    virtio_9p_remove_local_test_dir();
> +}



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 0/2] 9pfs: test suite fixes
  2020-11-01 15:12 [PATCH v3 0/2] 9pfs: test suite fixes Christian Schoenebeck
  2020-11-01 14:25 ` [PATCH v3 1/2] tests/9pfs: make create/remove test dir public Christian Schoenebeck
  2020-11-01 14:37 ` [PATCH v3 2/2] tests/9pfs: fix test dir for parallel tests Christian Schoenebeck
@ 2020-11-01 18:02 ` Mark Cave-Ayland
  2020-11-01 19:17   ` Christian Schoenebeck
  2 siblings, 1 reply; 10+ messages in thread
From: Mark Cave-Ayland @ 2020-11-01 18:02 UTC (permalink / raw)
  To: Christian Schoenebeck, qemu-devel; +Cc: Greg Kurz

On 01/11/2020 15:12, Christian Schoenebeck wrote:

> Fixes test failures with the 9pfs 'local' tests as discussed with latest
> 9P PR. See the discussion of that PR v2 (Fri, Oct 30th) for details.
> 
> In conjunction with Peter Xu's two migration patches (fixing occasional
> lockups of migration tests) overall situation appears to be smooth now:
> https://lore.kernel.org/qemu-devel/20201030135350.GA588069@xz-x1/
> 
> v2->v3:
> 
>    - Make the two functions for creating and removing the 9pfs test directory
>      public [NEW patch 1].
> 
>    - Place the constructor and destructor functions in virtio-9p-test.c, not
>      in virtio-9p.c, because the latter location would cause the constructor
>      to be executed whenever libqos is loaded, which would break other,
>      completely unrelated tests suites that just link to libqos [patch 2].
> 
>    - Previous patch 2 (coverity fix) is already queued, no changes, hence
>      omitted in this v3.
> 
> v1->v2:
> 
>    - Added Greg's tested-by tag [patch 1].
> 
>    - Log an info-level message if mkdir() failed [patch 2].
> 
>    - Update commit log message about coverity being the reporter and
>      details of the coverity report [patch 2].
> 
> Christian Schoenebeck (2):
>    tests/9pfs: make create/remove test dir public
>    tests/9pfs: fix test dir for parallel tests
> 
>   tests/qtest/libqos/virtio-9p.c | 20 ++++++++++----------
>   tests/qtest/libqos/virtio-9p.h | 10 ++++++++++
>   tests/qtest/virtio-9p-test.c   | 12 ++++++++++++
>   3 files changed, 32 insertions(+), 10 deletions(-)

FWIW one thing I've noticed recently is that my builds for qemu-system-sparc64 have 
started giving this warning about a missing "qtest-9p-local" directory during make check:

...
...
Running test QAPI schema regression tests
Running test qtest-sparc64/endianness-test
Running test qtest-sparc64/prom-env-test
Running test qtest-sparc64/boot-serial-test
Running test qtest-sparc64/cdrom-test
Running test qtest-sparc64/device-introspect-test
Running test qtest-sparc64/machine-none-test
Running test qtest-sparc64/qmp-test
Running test qtest-sparc64/qmp-cmd-test
Running test qtest-sparc64/qom-test
Running test qtest-sparc64/test-hmp
Running test qtest-sparc64/qos-test
rm: cannot remove '/home/build/src/qemu/git/qemu/build/qtest-9p-local': No such file 
or directory
   TEST    iotest-qcow2: 001
   TEST    iotest-qcow2: 002
   TEST    iotest-qcow2: 003
   TEST    iotest-qcow2: 004
   TEST    iotest-qcow2: 005
...
...

Would this get resolved by the changes to the test directory in this patchset? The 
build is a simple configure run with "--target-list=sparc64-softmmu".


ATB,

Mark.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 2/2] tests/9pfs: fix test dir for parallel tests
  2020-11-01 17:44   ` Greg Kurz
@ 2020-11-01 19:14     ` Christian Schoenebeck
  2020-11-01 19:31       ` Greg Kurz
  0 siblings, 1 reply; 10+ messages in thread
From: Christian Schoenebeck @ 2020-11-01 19:14 UTC (permalink / raw)
  To: qemu-devel; +Cc: Greg Kurz

On Sonntag, 1. November 2020 18:44:44 CET Greg Kurz wrote:
> On Sun, 1 Nov 2020 15:37:12 +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.
> > 
> > The constructor functions needs to be in virtio-9p-test.c, not in
> > virtio-9p.c, because in the latter location it would cause all apps that
> > link to libqos (i.e. entirely unrelated test suites) to create a 9pfs
> > test directory as well, which would even break other test suites.
> > 
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>

Thanks for the overtime, on a Sunday!

Queued on 9p.next:
https://github.com/cschoenebeck/qemu/commits/9p.next

And this one with Peter Xu's patches on top, just for testing:
https://github.com/cschoenebeck/qemu/commits/9p.experimental.2

> I could run 'make check -j' with 4 archs (ppc64, x86_64, aarch64, s390x)
> on a POWER9 system with 128 cpus, for ~1 hour without seeing any failure.
> 
> Tested-by: Greg Kurz <groug@kaod.org>

OO Sounds like having advantages working for IBM. Respect. I start to get envy 
as these beasts are running towards PCIe 6, while we regular x86 users would 
already be glad having PCIe 4.

I give it some more spinning hours this time, just to be sure, before sending 
the PR tomorrow morning. But I think it's all right now.

Thanks!

Best regards,
Christian Schoenebeck




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 0/2] 9pfs: test suite fixes
  2020-11-01 18:02 ` [PATCH v3 0/2] 9pfs: test suite fixes Mark Cave-Ayland
@ 2020-11-01 19:17   ` Christian Schoenebeck
  2020-11-01 19:35     ` Mark Cave-Ayland
  0 siblings, 1 reply; 10+ messages in thread
From: Christian Schoenebeck @ 2020-11-01 19:17 UTC (permalink / raw)
  To: qemu-devel; +Cc: Mark Cave-Ayland, Greg Kurz

On Sonntag, 1. November 2020 19:02:28 CET Mark Cave-Ayland wrote:
> On 01/11/2020 15:12, Christian Schoenebeck wrote:
> > Fixes test failures with the 9pfs 'local' tests as discussed with latest
> > 9P PR. See the discussion of that PR v2 (Fri, Oct 30th) for details.
> > 
> > In conjunction with Peter Xu's two migration patches (fixing occasional
> > lockups of migration tests) overall situation appears to be smooth now:
> > https://lore.kernel.org/qemu-devel/20201030135350.GA588069@xz-x1/
> > 
> > v2->v3:
> >    - Make the two functions for creating and removing the 9pfs test
> >    directory
> >    
> >      public [NEW patch 1].
> >    
> >    - Place the constructor and destructor functions in virtio-9p-test.c,
> >    not
> >    
> >      in virtio-9p.c, because the latter location would cause the
> >      constructor
> >      to be executed whenever libqos is loaded, which would break other,
> >      completely unrelated tests suites that just link to libqos [patch 2].
> >    
> >    - Previous patch 2 (coverity fix) is already queued, no changes, hence
> >    
> >      omitted in this v3.
> > 
> > v1->v2:
> >    - Added Greg's tested-by tag [patch 1].
> >    
> >    - Log an info-level message if mkdir() failed [patch 2].
> >    
> >    - Update commit log message about coverity being the reporter and
> >    
> >      details of the coverity report [patch 2].
> > 
> > Christian Schoenebeck (2):
> >    tests/9pfs: make create/remove test dir public
> >    tests/9pfs: fix test dir for parallel tests
> >   
> >   tests/qtest/libqos/virtio-9p.c | 20 ++++++++++----------
> >   tests/qtest/libqos/virtio-9p.h | 10 ++++++++++
> >   tests/qtest/virtio-9p-test.c   | 12 ++++++++++++
> >   3 files changed, 32 insertions(+), 10 deletions(-)
> 
> FWIW one thing I've noticed recently is that my builds for
> qemu-system-sparc64 have started giving this warning about a missing
> "qtest-9p-local" directory during make check:
> 
> ...
> ...
> Running test QAPI schema regression tests
> Running test qtest-sparc64/endianness-test
> Running test qtest-sparc64/prom-env-test
> Running test qtest-sparc64/boot-serial-test
> Running test qtest-sparc64/cdrom-test
> Running test qtest-sparc64/device-introspect-test
> Running test qtest-sparc64/machine-none-test
> Running test qtest-sparc64/qmp-test
> Running test qtest-sparc64/qmp-cmd-test
> Running test qtest-sparc64/qom-test
> Running test qtest-sparc64/test-hmp
> Running test qtest-sparc64/qos-test
> rm: cannot remove '/home/build/src/qemu/git/qemu/build/qtest-9p-local': No
> such file or directory
>    TEST    iotest-qcow2: 001
>    TEST    iotest-qcow2: 002
>    TEST    iotest-qcow2: 003
>    TEST    iotest-qcow2: 004
>    TEST    iotest-qcow2: 005
> ...
> ...
> 
> Would this get resolved by the changes to the test directory in this
> patchset? The build is a simple configure run with
> "--target-list=sparc64-softmmu".
> 
> 
> ATB,
> 
> Mark.

Yes, that should be resolved with the next 9p PR as well, additionally with 
the following patch that is:
https://github.com/cschoenebeck/qemu/commit/603cc76a6069

Thanks for the feedback!

Best regards,
Christian Schoenebeck




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 2/2] tests/9pfs: fix test dir for parallel tests
  2020-11-01 19:14     ` Christian Schoenebeck
@ 2020-11-01 19:31       ` Greg Kurz
  0 siblings, 0 replies; 10+ messages in thread
From: Greg Kurz @ 2020-11-01 19:31 UTC (permalink / raw)
  To: Christian Schoenebeck; +Cc: qemu-devel

On Sun, 01 Nov 2020 20:14:16 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Sonntag, 1. November 2020 18:44:44 CET Greg Kurz wrote:
> > On Sun, 1 Nov 2020 15:37:12 +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.
> > > 
> > > The constructor functions needs to be in virtio-9p-test.c, not in
> > > virtio-9p.c, because in the latter location it would cause all apps that
> > > link to libqos (i.e. entirely unrelated test suites) to create a 9pfs
> > > test directory as well, which would even break other test suites.
> > > 
> > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > ---
> > 
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> Thanks for the overtime, on a Sunday!
> 
> Queued on 9p.next:
> https://github.com/cschoenebeck/qemu/commits/9p.next
> 
> And this one with Peter Xu's patches on top, just for testing:
> https://github.com/cschoenebeck/qemu/commits/9p.experimental.2
> 
> > I could run 'make check -j' with 4 archs (ppc64, x86_64, aarch64, s390x)
> > on a POWER9 system with 128 cpus, for ~1 hour without seeing any failure.
> > 
> > Tested-by: Greg Kurz <groug@kaod.org>
> 
> OO Sounds like having advantages working for IBM. Respect. I start to get envy 
> as these beasts are running towards PCIe 6, while we regular x86 users would 
> already be glad having PCIe 4.
> 

I work for Red Hat now but yes, this allows easier access to bigger systems.

> I give it some more spinning hours this time, just to be sure, before sending 
> the PR tomorrow morning. But I think it's all right now.
> 

Cool ! :)

> Thanks!
> 
> Best regards,
> Christian Schoenebeck
> 
> 



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 0/2] 9pfs: test suite fixes
  2020-11-01 19:17   ` Christian Schoenebeck
@ 2020-11-01 19:35     ` Mark Cave-Ayland
  0 siblings, 0 replies; 10+ messages in thread
From: Mark Cave-Ayland @ 2020-11-01 19:35 UTC (permalink / raw)
  To: Christian Schoenebeck, qemu-devel; +Cc: Greg Kurz

On 01/11/2020 19:17, Christian Schoenebeck wrote:

> Yes, that should be resolved with the next 9p PR as well, additionally with
> the following patch that is:
> https://github.com/cschoenebeck/qemu/commit/603cc76a6069
> 
> Thanks for the feedback!

Fantastic - thanks a lot :)


ATB,

Mark.


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-11-01 19:36 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-11-01 15:12 [PATCH v3 0/2] 9pfs: test suite fixes Christian Schoenebeck
2020-11-01 14:25 ` [PATCH v3 1/2] tests/9pfs: make create/remove test dir public Christian Schoenebeck
2020-11-01 17:03   ` Greg Kurz
2020-11-01 14:37 ` [PATCH v3 2/2] tests/9pfs: fix test dir for parallel tests Christian Schoenebeck
2020-11-01 17:44   ` Greg Kurz
2020-11-01 19:14     ` Christian Schoenebeck
2020-11-01 19:31       ` Greg Kurz
2020-11-01 18:02 ` [PATCH v3 0/2] 9pfs: test suite fixes Mark Cave-Ayland
2020-11-01 19:17   ` Christian Schoenebeck
2020-11-01 19:35     ` Mark Cave-Ayland

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).