| Message ID | 20240131135026.1562-1-mdoucha@suse.cz |
|---|---|
| State | Superseded |
| Headers | show |
| Series | waitpid04: Convert to new API | expand |
> Convert waitpid() error state checks to new API, fix bugs in the original > test and add ESRCH subtest. Very nice cleanup. Reviewed-by: Petr Vorel <pvorel@suse.cz> BTW what were these bugs? It looks like old test uses pid O for the first ECHILD test (not sure how it got defined to 0). > #include <sys/signal.h> nit: this should be <signal.h> warning: #warning redirecting incorrect #include <sys/signal.h> to <signal.h> [-Wcpp] Can be fixed before merge. > #include <sys/types.h> > #include <sys/wait.h> ... > +#include "tst_test.h" > + > +#define TCFMT "waipid(%d, NULL, 0x%x)" > +#define TCFMTARGS(tc) (tc)->pid, (tc)->flags checkpatch.pl complains: waitpid04.c:19: ERROR: Macros with complex values should be enclosed in parentheses I guess we can ignore this unless you see a quick fix (I guess macro which runs tst_res() inside would be needed. > +static struct testcase { > + pid_t pid; > + int flags; > + int err; > +} testcase_list[] = { > + {-1, 0, ECHILD}, /* Wait for any child when none exist */ > + {1, 0, ECHILD}, /* Wait for non-child process */ > + {-1, -1, EINVAL}, /* Invalid flags */ > + {INT_MIN, 0, ESRCH}, /* Wait for invalid process group */ > +}; > + > +void run(unsigned int n) > { ... > + const struct testcase *tc = testcase_list + n; > - if (FORK_OR_VFORK() == 0) > - exit(0); > + TEST(waitpid(tc->pid, NULL, tc->flags)); How about using TST_EXP_FAIL2() to avoid code below? It would also help to avoid macros, right? Or you want to explicitly state what failed? Kind regards, Petr > + if (TST_RET == -1 && TST_ERR == tc->err) { > + tst_res(TPASS | TTERRNO, TCFMT " failed as expected", > + TCFMTARGS(tc)); > + return; > + } > + if (TST_RET == -1) { > + tst_res(TFAIL | TTERRNO, TCFMT ": expected error %s, got", > + TCFMTARGS(tc), tst_strerrno(tc->err)); > + return; > } > + if (TST_RET < 0) { > + tst_res(TFAIL | TTERRNO, TCFMT ": invalid return value %ld", > + TCFMTARGS(tc), TST_RET); > + return; > + } > + tst_res(TFAIL, TCFMT " returned unexpected PID %ld", TCFMTARGS(tc), > + TST_RET); > }
On 31. 01. 24 18:21, Petr Vorel wrote: >> Convert waitpid() error state checks to new API, fix bugs in the original >> test and add ESRCH subtest. > > Very nice cleanup. > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > BTW what were these bugs? It looks like old test uses pid O for the first ECHILD > test (not sure how it got defined to 0). The first subtest used uninitialized pid value in the first iteration and then it'd test only pid=1 in all other iterations. > >> #include <sys/signal.h> > nit: this should be <signal.h> > warning: #warning redirecting incorrect #include <sys/signal.h> to <signal.h> [-Wcpp] > > Can be fixed before merge. Yes, please fix. Strangely, I didn't get this warning during compilation. >> #include <sys/types.h> >> #include <sys/wait.h> > ... >> +#include "tst_test.h" >> + >> +#define TCFMT "waipid(%d, NULL, 0x%x)" >> +#define TCFMTARGS(tc) (tc)->pid, (tc)->flags > > checkpatch.pl complains: > waitpid04.c:19: ERROR: Macros with complex values should be enclosed in parentheses > > I guess we can ignore this unless you see a quick fix (I guess macro which runs > tst_res() inside would be needed. The warning is expected, the macro should expand to multiple arguments. Parentheses would prevent that. I could fix it by tst_res(TINFO, "waitpid($args)"); before each test. > >> +static struct testcase { >> + pid_t pid; >> + int flags; >> + int err; >> +} testcase_list[] = { >> + {-1, 0, ECHILD}, /* Wait for any child when none exist */ >> + {1, 0, ECHILD}, /* Wait for non-child process */ >> + {-1, -1, EINVAL}, /* Invalid flags */ >> + {INT_MIN, 0, ESRCH}, /* Wait for invalid process group */ >> +}; >> + >> +void run(unsigned int n) >> { > ... >> + const struct testcase *tc = testcase_list + n; > >> - if (FORK_OR_VFORK() == 0) >> - exit(0); >> + TEST(waitpid(tc->pid, NULL, tc->flags)); > How about using TST_EXP_FAIL2() to avoid code below? It would also help to avoid > macros, right? Or you want to explicitly state what failed? The waitpid() call would be converted to a string that gives no useful information about which testcase is actually running and there isn't any good error message if the call succeeds. > > Kind regards, > Petr > >> + if (TST_RET == -1 && TST_ERR == tc->err) { >> + tst_res(TPASS | TTERRNO, TCFMT " failed as expected", >> + TCFMTARGS(tc)); >> + return; >> + } > >> + if (TST_RET == -1) { >> + tst_res(TFAIL | TTERRNO, TCFMT ": expected error %s, got", >> + TCFMTARGS(tc), tst_strerrno(tc->err)); >> + return; >> } > >> + if (TST_RET < 0) { >> + tst_res(TFAIL | TTERRNO, TCFMT ": invalid return value %ld", >> + TCFMTARGS(tc), TST_RET); >> + return; >> + } > >> + tst_res(TFAIL, TCFMT " returned unexpected PID %ld", TCFMTARGS(tc), >> + TST_RET); >> }
Hi Martin, > On 31. 01. 24 18:21, Petr Vorel wrote: > > > Convert waitpid() error state checks to new API, fix bugs in the original > > > test and add ESRCH subtest. > > Very nice cleanup. > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > BTW what were these bugs? It looks like old test uses pid O for the first ECHILD > > test (not sure how it got defined to 0). > The first subtest used uninitialized pid value in the first iteration and > then it'd test only pid=1 in all other iterations. Thanks for info. > > > #include <sys/signal.h> > > nit: this should be <signal.h> > > warning: #warning redirecting incorrect #include <sys/signal.h> to <signal.h> [-Wcpp] > > Can be fixed before merge. > Yes, please fix. Strangely, I didn't get this warning during compilation. The problem is visible only on pedantic MUSL. Not a big issue, it would work: cat /usr/include/sys/signal.h #include <signal.h> but it's just better to use the standard header, not the fallback one (one day MUSL might removes the fallback). > > > #include <sys/types.h> > > > #include <sys/wait.h> > > ... > > > +#include "tst_test.h" > > > + > > > +#define TCFMT "waipid(%d, NULL, 0x%x)" > > > +#define TCFMTARGS(tc) (tc)->pid, (tc)->flags > > checkpatch.pl complains: > > waitpid04.c:19: ERROR: Macros with complex values should be enclosed in parentheses > > I guess we can ignore this unless you see a quick fix (I guess macro which runs > > tst_res() inside would be needed. > The warning is expected, the macro should expand to multiple arguments. > Parentheses would prevent that. I could fix it by tst_res(TINFO, > "waitpid($args)"); before each test. Yep, that's what I would do - just to print params before test: tst_res(TINFO, "waipid(%d, NULL, 0x%x)", tc->pid, tc->flags); TEST(waitpid(tc->pid, NULL, tc->flags)); > > > +static struct testcase { > > > + pid_t pid; > > > + int flags; > > > + int err; > > > +} testcase_list[] = { > > > + {-1, 0, ECHILD}, /* Wait for any child when none exist */ > > > + {1, 0, ECHILD}, /* Wait for non-child process */ > > > + {-1, -1, EINVAL}, /* Invalid flags */ > > > + {INT_MIN, 0, ESRCH}, /* Wait for invalid process group */ > > > +}; > > > + > > > +void run(unsigned int n) > > > { > > ... > > > + const struct testcase *tc = testcase_list + n; > > > - if (FORK_OR_VFORK() == 0) > > > - exit(0); > > > + TEST(waitpid(tc->pid, NULL, tc->flags)); > > How about using TST_EXP_FAIL2() to avoid code below? It would also help to avoid > > macros, right? Or you want to explicitly state what failed? > The waitpid() call would be converted to a string that gives no useful > information about which testcase is actually running and there isn't any > good error message if the call succeeds. Indeed plain TST_EXP_FAIL2() would do it: TST_EXP_FAIL2(waitpid(tc->pid, NULL, tc->flags), tc->err); waitpid04.c:38: TFAIL: waitpid(tc->pid, NULL, tc->flags) expected ESRCH: ECHILD (10) ... waitpid04.c:38: TFAIL: waitpid(tc->pid, NULL, tc->flags) expected EINVAL: ECHILD (10) I definitely don't want to push for it, but the output is similar: TST_EXP_FAIL2(waitpid(tc->pid, NULL, tc->flags), tc->err, "waipid(%d, NULL, 0x%x)", tc->pid, tc->flags); When testing that with 2 broken testcase values: {-1, 0, ESRCH}, /* pvorel: changed from ECHILD */ {1, 0, ECHILD}, /* Wait for non-child process */ {-1, 0, EINVAL}, /* Invalid flags */ {INT_MIN, 0, ESRCH}, /* pvorel: flag changed from -1 */ waitpid04.c:38: TFAIL: waipid(-1, NULL, 0x0) expected ESRCH: ECHILD (10) /* new */ waitpid04.c:49: TFAIL: waipid(-1, NULL, 0x0): expected error ESRCH, got: ECHILD (10) /* orig */ ... waitpid04.c:38: TFAIL: waipid(-1, NULL, 0x0) expected EINVAL: ECHILD (10) /* new */ waitpid04.c:49: TFAIL: waipid(-1, NULL, 0x0): expected error EINVAL, got: ECHILD (10) /* orig */ But no problem to merge it as is. Sooner or later we should add test specific checkpatch.pl options to ignore certain warnings. Kind regards, Petr
diff --git a/testcases/kernel/syscalls/waitpid/waitpid04.c b/testcases/kernel/syscalls/waitpid/waitpid04.c index abf94eed1..ab3c1a4f4 100644 --- a/testcases/kernel/syscalls/waitpid/waitpid04.c +++ b/testcases/kernel/syscalls/waitpid/waitpid04.c @@ -1,144 +1,63 @@ +// SPDX-License-Identifier: GPL-2.0-or-later /* - * - * Copyright (c) International Business Machines Corp., 2001 - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; either version 2 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See - * the GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program; if not, write to the Free Software - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + * Copyright (c) International Business Machines Corp., 2001 + * Copyright (c) 2024 SUSE LLC <mdoucha@suse.cz> */ -/* - * NAME - * waitpid04.c - * - * DESCRIPTION - * test to check the error conditions in waitpid sys call +/*\ + * [Description] * - * USAGE: <for command-line> - * waitpid04 [-c n] [-e] [-i n] [-I x] [-P x] [-t] - * where, -c n : Run n copies concurrently. - * -e : Turn on errno logging. - * -i n : Execute test n times. - * -I x : Execute test for x seconds. - * -P x : Pause for x seconds between iterations. - * -t : Turn on syscall timing. - * - * History - * 07/2001 John George - * -Ported - * - * Restrictions - * NONE + * Test to check the error conditions in waitpid() syscall. */ #include <sys/signal.h> #include <sys/types.h> #include <sys/wait.h> -#include <errno.h> -#include "test.h" - -static void setup(void); -static void cleanup(void); - -char *TCID = "waitpid04"; -int TST_TOTAL = 1; - -#define INVAL_FLAG -1 - -static int flag, condition_number; - -int main(int ac, char **av) +#include "tst_test.h" + +#define TCFMT "waipid(%d, NULL, 0x%x)" +#define TCFMTARGS(tc) (tc)->pid, (tc)->flags + +static struct testcase { + pid_t pid; + int flags; + int err; +} testcase_list[] = { + {-1, 0, ECHILD}, /* Wait for any child when none exist */ + {1, 0, ECHILD}, /* Wait for non-child process */ + {-1, -1, EINVAL}, /* Invalid flags */ + {INT_MIN, 0, ESRCH}, /* Wait for invalid process group */ +}; + +void run(unsigned int n) { - int pid, status, ret; - - int lc; - - tst_parse_opts(ac, av, NULL, NULL); - - setup(); - - /* check for looping state if -i option is given */ - for (lc = 0; TEST_LOOPING(lc); lc++) { - /* reset tst_count in case we are looping */ - tst_count = 0; - - ret = waitpid(pid, &status, WNOHANG); - flag = 0; - condition_number = 1; - if (ret != -1) { - tst_resm(TFAIL, "condition %d test failed", - condition_number); - } else { - if (errno != ECHILD) { - tst_resm(TFAIL, "waitpid() set invalid " - "errno, expected ECHILD, got: %d", - errno); - } else { - tst_resm(TPASS, "condition %d test passed", - condition_number); - } - } - condition_number++; + const struct testcase *tc = testcase_list + n; - if (FORK_OR_VFORK() == 0) - exit(0); + TEST(waitpid(tc->pid, NULL, tc->flags)); - pid = 1; - ret = waitpid(pid, &status, WUNTRACED); - flag = 0; - if (ret != -1) { - tst_resm(TFAIL, "condition %d test failed", - condition_number); - } else { - if (errno != ECHILD) { - tst_resm(TFAIL, "waitpid() set invalid " - "errno, expected ECHILD, got: %d", - errno); - } else { - tst_resm(TPASS, "condition %d test passed", - condition_number); - } - } - condition_number++; + if (TST_RET == -1 && TST_ERR == tc->err) { + tst_res(TPASS | TTERRNO, TCFMT " failed as expected", + TCFMTARGS(tc)); + return; + } - /* Option is Inval = INVAL_FLAG */ - ret = waitpid(pid, &status, INVAL_FLAG); - flag = 0; - if (ret != -1) { - tst_resm(TFAIL, "condition %d test failed", - condition_number); - } else { - if (errno != EINVAL) { - tst_resm(TFAIL, "waitpid() set invalid " - "errno, expected EINVAL, got: %d", - errno); - } else { - tst_resm(TPASS, "condition %d test passed", - condition_number); - } - } - condition_number++; + if (TST_RET == -1) { + tst_res(TFAIL | TTERRNO, TCFMT ": expected error %s, got", + TCFMTARGS(tc), tst_strerrno(tc->err)); + return; } - cleanup(); - tst_exit(); -} + if (TST_RET < 0) { + tst_res(TFAIL | TTERRNO, TCFMT ": invalid return value %ld", + TCFMTARGS(tc), TST_RET); + return; + } -static void setup(void) -{ - TEST_PAUSE; + tst_res(TFAIL, TCFMT " returned unexpected PID %ld", TCFMTARGS(tc), + TST_RET); } -static void cleanup(void) -{ -} +static struct tst_test test = { + .test = run, + .tcnt = ARRAY_SIZE(testcase_list) +};
Convert waitpid() error state checks to new API, fix bugs in the original test and add ESRCH subtest. Signed-off-by: Martin Doucha <mdoucha@suse.cz> --- testcases/kernel/syscalls/waitpid/waitpid04.c | 171 +++++------------- 1 file changed, 45 insertions(+), 126 deletions(-)