[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/2] tests/9pfs: fix coverity error in create_local_test_dir(
From: |
Greg Kurz |
Subject: |
Re: [PATCH 2/2] tests/9pfs: fix coverity error in create_local_test_dir() |
Date: |
Fri, 30 Oct 2020 13:32:04 +0100 |
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
>
>