Message ID | fb31dd18ad396ab602ba8957ee01a666f79ad9bb.1599558175.git.viresh.kumar@linaro.org |
---|---|
State | Changes Requested |
Headers | show |
Series | [V3,1/3] syscalls: select: Merge few tests and migrate to new format | expand |
Hi! > select() returns a positive value on success if timeout hasn't happened, > else returns 0. Check that and send some data to the write file > descriptor for the same. > > Acked-by: Li Wang <liwang@redhat.com> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > testcases/kernel/syscalls/select/select01.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/testcases/kernel/syscalls/select/select01.c b/testcases/kernel/syscalls/select/select01.c > index 1213aa931251..c6b30aa67dd7 100644 > --- a/testcases/kernel/syscalls/select/select01.c > +++ b/testcases/kernel/syscalls/select/select01.c > @@ -25,25 +25,32 @@ static struct select_info { > int *nfds; > fd_set *readfds; > fd_set *writefds; > + int *writefd; > char *desc; > } tests[] = { > - {&fd_reg, &readfds_reg, NULL, "with regular file"}, > - {&fds_pipe[1], &readfds_pipe, &writefds_pipe, "with system pipe"}, > - {&fd_npipe, &readfds_npipe, &writefds_npipe, "with named pipe (FIFO)"}, > + {&fd_reg, &readfds_reg, NULL, NULL, "with regular file"}, > + {&fds_pipe[1], &readfds_pipe, &writefds_pipe, &fds_pipe[1], "with system pipe"}, > + {&fd_npipe, &readfds_npipe, &writefds_npipe, &fd_npipe, "with named pipe (FIFO)"}, > }; > > static void run(unsigned int n) > { > struct select_info *tc = &tests[n]; > struct timeval timeout; > + char buf; > > timeout.tv_sec = 0; > timeout.tv_usec = 100000; > > + if (tc->writefd) > + SAFE_WRITE(0, *tc->writefd, &buf, sizeof(buf)); I'm not sure why we are writing data here. For both the pipe and fifo the select() will return that we can write there, hence the return would be non-zero regardless. Also I would like to be more specific. E.g. expecting specific return instead of just non-zero and also making sure the right bits are enabled in the fd sets. > TEST(do_select(*tc->nfds + 1, tc->readfds, tc->writefds, 0, &timeout)); > > if (TST_RET == -1) > tst_res(TFAIL | TTERRNO, "select() failed %s", tc->desc); > + else if (!TST_RET) > + tst_res(TFAIL, "select() timed out %s", tc->desc); > else > tst_res(TPASS, "select() passed %s", tc->desc); > } > -- > 2.25.0.rc1.19.g042ed3e048af >
On 14-10-20, 14:13, Cyril Hrubis wrote: > Hi! > > select() returns a positive value on success if timeout hasn't happened, > > else returns 0. Check that and send some data to the write file > > descriptor for the same. > > > > Acked-by: Li Wang <liwang@redhat.com> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > --- > > testcases/kernel/syscalls/select/select01.c | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/testcases/kernel/syscalls/select/select01.c b/testcases/kernel/syscalls/select/select01.c > > index 1213aa931251..c6b30aa67dd7 100644 > > --- a/testcases/kernel/syscalls/select/select01.c > > +++ b/testcases/kernel/syscalls/select/select01.c > > @@ -25,25 +25,32 @@ static struct select_info { > > int *nfds; > > fd_set *readfds; > > fd_set *writefds; > > + int *writefd; > > char *desc; > > } tests[] = { > > - {&fd_reg, &readfds_reg, NULL, "with regular file"}, > > - {&fds_pipe[1], &readfds_pipe, &writefds_pipe, "with system pipe"}, > > - {&fd_npipe, &readfds_npipe, &writefds_npipe, "with named pipe (FIFO)"}, > > + {&fd_reg, &readfds_reg, NULL, NULL, "with regular file"}, > > + {&fds_pipe[1], &readfds_pipe, &writefds_pipe, &fds_pipe[1], "with system pipe"}, > > + {&fd_npipe, &readfds_npipe, &writefds_npipe, &fd_npipe, "with named pipe (FIFO)"}, > > }; > > > > static void run(unsigned int n) > > { > > struct select_info *tc = &tests[n]; > > struct timeval timeout; > > + char buf; > > > > timeout.tv_sec = 0; > > timeout.tv_usec = 100000; > > > > + if (tc->writefd) > > + SAFE_WRITE(0, *tc->writefd, &buf, sizeof(buf)); > > I'm not sure why we are writing data here. For both the pipe and fifo > the select() will return that we can write there, hence the return would > be non-zero regardless. Maybe I haven't understood what you meant when you said this earlier: And the coverate in these tests is a bit lacking, we do not have a single tests that would send a data over a pipe to a fd select is watching and check that select was woken up by that. There is no such test in the pselect/ directory either. > Also I would like to be more specific. E.g. expecting specific return > instead of just non-zero and also making sure the right bits are enabled > in the fd sets. Something like this ? diff --git a/testcases/kernel/syscalls/select/select01.c b/testcases/kernel/syscalls/select/select01.c index e4b5caecbb10..4b33c0a01380 100644 --- a/testcases/kernel/syscalls/select/select01.c +++ b/testcases/kernel/syscalls/select/select01.c @@ -38,12 +38,15 @@ static void run(unsigned int n) struct tcases *tc = &tests[n]; struct timeval timeout; char buf; + int exp_ret = 1; timeout.tv_sec = 0; timeout.tv_usec = 100000; - if (tc->writefd) + if (tc->writefd) { SAFE_WRITE(0, *tc->writefd, &buf, sizeof(buf)); + exp_ret++; + } TEST(do_select(*tc->nfds + 1, tc->readfds, tc->writefds, 0, &timeout)); @@ -51,6 +54,8 @@ static void run(unsigned int n) tst_res(TFAIL | TTERRNO, "select() failed %s", tc->desc); else if (!TST_RET) tst_res(TFAIL, "select() timed out %s", tc->desc); + else if (TST_RET != exp_ret) + tst_res(TFAIL, "select() returned incorrect value: %s, expected: %d, got: %lu", tc->desc, exp_ret, TST_RET); else tst_res(TPASS, "select() passed %s", tc->desc); }
Hi! > Maybe I haven't understood what you meant when you said this earlier: > > And the coverate in these tests is a bit lacking, we do not have a > single tests that would send a data over a pipe to a fd select is > watching and check that select was woken up by that. There is no such > test in the pselect/ directory either. > > > Also I would like to be more specific. E.g. expecting specific return > > instead of just non-zero and also making sure the right bits are enabled > > in the fd sets. > > Something like this ? This is much better, I suppose that we should as well check the individual bits in the fd_sets to make it perfect. > diff --git a/testcases/kernel/syscalls/select/select01.c b/testcases/kernel/syscalls/select/select01.c > index e4b5caecbb10..4b33c0a01380 100644 > --- a/testcases/kernel/syscalls/select/select01.c > +++ b/testcases/kernel/syscalls/select/select01.c > @@ -38,12 +38,15 @@ static void run(unsigned int n) > struct tcases *tc = &tests[n]; > struct timeval timeout; > char buf; > + int exp_ret = 1; > > timeout.tv_sec = 0; > timeout.tv_usec = 100000; > > - if (tc->writefd) > + if (tc->writefd) { > SAFE_WRITE(0, *tc->writefd, &buf, sizeof(buf)); > + exp_ret++; > + } > > TEST(do_select(*tc->nfds + 1, tc->readfds, tc->writefds, 0, &timeout)); > > @@ -51,6 +54,8 @@ static void run(unsigned int n) > tst_res(TFAIL | TTERRNO, "select() failed %s", tc->desc); > else if (!TST_RET) > tst_res(TFAIL, "select() timed out %s", tc->desc); > + else if (TST_RET != exp_ret) > + tst_res(TFAIL, "select() returned incorrect value: %s, expected: %d, got: %lu", tc->desc, exp_ret, TST_RET); > else > tst_res(TPASS, "select() passed %s", tc->desc); > } > > -- > viresh
diff --git a/testcases/kernel/syscalls/select/select01.c b/testcases/kernel/syscalls/select/select01.c index 1213aa931251..c6b30aa67dd7 100644 --- a/testcases/kernel/syscalls/select/select01.c +++ b/testcases/kernel/syscalls/select/select01.c @@ -25,25 +25,32 @@ static struct select_info { int *nfds; fd_set *readfds; fd_set *writefds; + int *writefd; char *desc; } tests[] = { - {&fd_reg, &readfds_reg, NULL, "with regular file"}, - {&fds_pipe[1], &readfds_pipe, &writefds_pipe, "with system pipe"}, - {&fd_npipe, &readfds_npipe, &writefds_npipe, "with named pipe (FIFO)"}, + {&fd_reg, &readfds_reg, NULL, NULL, "with regular file"}, + {&fds_pipe[1], &readfds_pipe, &writefds_pipe, &fds_pipe[1], "with system pipe"}, + {&fd_npipe, &readfds_npipe, &writefds_npipe, &fd_npipe, "with named pipe (FIFO)"}, }; static void run(unsigned int n) { struct select_info *tc = &tests[n]; struct timeval timeout; + char buf; timeout.tv_sec = 0; timeout.tv_usec = 100000; + if (tc->writefd) + SAFE_WRITE(0, *tc->writefd, &buf, sizeof(buf)); + TEST(do_select(*tc->nfds + 1, tc->readfds, tc->writefds, 0, &timeout)); if (TST_RET == -1) tst_res(TFAIL | TTERRNO, "select() failed %s", tc->desc); + else if (!TST_RET) + tst_res(TFAIL, "select() timed out %s", tc->desc); else tst_res(TPASS, "select() passed %s", tc->desc); }