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

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(-)

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();
> +}

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

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
> 
>