Message ID | 20210923085224.868-5-zhanglianjie@uniontech.com |
---|---|
State | Changes Requested |
Headers | show |
Series | [v3,1/5] syscalls/clone02: Convert to new API | expand |
Hi! > diff --git a/testcases/kernel/syscalls/clone/clone07.c b/testcases/kernel/syscalls/clone/clone07.c > index 4b2e04ee7..304810c40 100644 > --- a/testcases/kernel/syscalls/clone/clone07.c > +++ b/testcases/kernel/syscalls/clone/clone07.c > @@ -1,86 +1,58 @@ > +// SPDX-License-Identifier: GPL-2.0-only > /* > * Copyright (c) International Business Machines Corp., 2003. > * Copyright (c) 2012 Wanlong Gao <gaowanlong@cn.fujitsu.com> > - * > - * This program is free software; you can redistribute it and/or modify it > - * under the terms of version 2 of the GNU General Public License as > - * published by the Free Software Foundation. > - * > - * This program is distributed in the hope that it would be useful, but > - * WITHOUT ANY WARRANTY; without even the implied warranty of > - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. > - * > - * You should have received a copy of the GNU General Public License along > - * with this program; if not, write the Free Software Foundation, Inc., > - * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. > - * > - */ > -/* > - * This is a test for a glibc bug for the clone(2) system call. > */ > > -#if defined UCLINUX && !__THROW > -/* workaround for libc bug */ > -#define __THROW > -#endif > +/*\ > + * [Description] > + * This is a test for a glibc bug for the clone(2) system call. We can describe this a bit better. Apparently the bug was that using return from the do_child() function caused SIGSEGV so this should probably be: [Description] Test for a libc bug where exitting child function by returning from it caused SIGSEGV. > + */ > > -#include <errno.h> > #include <sched.h> > -#include <sys/wait.h> > -#include "test.h" > +#include <stdio.h> > +#include <stdlib.h> > +#include "tst_test.h" > +#include "lapi/syscalls.h" > #include "clone_platform.h" > > -#define TRUE 1 > -#define FALSE 0 > - > -static void setup(); > -static int do_child(); > - > -char *TCID = "clone07"; > -int TST_TOTAL = 1; > - > -static void sigsegv_handler(int); > -static void sigusr2_handler(int); > static int child_pid; > -static int fail = FALSE; > +static int fail; > +static void *child_stack; > > -int main(int ac, char **av) > +static int do_child(void *arg LTP_ATTRIBUTE_UNUSED) > { > + return 0; > +} > > - int lc, status; > - void *child_stack; > - > - tst_parse_opts(ac, av, NULL, NULL); > - > - setup(); > +static void sigsegv_handler(int sig LTP_ATTRIBUTE_UNUSED) > +{ > + if (child_pid == 0) { > + kill(getppid(), SIGUSR2); > + _exit(42); > + } > +} > > - for (lc = 0; TEST_LOOPING(lc); lc++) { > - tst_count = 0; > - child_stack = malloc(CHILD_STACK_SIZE); > - if (child_stack == NULL) > - tst_brkm(TBROK, NULL, > - "Cannot allocate stack for child"); > +static void sigusr2_handler(int sig LTP_ATTRIBUTE_UNUSED) > +{ > + if (child_pid != 0) > + fail = 1; > +} I do not get why we need this complicated handler setup. Why can't we just SAFE_WAITPID() for the child, check the status and fail the test if the status is anything else than: WIFEXITED(status) && WEXITSTATUS(status) == 0 > - child_pid = ltp_clone(SIGCHLD, do_child, NULL, > - CHILD_STACK_SIZE, child_stack); > +static void verify_clone(void) > +{ > + TST_EXP_PID_SILENT(ltp_clone(SIGCHLD, do_child, NULL, CHILD_STACK_SIZE, > + child_stack)); > > - if (child_pid < 0) > - tst_brkm(TBROK | TERRNO, NULL, "clone failed"); > + child_pid = TST_RET; > > - if ((wait(&status)) == -1) > - tst_brkm(TBROK | TERRNO, NULL, > - "wait failed, status: %d", status); > + if (!TST_PASS) > + return; > > - free(child_stack); > - } > + tst_reap_children(); > > - if (fail == FALSE) > - tst_resm(TPASS, > - "Use of return() in child did not cause SIGSEGV"); > - else > - tst_resm(TFAIL, "Use of return() in child caused SIGSEGV"); > + TST_EXP_VAL(fail, 0, "Use of return() in child did not cause SIGSEGV"); > > - tst_exit(); > } > > static void setup(void) > @@ -88,41 +60,26 @@ static void setup(void) > struct sigaction def_act; > struct sigaction act; > > - TEST_PAUSE; > - > act.sa_handler = sigsegv_handler; > act.sa_flags = SA_RESTART; > - sigemptyset(&act.sa_mask); > - if ((sigaction(SIGSEGV, &act, NULL)) == -1) > - tst_resm(TWARN | TERRNO, > - "sigaction() for SIGSEGV failed in test_setup()"); > + SAFE_SIGEMPTYSET(&act.sa_mask); > + SAFE_SIGACTION(SIGSEGV, &act, NULL); > > - /* Setup signal handler for SIGUSR2 */ > def_act.sa_handler = sigusr2_handler; > def_act.sa_flags = SA_RESTART | SA_RESETHAND; > - sigemptyset(&def_act.sa_mask); > - > - if ((sigaction(SIGUSR2, &def_act, NULL)) == -1) > - tst_resm(TWARN | TERRNO, > - "sigaction() for SIGUSR2 failed in test_setup()"); > -} > + SAFE_SIGEMPTYSET(&def_act.sa_mask); > + SAFE_SIGACTION(SIGUSR2, &def_act, NULL); > > -static int do_child(void) > -{ > - return 0; > + child_stack = SAFE_MALLOC(CHILD_STACK_SIZE); > } > > -static void sigsegv_handler(int sig) > +static void cleanup(void) > { > - if (child_pid == 0) { > - kill(getppid(), SIGUSR2); > - _exit(42); > - } > + free(child_stack); > } > > -/* sig_default_handler() - Default handler for parent */ > -static void sigusr2_handler(int sig) > -{ > - if (child_pid != 0) > - fail = TRUE; > -} > +static struct tst_test test = { > + .setup = setup, > + .test_all = verify_clone, > + .cleanup = cleanup, > +}; > -- > 2.20.1 > > > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
Hi, On 2021-10-12 17:37, Cyril Hrubis wrote: > Hi! >> diff --git a/testcases/kernel/syscalls/clone/clone07.c b/testcases/kernel/syscalls/clone/clone07.c >> index 4b2e04ee7..304810c40 100644 >> --- a/testcases/kernel/syscalls/clone/clone07.c >> +++ b/testcases/kernel/syscalls/clone/clone07.c >> @@ -1,86 +1,58 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> /* >> * Copyright (c) International Business Machines Corp., 2003. >> * Copyright (c) 2012 Wanlong Gao <gaowanlong@cn.fujitsu.com> >> - * >> - * This program is free software; you can redistribute it and/or modify it >> - * under the terms of version 2 of the GNU General Public License as >> - * published by the Free Software Foundation. >> - * >> - * This program is distributed in the hope that it would be useful, but >> - * WITHOUT ANY WARRANTY; without even the implied warranty of >> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. >> - * >> - * You should have received a copy of the GNU General Public License along >> - * with this program; if not, write the Free Software Foundation, Inc., >> - * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. >> - * >> - */ >> -/* >> - * This is a test for a glibc bug for the clone(2) system call. >> */ >> >> -#if defined UCLINUX && !__THROW >> -/* workaround for libc bug */ >> -#define __THROW >> -#endif >> +/*\ >> + * [Description] >> + * This is a test for a glibc bug for the clone(2) system call. > > We can describe this a bit better. > > Apparently the bug was that using return from the do_child() function > caused SIGSEGV so this should probably be: > > [Description] > > Test for a libc bug where exitting child function by returning from it > caused SIGSEGV. > >> + */ >> >> -#include <errno.h> >> #include <sched.h> >> -#include <sys/wait.h> >> -#include "test.h" >> +#include <stdio.h> >> +#include <stdlib.h> >> +#include "tst_test.h" >> +#include "lapi/syscalls.h" >> #include "clone_platform.h" >> >> -#define TRUE 1 >> -#define FALSE 0 >> - >> -static void setup(); >> -static int do_child(); >> - >> -char *TCID = "clone07"; >> -int TST_TOTAL = 1; >> - >> -static void sigsegv_handler(int); >> -static void sigusr2_handler(int); >> static int child_pid; >> -static int fail = FALSE; >> +static int fail; >> +static void *child_stack; >> >> -int main(int ac, char **av) >> +static int do_child(void *arg LTP_ATTRIBUTE_UNUSED) >> { >> + return 0; >> +} >> >> - int lc, status; >> - void *child_stack; >> - >> - tst_parse_opts(ac, av, NULL, NULL); >> - >> - setup(); >> +static void sigsegv_handler(int sig LTP_ATTRIBUTE_UNUSED) >> +{ >> + if (child_pid == 0) { >> + kill(getppid(), SIGUSR2); >> + _exit(42); >> + } >> +} >> >> - for (lc = 0; TEST_LOOPING(lc); lc++) { >> - tst_count = 0; >> - child_stack = malloc(CHILD_STACK_SIZE); >> - if (child_stack == NULL) >> - tst_brkm(TBROK, NULL, >> - "Cannot allocate stack for child"); >> +static void sigusr2_handler(int sig LTP_ATTRIBUTE_UNUSED) >> +{ >> + if (child_pid != 0) >> + fail = 1; >> +} > > I do not get why we need this complicated handler setup. > > Why can't we just SAFE_WAITPID() for the child, check the status and > fail the test if the status is anything else than: > > WIFEXITED(status) && WEXITSTATUS(status) == 0 > The processing here is to accurately determine whether a segment fault signal has occurred in the child process. Of course, can also use the method you provide, but you can only judge that the child process exits abnormally. >> - child_pid = ltp_clone(SIGCHLD, do_child, NULL, >> - CHILD_STACK_SIZE, child_stack); >> +static void verify_clone(void) >> +{ >> + TST_EXP_PID_SILENT(ltp_clone(SIGCHLD, do_child, NULL, CHILD_STACK_SIZE, >> + child_stack)); >> >> - if (child_pid < 0) >> - tst_brkm(TBROK | TERRNO, NULL, "clone failed"); >> + child_pid = TST_RET; >> >> - if ((wait(&status)) == -1) >> - tst_brkm(TBROK | TERRNO, NULL, >> - "wait failed, status: %d", status); >> + if (!TST_PASS) >> + return; >> >> - free(child_stack); >> - } >> + tst_reap_children(); >> >> - if (fail == FALSE) >> - tst_resm(TPASS, >> - "Use of return() in child did not cause SIGSEGV"); >> - else >> - tst_resm(TFAIL, "Use of return() in child caused SIGSEGV"); >> + TST_EXP_VAL(fail, 0, "Use of return() in child did not cause SIGSEGV"); >> >> - tst_exit(); >> } >> >> static void setup(void) >> @@ -88,41 +60,26 @@ static void setup(void) >> struct sigaction def_act; >> struct sigaction act; >> >> - TEST_PAUSE; >> - >> act.sa_handler = sigsegv_handler; >> act.sa_flags = SA_RESTART; >> - sigemptyset(&act.sa_mask); >> - if ((sigaction(SIGSEGV, &act, NULL)) == -1) >> - tst_resm(TWARN | TERRNO, >> - "sigaction() for SIGSEGV failed in test_setup()"); >> + SAFE_SIGEMPTYSET(&act.sa_mask); >> + SAFE_SIGACTION(SIGSEGV, &act, NULL); >> >> - /* Setup signal handler for SIGUSR2 */ >> def_act.sa_handler = sigusr2_handler; >> def_act.sa_flags = SA_RESTART | SA_RESETHAND; >> - sigemptyset(&def_act.sa_mask); >> - >> - if ((sigaction(SIGUSR2, &def_act, NULL)) == -1) >> - tst_resm(TWARN | TERRNO, >> - "sigaction() for SIGUSR2 failed in test_setup()"); >> -} >> + SAFE_SIGEMPTYSET(&def_act.sa_mask); >> + SAFE_SIGACTION(SIGUSR2, &def_act, NULL); >> >> -static int do_child(void) >> -{ >> - return 0; >> + child_stack = SAFE_MALLOC(CHILD_STACK_SIZE); >> } >> >> -static void sigsegv_handler(int sig) >> +static void cleanup(void) >> { >> - if (child_pid == 0) { >> - kill(getppid(), SIGUSR2); >> - _exit(42); >> - } >> + free(child_stack); >> } >> >> -/* sig_default_handler() - Default handler for parent */ >> -static void sigusr2_handler(int sig) >> -{ >> - if (child_pid != 0) >> - fail = TRUE; >> -} >> +static struct tst_test test = { >> + .setup = setup, >> + .test_all = verify_clone, >> + .cleanup = cleanup, >> +}; >> -- >> 2.20.1 >> >> >> >> >> -- >> Mailing list info: https://lists.linux.it/listinfo/ltp >
Hi! > > I do not get why we need this complicated handler setup. > > > > Why can't we just SAFE_WAITPID() for the child, check the status and > > fail the test if the status is anything else than: > > > > WIFEXITED(status) && WEXITSTATUS(status) == 0 > > > The processing here is to accurately determine whether a segment fault > signal has occurred in the child process. Of course, can also use the > method you provide, but you can only judge that the child process exits > abnormally. Well I do not think that the SEGFAULT needs to have special treatement here, as long as the child does anything else than exit with 0 it's a bug, that's why I think that we should fail the test in all other cases. I guess that we can add special check for the regression with WIFSIGNALED() && WTERMSIG() == SIGSEGV and produce slightly different TFAIL message, but we should really fail all the other cases as well.
diff --git a/testcases/kernel/syscalls/clone/clone07.c b/testcases/kernel/syscalls/clone/clone07.c index 4b2e04ee7..304810c40 100644 --- a/testcases/kernel/syscalls/clone/clone07.c +++ b/testcases/kernel/syscalls/clone/clone07.c @@ -1,86 +1,58 @@ +// SPDX-License-Identifier: GPL-2.0-only /* * Copyright (c) International Business Machines Corp., 2003. * Copyright (c) 2012 Wanlong Gao <gaowanlong@cn.fujitsu.com> - * - * This program is free software; you can redistribute it and/or modify it - * under the terms of version 2 of the GNU General Public License as - * published by the Free Software Foundation. - * - * This program is distributed in the hope that it would be useful, but - * WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. - * - * You should have received a copy of the GNU General Public License along - * with this program; if not, write the Free Software Foundation, Inc., - * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. - * - */ -/* - * This is a test for a glibc bug for the clone(2) system call. */ -#if defined UCLINUX && !__THROW -/* workaround for libc bug */ -#define __THROW -#endif +/*\ + * [Description] + * This is a test for a glibc bug for the clone(2) system call. + */ -#include <errno.h> #include <sched.h> -#include <sys/wait.h> -#include "test.h" +#include <stdio.h> +#include <stdlib.h> +#include "tst_test.h" +#include "lapi/syscalls.h" #include "clone_platform.h" -#define TRUE 1 -#define FALSE 0 - -static void setup(); -static int do_child(); - -char *TCID = "clone07"; -int TST_TOTAL = 1; - -static void sigsegv_handler(int); -static void sigusr2_handler(int); static int child_pid; -static int fail = FALSE; +static int fail; +static void *child_stack; -int main(int ac, char **av) +static int do_child(void *arg LTP_ATTRIBUTE_UNUSED) { + return 0; +} - int lc, status; - void *child_stack; - - tst_parse_opts(ac, av, NULL, NULL); - - setup(); +static void sigsegv_handler(int sig LTP_ATTRIBUTE_UNUSED) +{ + if (child_pid == 0) { + kill(getppid(), SIGUSR2); + _exit(42); + } +} - for (lc = 0; TEST_LOOPING(lc); lc++) { - tst_count = 0; - child_stack = malloc(CHILD_STACK_SIZE); - if (child_stack == NULL) - tst_brkm(TBROK, NULL, - "Cannot allocate stack for child"); +static void sigusr2_handler(int sig LTP_ATTRIBUTE_UNUSED) +{ + if (child_pid != 0) + fail = 1; +} - child_pid = ltp_clone(SIGCHLD, do_child, NULL, - CHILD_STACK_SIZE, child_stack); +static void verify_clone(void) +{ + TST_EXP_PID_SILENT(ltp_clone(SIGCHLD, do_child, NULL, CHILD_STACK_SIZE, + child_stack)); - if (child_pid < 0) - tst_brkm(TBROK | TERRNO, NULL, "clone failed"); + child_pid = TST_RET; - if ((wait(&status)) == -1) - tst_brkm(TBROK | TERRNO, NULL, - "wait failed, status: %d", status); + if (!TST_PASS) + return; - free(child_stack); - } + tst_reap_children(); - if (fail == FALSE) - tst_resm(TPASS, - "Use of return() in child did not cause SIGSEGV"); - else - tst_resm(TFAIL, "Use of return() in child caused SIGSEGV"); + TST_EXP_VAL(fail, 0, "Use of return() in child did not cause SIGSEGV"); - tst_exit(); } static void setup(void) @@ -88,41 +60,26 @@ static void setup(void) struct sigaction def_act; struct sigaction act; - TEST_PAUSE; - act.sa_handler = sigsegv_handler; act.sa_flags = SA_RESTART; - sigemptyset(&act.sa_mask); - if ((sigaction(SIGSEGV, &act, NULL)) == -1) - tst_resm(TWARN | TERRNO, - "sigaction() for SIGSEGV failed in test_setup()"); + SAFE_SIGEMPTYSET(&act.sa_mask); + SAFE_SIGACTION(SIGSEGV, &act, NULL); - /* Setup signal handler for SIGUSR2 */ def_act.sa_handler = sigusr2_handler; def_act.sa_flags = SA_RESTART | SA_RESETHAND; - sigemptyset(&def_act.sa_mask); - - if ((sigaction(SIGUSR2, &def_act, NULL)) == -1) - tst_resm(TWARN | TERRNO, - "sigaction() for SIGUSR2 failed in test_setup()"); -} + SAFE_SIGEMPTYSET(&def_act.sa_mask); + SAFE_SIGACTION(SIGUSR2, &def_act, NULL); -static int do_child(void) -{ - return 0; + child_stack = SAFE_MALLOC(CHILD_STACK_SIZE); } -static void sigsegv_handler(int sig) +static void cleanup(void) { - if (child_pid == 0) { - kill(getppid(), SIGUSR2); - _exit(42); - } + free(child_stack); } -/* sig_default_handler() - Default handler for parent */ -static void sigusr2_handler(int sig) -{ - if (child_pid != 0) - fail = TRUE; -} +static struct tst_test test = { + .setup = setup, + .test_all = verify_clone, + .cleanup = cleanup, +};
Signed-off-by: zhanglianjie <zhanglianjie@uniontech.com> -- 2.20.1