| Message ID | b111895492f8fb79bcca1c3e9586c0615f46cc97.1604046404.git.qemu_oss@crudebyte.com |
|---|---|
| State | New |
| Headers | show |
| Series | 9pfs: test suite fixes | expand |
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);
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
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
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 > >
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
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);
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(-)