1. Patchew
  2. QEMU
  3. [PATCH 0/2] 9pfs: test suite fixes
  4. View patch

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

Christian Schoenebeck posted 2 patches 5 years, 3 months ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Laurent Vivier <lvivier@redhat.com>, Thomas Huth <thuth@redhat.com>
There is a newer version of this series
[PATCH 2/2] tests/9pfs: fix coverity error in create_local_test_dir()
Posted by Christian Schoenebeck 5 years, 3 months ago 
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()
Posted by Greg Kurz 5 years, 3 months ago 
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()
Posted by Christian Schoenebeck 5 years, 3 months ago 
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()
Posted by Peter Maydell 5 years, 3 months ago 
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()
Posted by Christian Schoenebeck 5 years, 3 months ago 
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()
Posted by Greg Kurz 5 years, 3 months ago 
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
> 
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.