[PATCH v2 2/2] tests/9pfs: fix coverity error in create_local_test_dir()

[PATCH v2 2/2] tests/9pfs: fix coverity error in create_local_test_dir()

[Christian Schoenebeck]

Coverity wants the return value of mkdir() to be checked:

  /qemu/tests/qtest/libqos/virtio-9p.c: 48 in create_local_test_dir()
  42     /* Creates the directory for the 9pfs 'local' filesystem driver to
  access. */
  43     static void create_local_test_dir(void)
  44     {
  45         struct stat st;
  46
  47         g_assert(local_test_path != NULL);
  >>> CID 1435963:  Error handling issues  (CHECKED_RETURN)
  >>> Calling "mkdir(local_test_path, 511U)" without checking return value.
  This library function may fail and return an error code.
  48         mkdir(local_test_path, 0777);
  49
  50         /* ensure test directory exists now ... */
  51         g_assert(stat(local_test_path, &st) == 0);
  52         /* ... and is actually a directory */
  53         g_assert((st.st_mode & S_IFMT) == S_IFDIR);

So let's just do that and log an info-level message at least, because we
actually only care if the required directory exists and we do have an
existence check for that in place already.

Reported-by: Coverity (CID 1435963)
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/libqos/virtio-9p.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index 6b22fa0e9a..8459a3ee58 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -48,9 +48,14 @@ static void init_local_test_path(void)
 static void create_local_test_dir(void)
 {
     struct stat st;
+    int res;
 
     g_assert(local_test_path != NULL);
-    mkdir(local_test_path, 0777);
+    res = mkdir(local_test_path, 0777);
+    if (res < 0) {
+        g_test_message("mkdir('%s') failed: %s", local_test_path,
+                       strerror(errno));
+    }
 
     /* ensure test directory exists now ... */
     g_assert(stat(local_test_path, &st) == 0);
-- 
2.20.1

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

[Christian Schoenebeck]

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

[PATCH v2 0/2] 9pfs: test suite fixes

[Christian Schoenebeck]

Fixes two bugs with the 9pfs 'local' tests as discussed with latest 9P PR
(2020-10-23). See the discussion of that PR for details.

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: fix test dir for parallel tests
  tests/9pfs: fix coverity error in create_local_test_dir()

 tests/qtest/libqos/virtio-9p.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

-- 
2.20.1

Re: [PATCH v2 0/2] 9pfs: test suite fixes

[Greg Kurz]

On Fri, 30 Oct 2020 13:43:59 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> Fixes two bugs with the 9pfs 'local' tests as discussed with latest 9P PR
> (2020-10-23). See the discussion of that PR for details.
> 
> 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: fix test dir for parallel tests
>   tests/9pfs: fix coverity error in create_local_test_dir()
> 
>  tests/qtest/libqos/virtio-9p.c | 32 +++++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)
> 

Series,

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

Re: [PATCH v2 0/2] 9pfs: test suite fixes

[Christian Schoenebeck]

On Freitag, 30. Oktober 2020 14:23:15 CET Greg Kurz wrote:
> On Fri, 30 Oct 2020 13:43:59 +0100
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > Fixes two bugs with the 9pfs 'local' tests as discussed with latest 9P PR
> > (2020-10-23). See the discussion of that PR for details.
> > 
> > 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: fix test dir for parallel tests
> >   tests/9pfs: fix coverity error in create_local_test_dir()
> >  
> >  tests/qtest/libqos/virtio-9p.c | 32 +++++++++++++++++++++++++-------
> >  1 file changed, 25 insertions(+), 7 deletions(-)
> 
> Series,
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>

Thanks for review and testing. I appreciate it.

Best regards,
Christian Schoenebeck