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

[PATCH 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.

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 | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

-- 
2.20.1

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

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

[Greg Kurz]

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.

[...]

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

[Thomas Huth]

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

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

[Christian Schoenebeck]

Coverity wants the return value of mkdir() to be checked, so let's
pretend to do that. We're actually just making a dummy check and
ignore the result, because we actually only care if the required
directory exists and we have an existence check for that in place
already.

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/libqos/virtio-9p.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index 6b22fa0e9a..0a7c0ee5d8 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -48,9 +48,13 @@ 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) {
+        /* ignore error, dummy check to prevent error by coverity */
+    }
 
     /* ensure test directory exists now ... */
     g_assert(stat(local_test_path, &st) == 0);
-- 
2.20.1

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

[Greg Kurz]

On Fri, 30 Oct 2020 09:19:46 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> Coverity wants the return value of mkdir() to be checked, so let's
> pretend to do that. We're actually just making a dummy check and
> ignore the result, because we actually only care if the required
> directory exists and we have an existence check for that in place
> already.
> 

I see that sometimes changelog shows a copy of the original
coverity report (e.g. commit df1a312fea58).

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

Please give credits to coverity, not me :-)

And most importantly, we want to mention the CID in the changelog.

e.g.

Reported-by: Coverity (CID 1435963)

> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  tests/qtest/libqos/virtio-9p.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
> index 6b22fa0e9a..0a7c0ee5d8 100644
> --- a/tests/qtest/libqos/virtio-9p.c
> +++ b/tests/qtest/libqos/virtio-9p.c
> @@ -48,9 +48,13 @@ 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) {
> +        /* ignore error, dummy check to prevent error by coverity */

Why not printing an error message with errno there like you did in
the previous patch ?

> +    }
>  
>      /* ensure test directory exists now ... */
>      g_assert(stat(local_test_path, &st) == 0);

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

[Christian Schoenebeck]

On Freitag, 30. Oktober 2020 12:44:18 CET Greg Kurz wrote:
> On Fri, 30 Oct 2020 09:19:46 +0100
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > Coverity wants the return value of mkdir() to be checked, so let's
> > pretend to do that. We're actually just making a dummy check and
> > ignore the result, because we actually only care if the required
> > directory exists and we have an existence check for that in place
> > already.
> 
> I see that sometimes changelog shows a copy of the original
> coverity report (e.g. commit df1a312fea58).

Ok, I'll add that.

> > Reported-by: Greg Kurz <groug@kaod.org>
> 
> Please give credits to coverity, not me :-)
> 
> And most importantly, we want to mention the CID in the changelog.
> 
> e.g.
> 
> Reported-by: Coverity (CID 1435963)

Ok.

It's not clear to me where this coverity report is accessible online. A quick 
search only brought me to statistics about its latest check, but not the 
details of the report you quoted.

And more importantly: is there coverity CI support that one could enable on 
github, so that pending patches were checked before upstream merge?

> 
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> > 
> >  tests/qtest/libqos/virtio-9p.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/qtest/libqos/virtio-9p.c
> > b/tests/qtest/libqos/virtio-9p.c index 6b22fa0e9a..0a7c0ee5d8 100644
> > --- a/tests/qtest/libqos/virtio-9p.c
> > +++ b/tests/qtest/libqos/virtio-9p.c
> > @@ -48,9 +48,13 @@ 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) {
> > +        /* ignore error, dummy check to prevent error by coverity */
> 
> Why not printing an error message with errno there like you did in
> the previous patch ?

Yeah, originally I didn't want to trigger false positives on automated CIs if 
mkdir() failed just because the directory already exists. But OTOH 
g_test_message() is just an info-level message, so it is Ok to bark silently 
and I will add it.

> 
> > +    }
> > 
> >      /* ensure test directory exists now ... */
> >      g_assert(stat(local_test_path, &st) == 0);

Thanks!

Best regards,
Christian Schoenebeck

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

[Peter Maydell]

On Fri, 30 Oct 2020 at 12:02, Christian Schoenebeck
<qemu_oss@crudebyte.com> wrote:
> On Freitag, 30. Oktober 2020 12:44:18 CET Greg Kurz wrote:
> It's not clear to me where this coverity report is accessible online. A quick
> search only brought me to statistics about its latest check, but not the
> details of the report you quoted.

