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

[Petr Vorel <pvorel@suse.cz>]

Hi Wei,

Generally LGTM, with few mostly formatting notes below.
Thanks!

Reviewed-by: Petr Vorel <pvorel@suse.cz>

> diff --git a/testcases/kernel/syscalls/ptrace/ptrace05.c b/testcases/kernel/syscalls/ptrace/ptrace05.c
...
> +/*\
> + * [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.
> + * This test ptraces itself as per arbitrarily specified signals,
> + * over 0 to SIGRTMAX range.
>   *
nit: could you please pay a bit of attention to the blank lines like this one?
It's also in print_dbg_sig() and test_signal().
> - ******************************************************************************
>   */

> -int usage(const char *);
> -
> -int usage(const char *argv0)
> +static void print_dbg_sig(int signum)
>  {
> -	fprintf(stderr, "usage: %s [start-signum] [end-signum]\n", argv0);
> -	return 1;
> +
> +	char const *strsig = tst_strsig(signum);
> +
> +	if (strstr(strsig, "???")) {
> +		tst_res(TDEBUG, "[child] Sending kill(.., %d)",
> +				signum);
> +	} else {
> +		tst_res(TDEBUG, "[child] Sending kill(.., %s)",
> +				strsig);
nit: this could be in a single line (below 80 or 90 chars), thus without {}
(readability).

static void print_dbg_sig(int signum)
{
	char const *strsig = tst_strsig(signum);

	if (strstr(strsig, "???"))
		tst_res(TDEBUG, "[child] Sending kill(.., %d)", signum);
	else
		tst_res(TDEBUG, "[child] Sending kill(.., %s)", strsig);
}

> +	}
>  }

> -int main(int argc, char **argv)
> +static void test_signal(int signum)
>  {

> -	int end_signum = -1;
> -	int signum;
> -	int start_signum = -1;
>  	int status;
> -
>  	pid_t child;

> -	tst_parse_opts(argc, argv, NULL, NULL);
> +	child = SAFE_FORK();
> +
> +	if (!child) {
> +		TST_EXP_PASS_SILENT(ptrace(PTRACE_TRACEME, 0, NULL, NULL));

> -	if (start_signum == -1) {
> -		start_signum = 0;
> +		print_dbg_sig(signum);
> +		SAFE_KILL(getpid(), signum);
> +		exit(0);
>  	}
> -	if (end_signum == -1) {
> -		end_signum = SIGRTMAX;
> +
> +	SAFE_WAITPID(child, &status, 0);
> +
> +	switch (signum) {
> +	case 0:
> +		if (WIFEXITED(status)
> +				&& WEXITSTATUS(status) == 0) {
> +			tst_res(TPASS,
> +					"kill(.., 0) exited with 0, as expected.");
> +		} else {
> +			tst_res(TFAIL,
> +					"kill(.., 0) exited with unexpected %s.", tst_strstatus(status));
> +		}
> +		break;
> +	case SIGKILL:
> +		if (WIFSIGNALED(status) && WTERMSIG(status) == SIGKILL)
> +			tst_res(TPASS, "Child killed by SIGKILL");
> +		else
> +			tst_res(TFAIL, "Child %s", tst_strstatus(status));
> +		break;
> +		/* All other processes should be stopped. */
> +	default:
> +		if (WIFSTOPPED(status))
> +			tst_res(TDEBUG, "Stopped as expected");
> +		else {
nit: if else uses { }, if should use them as well.

https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces

	This does not apply if only one branch of a conditional statement is a single
	statement; in the latter case use braces in both branches

> +			tst_res(TFAIL, "Didn't stop as expected. Child %s", tst_strstatus(status));
> +			expect_stop++;
> +		}
> +		break;
>  	}
...

> +	if (expect_stop == 0)
nit: if (!expect_stop)

> +		tst_res(TPASS, "Stopped as expected");
> +	else
> +		tst_res(TFAIL, "Didn't stop as expected, total %d cases failed", expect_stop);
>  }
We don't need another TFAIL it just repeats another TFAIL.

IMHO also TPASS should not be needed (we always get TPASS/TFAIL result for
signum == 0), but it does not harm.

Kind regards,
Petr

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