| Message ID | 20230925112245.30701-2-wegao@suse.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ptrace: Refactor | expand |
Hello, Wei Gao via ltp <ltp@lists.linux.it> writes: > Signed-off-by: Wei Gao <wegao@suse.com> > --- > testcases/kernel/syscalls/ptrace/ptrace05.c | 147 ++++++-------------- > 1 file changed, 39 insertions(+), 108 deletions(-) > > diff --git a/testcases/kernel/syscalls/ptrace/ptrace05.c b/testcases/kernel/syscalls/ptrace/ptrace05.c > index 54cfa4d7b..4904b959c 100644 > --- a/testcases/kernel/syscalls/ptrace/ptrace05.c > +++ b/testcases/kernel/syscalls/ptrace/ptrace05.c > @@ -1,122 +1,67 @@ > +// SPDX-License-Identifier: GPL-2.0-only > /* > - ****************************************************************************** > - * > - * ptrace05 - an app which ptraces itself as per arbitrarily specified signals, > - * over a user specified range. > - * > - * Copyright (C) 2009, Ngie Cooper > - * > - * 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. > + * Copyright (C) 2009, Ngie Cooper > + * Copyright (c) 2023 Wei Gao <wegao@suse.com> > + */ > + > +/*\ > + * [Description] > * > - * 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. > + * ptrace05 - an app which ptraces itself as per arbitrarily specified signals > * > - ****************************************************************************** > */ > > -#include <sys/types.h> > -#include <sys/wait.h> > -#include <signal.h> > -#include <errno.h> > -#include <libgen.h> > -#include <math.h> > #include <stdlib.h> > -#include <stdio.h> > -#include <string.h> > -#include <unistd.h> > - > #include <config.h> > #include "ptrace.h" > > -#include "test.h" > #include "lapi/signal.h" > +#include "tst_test.h" > > -char *TCID = "ptrace05"; > -int TST_TOTAL = 0; > - > -int usage(const char *); > - > -int usage(const char *argv0) > -{ > - fprintf(stderr, "usage: %s [start-signum] [end-signum]\n", argv0); > - return 1; > -} > - > -int main(int argc, char **argv) > +static void run(void) > { > > - int end_signum = -1; > - int signum; > - int start_signum = -1; > + int end_signum = SIGRTMAX; > + int signum = 0; > + int start_signum = 0; > int status; {start,end}_signum don't appear to serve a purpose anymore. > > pid_t child; > > - tst_parse_opts(argc, argv, NULL, NULL); > - > - if (start_signum == -1) { > - start_signum = 0; > - } > - if (end_signum == -1) { > - end_signum = SIGRTMAX; > - } > - > for (signum = start_signum; signum <= end_signum; signum++) { > > - if (signum >= __SIGRTMIN && signum < SIGRTMIN) > - continue; Why can this be removed? I remember we had an issue on some systems because some signals are reserved by libc. > - > - switch (child = fork()) { > + switch (child = SAFE_FORK()) { > case -1: > - tst_brkm(TBROK | TERRNO, NULL, "fork() failed"); > + tst_brk(TBROK | TERRNO, "fork() failed"); > case 0: > > - if (ptrace(PTRACE_TRACEME, 0, NULL, NULL) != -1) { > - tst_resm(TINFO, "[child] Sending kill(.., %d)", > - signum); > - if (kill(getpid(), signum) < 0) { > - tst_resm(TINFO | TERRNO, > - "[child] kill(.., %d) failed.", > - signum); > - } > + TEST(ptrace(PTRACE_TRACEME, 0, NULL, NULL)); > + if (TST_RET != -1) { > + tst_res(TINFO, "[child] Sending kill(.., %d)", > + signum); > + SAFE_KILL(getpid(), signum); > } else { > - > - /* > - * This won't increment the TST_COUNT var. > - * properly, but it'll show up as a failure > - * nonetheless. > - */ > - tst_resm(TFAIL | TERRNO, > + tst_brk(TFAIL | TERRNO, > "Failed to ptrace(PTRACE_TRACEME, ...) " > "properly"); > - > } > - /* Shouldn't get here if signum == 0. */ > - exit((signum == 0 ? 0 : 2)); > + > + exit(0); > break; > > default: > > - waitpid(child, &status, 0); > + SAFE_WAITPID(child, &status, 0); > > switch (signum) { > case 0: > if (WIFEXITED(status) > && WEXITSTATUS(status) == 0) { > - tst_resm(TPASS, > + tst_res(TPASS, > "kill(.., 0) exited " > "with 0, as expected."); > } else { > - tst_resm(TFAIL, > + tst_brk(TFAIL | TERRNO, > "kill(.., 0) didn't exit " > "with 0."); > } > @@ -125,20 +70,20 @@ int main(int argc, char **argv) > if (WIFSIGNALED(status)) { > /* SIGKILL must be uncatchable. */ > if (WTERMSIG(status) == SIGKILL) { > - tst_resm(TPASS, > + tst_res(TPASS, > "Killed with SIGKILL, " > "as expected."); > } else { > - tst_resm(TPASS, > + tst_brk(TFAIL | TERRNO, > "Didn't die with " > "SIGKILL (?!) "); > } > } else if (WIFEXITED(status)) { > - tst_resm(TFAIL, > + tst_brk(TFAIL | TERRNO, > "Exited unexpectedly instead " > "of dying with SIGKILL."); > } else if (WIFSTOPPED(status)) { > - tst_resm(TFAIL, > + tst_brk(TFAIL | TERRNO, > "Stopped instead of dying " > "with SIGKILL."); > } > @@ -146,35 +91,21 @@ int main(int argc, char **argv) > /* All other processes should be stopped. */ > default: > if (WIFSTOPPED(status)) { > - tst_resm(TPASS, "Stopped as expected"); > + tst_res(TPASS, "Stopped as expected"); > } else { > - tst_resm(TFAIL, "Didn't stop as " > + tst_brk(TFAIL | TERRNO, "Didn't stop as " > "expected."); > - if (kill(child, 0)) { > - tst_resm(TINFO, > - "Is still alive!?"); > - } else if (WIFEXITED(status)) { > - tst_resm(TINFO, > - "Exited normally"); > - } else if (WIFSIGNALED(status)) { > - tst_resm(TINFO, > - "Was signaled with " > - "signum=%d", > - WTERMSIG(status)); > - } > - > } > - > break; > - > } > - > } > - /* Make sure the child dies a quick and painless death ... */ > - kill(child, 9); > > + if (signum != 0 && signum != 9) > + SAFE_PTRACE(PTRACE_CONT, child, NULL, NULL); nit; it's clearer to write SIGKILL than 9 also some other signals change number between platforms. > } > - > - tst_exit(); > - > } > + > +static struct tst_test test = { > + .test_all = run, > + .forks_child = 1, > +}; > -- > 2.35.3
Hi Wei, make check-ptrace05 shows various redefinitions, that points on ptrace.h cleanup needed, I'll fix it as a separate change. CHECK testcases/kernel/syscalls/ptrace/ptrace05.c ptrace05.c: note: in included file (through ptrace.h): /usr/include/linux/ptrace.h:50:9: warning: preprocessor token PTRACE_GETREGSET redefined ptrace05.c: note: in included file (through ptrace.h): /usr/include/sys/ptrace.h:153:9: this was the original definition ptrace05.c: note: in included file (through ptrace.h): /usr/include/linux/ptrace.h:51:9: warning: preprocessor token PTRACE_SETREGSET redefined ptrace05.c: note: in included file (through ptrace.h): /usr/include/sys/ptrace.h:157:9: this was the original definition ptrace05.c: note: in included file (through ptrace.h): /usr/include/linux/ptrace.h:53:9: warning: preprocessor token PTRACE_SEIZE redefined I handled this in separate patchset [3], I Cc you. Could you please base v2 on it? I hope it will be merged soon. [1] https://sourceware.org/glibc/wiki/Synchronizing_Headers [2] https://github.com/linux-test-project/ltp/wiki/Supported-kernel,-libc,-toolchain-versions#11-oldest-tested-distributions [3] https://patchwork.ozlabs.org/project/ltp/list/?series=384172&state=* > Signed-off-by: Wei Gao <wegao@suse.com> > --- > testcases/kernel/syscalls/ptrace/ptrace05.c | 147 ++++++-------------- > 1 file changed, 39 insertions(+), 108 deletions(-) > diff --git a/testcases/kernel/syscalls/ptrace/ptrace05.c b/testcases/kernel/syscalls/ptrace/ptrace05.c > index 54cfa4d7b..4904b959c 100644 > --- a/testcases/kernel/syscalls/ptrace/ptrace05.c > +++ b/testcases/kernel/syscalls/ptrace/ptrace05.c > @@ -1,122 +1,67 @@ > +// SPDX-License-Identifier: GPL-2.0-only // SPDX-License-Identifier: GPL-2.0-or-later > /* > - ****************************************************************************** > - * > - * ptrace05 - an app which ptraces itself as per arbitrarily specified signals, > - * over a user specified range. > - * > - * Copyright (C) 2009, Ngie Cooper > - * > - * 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. "any later version" is why GPL-2.0-or-later instead of GPL-2.0-only. > - * 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. > + * Copyright (C) 2009, Ngie Cooper > + * Copyright (c) 2023 Wei Gao <wegao@suse.com> Please add also LTP copyright (more people contributed to this test): * Copyright (c) Linux Test Project, 2009-2019 > + */ > + > +/*\ > + * [Description] > * > - * 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. > + * ptrace05 - an app which ptraces itself as per arbitrarily specified signals I'm not really sure for a proper description, but we don't list the test name, also s/app/test/ > * > - ****************************************************************************** > */ > -#include <sys/types.h> > -#include <sys/wait.h> > -#include <signal.h> > -#include <errno.h> > -#include <libgen.h> > -#include <math.h> > #include <stdlib.h> > -#include <stdio.h> > -#include <string.h> > -#include <unistd.h> > - > #include <config.h> > #include "ptrace.h" > -#include "test.h" > #include "lapi/signal.h" > +#include "tst_test.h" > -char *TCID = "ptrace05"; > -int TST_TOTAL = 0; > - > -int usage(const char *); > - > -int usage(const char *argv0) > -{ > - fprintf(stderr, "usage: %s [start-signum] [end-signum]\n", argv0); > - return 1; > -} > - > -int main(int argc, char **argv) > +static void run(void) > { > - int end_signum = -1; > - int signum; > - int start_signum = -1; > + int end_signum = SIGRTMAX; > + int signum = 0; > + int start_signum = 0; > int status; > pid_t child; > - tst_parse_opts(argc, argv, NULL, NULL); > - > - if (start_signum == -1) { > - start_signum = 0; > - } > - if (end_signum == -1) { > - end_signum = SIGRTMAX; > - } > - > for (signum = start_signum; signum <= end_signum; signum++) { > - if (signum >= __SIGRTMIN && signum < SIGRTMIN) > - continue; > - > - switch (child = fork()) { > + switch (child = SAFE_FORK()) { > case -1: > - tst_brkm(TBROK | TERRNO, NULL, "fork() failed"); > + tst_brk(TBROK | TERRNO, "fork() failed"); We have this handled in SAFE_FORK(), it should be removed here. Therefore switch+case should be replaced with if+else. > case 0: > - tst_resm(TFAIL | TERRNO, > + tst_brk(TFAIL | TERRNO, > "Failed to ptrace(PTRACE_TRACEME, ...) " > "properly"); > - > } ... > - /* Shouldn't get here if signum == 0. */ > - exit((signum == 0 ? 0 : 2)); > + > + exit(0); Why there can be always exit(0) ? > break; > default: > - waitpid(child, &status, 0); > + SAFE_WAITPID(child, &status, 0); > switch (signum) { > case 0: > if (WIFEXITED(status) > && WEXITSTATUS(status) == 0) { > - tst_resm(TPASS, > + tst_res(TPASS, > "kill(.., 0) exited " > "with 0, as expected."); Please join strings (this applies to code below as well). > } else { > - tst_resm(TFAIL, > + tst_brk(TFAIL | TERRNO, > "kill(.., 0) didn't exit " > "with 0."); Why not tst_res(TFAIL, ...) ? (this applies to code below as well) ... Kind regards, Petr
diff --git a/testcases/kernel/syscalls/ptrace/ptrace05.c b/testcases/kernel/syscalls/ptrace/ptrace05.c index 54cfa4d7b..4904b959c 100644 --- a/testcases/kernel/syscalls/ptrace/ptrace05.c +++ b/testcases/kernel/syscalls/ptrace/ptrace05.c @@ -1,122 +1,67 @@ +// SPDX-License-Identifier: GPL-2.0-only /* - ****************************************************************************** - * - * ptrace05 - an app which ptraces itself as per arbitrarily specified signals, - * over a user specified range. - * - * Copyright (C) 2009, Ngie Cooper - * - * 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. + * Copyright (C) 2009, Ngie Cooper + * Copyright (c) 2023 Wei Gao <wegao@suse.com> + */ + +/*\ + * [Description] * - * 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. + * ptrace05 - an app which ptraces itself as per arbitrarily specified signals * - ****************************************************************************** */ -#include <sys/types.h> -#include <sys/wait.h> -#include <signal.h> -#include <errno.h> -#include <libgen.h> -#include <math.h> #include <stdlib.h> -#include <stdio.h> -#include <string.h> -#include <unistd.h> - #include <config.h> #include "ptrace.h" -#include "test.h" #include "lapi/signal.h" +#include "tst_test.h" -char *TCID = "ptrace05"; -int TST_TOTAL = 0; - -int usage(const char *); - -int usage(const char *argv0) -{ - fprintf(stderr, "usage: %s [start-signum] [end-signum]\n", argv0); - return 1; -} - -int main(int argc, char **argv) +static void run(void) { - int end_signum = -1; - int signum; - int start_signum = -1; + int end_signum = SIGRTMAX; + int signum = 0; + int start_signum = 0; int status; pid_t child; - tst_parse_opts(argc, argv, NULL, NULL); - - if (start_signum == -1) { - start_signum = 0; - } - if (end_signum == -1) { - end_signum = SIGRTMAX; - } - for (signum = start_signum; signum <= end_signum; signum++) { - if (signum >= __SIGRTMIN && signum < SIGRTMIN) - continue; - - switch (child = fork()) { + switch (child = SAFE_FORK()) { case -1: - tst_brkm(TBROK | TERRNO, NULL, "fork() failed"); + tst_brk(TBROK | TERRNO, "fork() failed"); case 0: - if (ptrace(PTRACE_TRACEME, 0, NULL, NULL) != -1) { - tst_resm(TINFO, "[child] Sending kill(.., %d)", - signum); - if (kill(getpid(), signum) < 0) { - tst_resm(TINFO | TERRNO, - "[child] kill(.., %d) failed.", - signum); - } + TEST(ptrace(PTRACE_TRACEME, 0, NULL, NULL)); + if (TST_RET != -1) { + tst_res(TINFO, "[child] Sending kill(.., %d)", + signum); + SAFE_KILL(getpid(), signum); } else { - - /* - * This won't increment the TST_COUNT var. - * properly, but it'll show up as a failure - * nonetheless. - */ - tst_resm(TFAIL | TERRNO, + tst_brk(TFAIL | TERRNO, "Failed to ptrace(PTRACE_TRACEME, ...) " "properly"); - } - /* Shouldn't get here if signum == 0. */ - exit((signum == 0 ? 0 : 2)); + + exit(0); break; default: - waitpid(child, &status, 0); + SAFE_WAITPID(child, &status, 0); switch (signum) { case 0: if (WIFEXITED(status) && WEXITSTATUS(status) == 0) { - tst_resm(TPASS, + tst_res(TPASS, "kill(.., 0) exited " "with 0, as expected."); } else { - tst_resm(TFAIL, + tst_brk(TFAIL | TERRNO, "kill(.., 0) didn't exit " "with 0."); } @@ -125,20 +70,20 @@ int main(int argc, char **argv) if (WIFSIGNALED(status)) { /* SIGKILL must be uncatchable. */ if (WTERMSIG(status) == SIGKILL) { - tst_resm(TPASS, + tst_res(TPASS, "Killed with SIGKILL, " "as expected."); } else { - tst_resm(TPASS, + tst_brk(TFAIL | TERRNO, "Didn't die with " "SIGKILL (?!) "); } } else if (WIFEXITED(status)) { - tst_resm(TFAIL, + tst_brk(TFAIL | TERRNO, "Exited unexpectedly instead " "of dying with SIGKILL."); } else if (WIFSTOPPED(status)) { - tst_resm(TFAIL, + tst_brk(TFAIL | TERRNO, "Stopped instead of dying " "with SIGKILL."); } @@ -146,35 +91,21 @@ int main(int argc, char **argv) /* All other processes should be stopped. */ default: if (WIFSTOPPED(status)) { - tst_resm(TPASS, "Stopped as expected"); + tst_res(TPASS, "Stopped as expected"); } else { - tst_resm(TFAIL, "Didn't stop as " + tst_brk(TFAIL | TERRNO, "Didn't stop as " "expected."); - if (kill(child, 0)) { - tst_resm(TINFO, - "Is still alive!?"); - } else if (WIFEXITED(status)) { - tst_resm(TINFO, - "Exited normally"); - } else if (WIFSIGNALED(status)) { - tst_resm(TINFO, - "Was signaled with " - "signum=%d", - WTERMSIG(status)); - } - } - break; - } - } - /* Make sure the child dies a quick and painless death ... */ - kill(child, 9); + if (signum != 0 && signum != 9) + SAFE_PTRACE(PTRACE_CONT, child, NULL, NULL); } - - tst_exit(); - } + +static struct tst_test test = { + .test_all = run, + .forks_child = 1, +};
Signed-off-by: Wei Gao <wegao@suse.com> --- testcases/kernel/syscalls/ptrace/ptrace05.c | 147 ++++++-------------- 1 file changed, 39 insertions(+), 108 deletions(-)