https://scan.coverity.com/projects/qemu . To see the actual
defect reports you need to create an account and request
access to the QEMU project (we happily give access to developers,
but it is a manual-approval process).

> And more importantly: is there coverity CI support that one could enable on
> github, so that pending patches were checked before upstream merge?

No, unfortunately not. The Coverity free-for-open-source-projects
system has a very limited number of scans it allows (for a project
the size of ours just one a day) so we can't open it up to
submaintainer branches or even use it on pull requests pre merge;
the best we can do is running it on master daily.

thanks
-- PMM

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

[Christian Schoenebeck]

On Freitag, 30. Oktober 2020 13:09:26 CET Peter Maydell wrote:
> On Fri, 30 Oct 2020 at 12:02, Christian Schoenebeck
> 
> <qemu_oss@crudebyte.com> wrote:
> > On Freitag, 30. Oktober 2020 12:44:18 CET Greg Kurz wrote:
> > It's not clear to me where this coverity report is accessible online. A
> > quick search only brought me to statistics about its latest check, but
> > not the details of the report you quoted.
> 
> https://scan.coverity.com/projects/qemu . To see the actual
> defect reports you need to create an account and request
> access to the QEMU project (we happily give access to developers,
> but it is a manual-approval process).
> 
> > And more importantly: is there coverity CI support that one could enable
> > on
> > github, so that pending patches were checked before upstream merge?
> 
> No, unfortunately not. The Coverity free-for-open-source-projects
> system has a very limited number of scans it allows (for a project
> the size of ours just one a day) so we can't open it up to
> submaintainer branches or even use it on pull requests pre merge;
> the best we can do is running it on master daily.
> 
> thanks
> -- PMM

Thanks for the clarification Peter!

I try to sign up for Coverity next week.

Best regards,
Christian Schoenebeck

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

[Greg Kurz]

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

> On Freitag, 30. Oktober 2020 12:44:18 CET Greg Kurz wrote:
> > On Fri, 30 Oct 2020 09:19:46 +0100
> > 
> > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > Coverity wants the return value of mkdir() to be checked, so let's
> > > pretend to do that. We're actually just making a dummy check and
> > > ignore the result, because we actually only care if the required
> > > directory exists and we have an existence check for that in place
> > > already.
> > 
> > I see that sometimes changelog shows a copy of the original
> > coverity report (e.g. commit df1a312fea58).
> 
> Ok, I'll add that.
> 
> > > Reported-by: Greg Kurz <groug@kaod.org>
> > 
> > Please give credits to coverity, not me :-)
> > 
> > And most importantly, we want to mention the CID in the changelog.
> > 
> > e.g.
> > 
> > Reported-by: Coverity (CID 1435963)
> 
> Ok.
> 
> It's not clear to me where this coverity report is accessible online. A quick 
> search only brought me to statistics about its latest check, but not the 
> details of the report you quoted.
> 

I've been notified by mail because I have an account there.

https://scan.coverity.com/users/sign_up

> And more importantly: is there coverity CI support that one could enable on 
> github, so that pending patches were checked before upstream merge?
> 

I see that Peter already provided the details.

> > 
> > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > ---
> > > 
> > >  tests/qtest/libqos/virtio-9p.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/qtest/libqos/virtio-9p.c
> > > b/tests/qtest/libqos/virtio-9p.c index 6b22fa0e9a..0a7c0ee5d8 100644
> > > --- a/tests/qtest/libqos/virtio-9p.c
> > > +++ b/tests/qtest/libqos/virtio-9p.c
> > > @@ -48,9 +48,13 @@ 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) {
> > > +        /* ignore error, dummy check to prevent error by coverity */
> > 
> > Why not printing an error message with errno there like you did in
> > the previous patch ?
> 
> Yeah, originally I didn't want to trigger false positives on automated CIs if 
> mkdir() failed just because the directory already exists. But OTOH 

mkdtemp() should buy you the directory doesn't exist.

> g_test_message() is just an info-level message, so it is Ok to bark silently 
> and I will add it.
> 
> > 
> > > +    }
> > > 
> > >      /* ensure test directory exists now ... */
> > >      g_assert(stat(local_test_path, &st) == 0);
> 
> Thanks!
> 
> Best regards,
> Christian Schoenebeck
> 
>