Re: [LTP] [PATCH v1] Refactor pidns16 test using new LTP API

[Petr Vorel <pvorel@suse.cz>]

Hi Andrea,

...
> +/*\
> + * [Description]
> + *
> + * Clone a process with CLONE_NEWPID flag and verifies that siginfo->si_pid is
You need to quote variable with +, i.e. +siginfo->si_pid+, otherwise
formatting show siginfo→si_pid instead of siginfo->si_pid.

> + * set to 0 if sender (parent process) sent the signal. Then send signal from
> + * container itself and check if siginfo->si_pid is set to 1.
and here as well.
> + */

>  #define _GNU_SOURCE 1
Do we really need _GNU_SOURCE? Why?

...
> +static int counter;
Shouldn't this be volatile? And maybe also sig_atomic_t instead of int (even
it's the same)?
static volatile sig_atomic_t counter;

> -char *TCID = "pidns16";
> -int TST_TOTAL = 3;
> -
> -void child_signal_handler(int sig, siginfo_t * si, void *unused)
> +static void child_signal_handler(LTP_ATTRIBUTE_UNUSED int sig, siginfo_t *si, LTP_ATTRIBUTE_UNUSED void *unused)
>  {
> -	static int c = 1;
> -	pid_t expected_pid;
> -
> -	/* Verifying from which process the signal handler is signalled */
> +	pid_t exp_pid;

> -	switch (c) {
> -	case 1:
> -		expected_pid = PARENT_PID;
> +	switch (counter) {
> +	case 0:
> +		exp_pid = 0;
>  		break;
> -	case 2:
> -		expected_pid = CHILD_PID;
> +	case 1:
> +		exp_pid = 1;
>  		break;
>  	default:
> -		tst_resm(TBROK, "child should NOT be signalled 3+ times");
> +		tst_brk(TBROK, "Child should NOT be signalled 3+ times");
>  		return;
>  	}

very nit: I'd use if (counter 
if (counter > 1) {
	tst_brk(TBROK, "Child should NOT be signalled 3+ times");
	return;
}
exp_pid = counter;

> +	if (si->si_pid == exp_pid)
> +		tst_res(TPASS, "Signalling PID is %d as expected", exp_pid);
>  	else
> +		tst_res(TFAIL, "Signalling PID is not %d, but %d", exp_pid, si->si_pid);
nit: use TST_EXP_PASS()

...
> +	if (cpid != 1 || ppid != 0) {
> +		tst_res(TFAIL, "Got unexpected result of cpid=%d ppid=%d", cpid, ppid);
> +		return 1;
> +	}
nit: maybe easier for reviewer if these two failures are handled separately.

I also hope we sooner or later get rid of ltp_clone_quick() in favor of
tst_clone(). Not sure where ended the discussion about it, from [1] it looks
like it's needed now. It can wait for another result after we manage to merge
pidns and mqns rewrites.

The rest LGTM. Thanks for your tireless work on rewriting LTP!

Kind regards,
Petr

[1] https://lore.kernel.org/ltp/945eb0ee-b346-5729-3dda-4bff39bb52d9@suse.com/

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