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

[Cyril Hrubis <chrubis@suse.cz>]

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

As long as we only modify the counter from within the signal handler
it's not required.

The probelm that sig_atomic_t fixes is to make sure that the load/store
is single CPU instruction so that we are sure that we don not jump into
the handler in the middle of a sequence of load/store instruction.

And the volatile "turns off" caching of the value, so again anything
that is accessed from both the handler and the rest of the code should
be volatile.

> 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;

The bigger problem though is that we call tst_brk() and tst_res() in the
handler, which is not safe at all. The are couple things that can go
wrong here, mostly glibc locks can get deadlocked for the underlying
FILE* and if the underlying fprintf() calls malloc() the malloc data
structures may end up corrupted. It's quite rare for this to happen, but
there have been a few tests that were failing with small probability due
to these mistakes.

-- 
Cyril Hrubis
chrubis@suse.cz

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