Re: [LTP] [PATCH v1 1/2] ptrace05: Refactor the test using new LTP API

[Richard Palethorpe <rpalethorpe@suse.de>]

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


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp