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

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> +	for (signum = 0; signum <= SIGRTMAX; signum++) {

I would put the code in this for loop into a function so that we save
some indentation as:

	for (signum = 0; signum <= SIGRTMAX; signum++) {
		if (signum >= __SIGRTMIN && signum < SIGRTMIN)
			continue;

		test_signal(sig);
	}
>  
> -		switch (child = fork()) {
> -		case -1:
> -			tst_brkm(TBROK | TERRNO, NULL, "fork() failed");
> -		case 0:
> +		child = SAFE_FORK();
>  
> -			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);
> -				}
> +		if (child == 0) {
> +			TEST(ptrace(PTRACE_TRACEME, 0, NULL, NULL));

TST_EXP_PASS()?

I guess it would look like:


	if (!child) {
		TST_EXP_PASS(ptrace(PTRACE_TRACEME, 0, NULL, NULL);

		if (!TST_PASS)
			exit(0);

		tst_res(...);

		SAFE_KILL(getpid(), signum);
	} else {
		...
	}

> +			if (TST_RET != -1) {
> +				tst_res(TINFO, "[child] Sending kill(.., %d)",
> +						signum);

Maybe TDEBUG instead of TINFO to avoid overly long output?

And we can use tst_strsig() to print the signal name as well.

> +				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,
> -					 "Failed to ptrace(PTRACE_TRACEME, ...) "
> -					 "properly");
> -
> +				tst_brk(TFAIL | TERRNO,
> +						"Failed to ptrace(PTRACE_TRACEME, ...) "
> +						"properly");
>  			}
> +
>  			/* Shouldn't get here if signum == 0. */
>  			exit((signum == 0 ? 0 : 2));

We do not use the exit value for anything in the new code, so this can
be just 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,
> -						 "kill(.., 0) exited "
> -						 "with 0, as expected.");
> -				} else {
> -					tst_resm(TFAIL,
> -						 "kill(.., 0) didn't exit "
> -						 "with 0.");
> -				}
> -				break;
> -			case SIGKILL:
> -				if (WIFSIGNALED(status)) {
> -					/* SIGKILL must be uncatchable. */
> -					if (WTERMSIG(status) == SIGKILL) {
> -						tst_resm(TPASS,
> -							 "Killed with SIGKILL, "
> -							 "as expected.");
> -					} else {
> -						tst_resm(TPASS,
> -							 "Didn't die with "
> -							 "SIGKILL (?!) ");
> -					}
> -				} else if (WIFEXITED(status)) {
> -					tst_resm(TFAIL,
> -						 "Exited unexpectedly instead "
> -						 "of dying with SIGKILL.");
> -				} else if (WIFSTOPPED(status)) {
> -					tst_resm(TFAIL,
> -						 "Stopped instead of dying "
> -						 "with SIGKILL.");
> -				}
> -				break;
> -				/* All other processes should be stopped. */
> -			default:
> -				if (WIFSTOPPED(status)) {
> -					tst_resm(TPASS, "Stopped as expected");
> +		switch (signum) {
> +		case 0:
> +			if (WIFEXITED(status)
> +					&& WEXITSTATUS(status) == 0) {
> +				tst_res(TPASS,
> +						"kill(.., 0) exited with 0, as expected.");
> +			} else {
> +				tst_brk(TFAIL | TERRNO,
> +						"kill(.., 0) didn't exit with 0.");

We do have tst_strstatus() so that we can print what was the reason
child did exit, please use it this TFAIL message.

> +			}
> +			break;
> +		case SIGKILL:
> +			if (WIFSIGNALED(status)) {
> +				/* SIGKILL must be uncatchable. */
> +				if (WTERMSIG(status) == SIGKILL) {
> +					tst_res(TPASS,
> +							"Killed with SIGKILL, as expected.");
>  				} else {
> -					tst_resm(TFAIL, "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));
> -					}
> -
> +					tst_brk(TFAIL | TERRNO,
> +							"Didn't die with SIGKILL (?!) ");
>  				}
> -
> -				break;
> -
> +			} else if (WIFEXITED(status)) {
> +				tst_res(TFAIL | TERRNO,
> +						"Exited unexpectedly instead of dying with SIGKILL.");
> +			} else if (WIFSTOPPED(status)) {
> +				tst_res(TFAIL | TERRNO,
> +						"Stopped instead of dying with SIGKILL.");

And here as well, this whole part can be replaced by just:

		if (WIFSIGNALED(status) && WTERMSIG(status) == SIGKILL)
			tst_res(TPASS, "Child killed by SIGKILL");
		else
			tst_res(TFAIL, "Child %s", tst_strstatus(status));

Also notice that we shouldn't print errno in this case, so no TERRNO
flag there.

> -
> +			break;
> +			/* All other processes should be stopped. */
> +		default:
> +			if (WIFSTOPPED(status))
> +				tst_res(TPASS, "Stopped as expected");
> +			else
> +				tst_res(TFAIL | TERRNO, "Didn't stop as expected.");

Here as well, make use of the tst_strstatus() please.

> +			break;
>  		}
> -		/* Make sure the child dies a quick and painless death ... */
> -		kill(child, 9);
>  
> +		if (signum != 0 && signum != SIGKILL)
> +			SAFE_PTRACE(PTRACE_CONT, child, NULL, NULL);
>  	}
> -
> -	tst_exit();
> -
>  }
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.forks_child = 1,
> +};
> -- 
> 2.35.3
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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