| Message ID | 20210526091248.434459-2-stefanha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vhost-user-blk-test and vdagent Coverity fixes | expand |
On Wed, 26 May 2021 at 10:16, Stefan Hajnoczi <stefanha@redhat.com> wrote: > > Coverity checks that the file descriptor return value of open(2) is > checked and used. Normally this is helpful in identifying real bugs but > vhost-user-blk-test opens /dev/null as stdin and stdout after fork. > > In this case we don't need to look at the return value because these > open(2) calls cannot fail in any reasonable environment. We already know > their return values ahead of time since we closed stdin and stdout > previously. open(2) is guaranteed to pick the lowest available fd > number. > > Silence Coverity by introducing code that checks what we already know to > be true. > diff --git a/tests/qtest/vhost-user-blk-test.c b/tests/qtest/vhost-user-blk-test.c > index 8796c74ca4..581e283a03 100644 > --- a/tests/qtest/vhost-user-blk-test.c > +++ b/tests/qtest/vhost-user-blk-test.c > @@ -910,14 +910,18 @@ static void start_vhost_user_blk(GString *cmd_line, int vus_instances, > storage_daemon_command->str); > pid_t pid = fork(); > if (pid == 0) { > + int fd; > + > /* > * Close standard file descriptors so tap-driver.pl pipe detects when > * our parent terminates. > */ > close(0); > + fd = open("/dev/null", O_RDONLY); > + g_assert_cmpint(fd, ==, 0); > close(1); > - open("/dev/null", O_RDONLY); > - open("/dev/null", O_WRONLY); > + fd = open("/dev/null", O_WRONLY); > + assert(fd == 1); Why use a different assert type for the two asserts? thanks -- PMM
On Sun, May 30, 2021 at 08:05:49PM +0100, Peter Maydell wrote: > On Wed, 26 May 2021 at 10:16, Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > > Coverity checks that the file descriptor return value of open(2) is > > checked and used. Normally this is helpful in identifying real bugs but > > vhost-user-blk-test opens /dev/null as stdin and stdout after fork. > > > > In this case we don't need to look at the return value because these > > open(2) calls cannot fail in any reasonable environment. We already know > > their return values ahead of time since we closed stdin and stdout > > previously. open(2) is guaranteed to pick the lowest available fd > > number. > > > > Silence Coverity by introducing code that checks what we already know to > > be true. > > > diff --git a/tests/qtest/vhost-user-blk-test.c b/tests/qtest/vhost-user-blk-test.c > > index 8796c74ca4..581e283a03 100644 > > --- a/tests/qtest/vhost-user-blk-test.c > > +++ b/tests/qtest/vhost-user-blk-test.c > > @@ -910,14 +910,18 @@ static void start_vhost_user_blk(GString *cmd_line, int vus_instances, > > storage_daemon_command->str); > > pid_t pid = fork(); > > if (pid == 0) { > > + int fd; > > + > > /* > > * Close standard file descriptors so tap-driver.pl pipe detects when > > * our parent terminates. > > */ > > close(0); > > + fd = open("/dev/null", O_RDONLY); > > + g_assert_cmpint(fd, ==, 0); > > close(1); > > - open("/dev/null", O_RDONLY); > > - open("/dev/null", O_WRONLY); > > + fd = open("/dev/null", O_WRONLY); > > + assert(fd == 1); > > > Why use a different assert type for the two asserts? Thanks for pointing this out. I will send a v2 that consistently uses g_assert_cmpint(). Stefan
diff --git a/tests/qtest/vhost-user-blk-test.c b/tests/qtest/vhost-user-blk-test.c index 8796c74ca4..581e283a03 100644 --- a/tests/qtest/vhost-user-blk-test.c +++ b/tests/qtest/vhost-user-blk-test.c @@ -910,14 +910,18 @@ static void start_vhost_user_blk(GString *cmd_line, int vus_instances, storage_daemon_command->str); pid_t pid = fork(); if (pid == 0) { + int fd; + /* * Close standard file descriptors so tap-driver.pl pipe detects when * our parent terminates. */ close(0); + fd = open("/dev/null", O_RDONLY); + g_assert_cmpint(fd, ==, 0); close(1); - open("/dev/null", O_RDONLY); - open("/dev/null", O_WRONLY); + fd = open("/dev/null", O_WRONLY); + assert(fd == 1); execlp("/bin/sh", "sh", "-c", storage_daemon_command->str, NULL); exit(1);