Re: [PATCH 4/7] tests: check kill is converting signals names correctly

[Bernhard Voelker <mail@bernhard-voelker.de>]

On 04/14/2014 09:00 PM, Sami Kerola wrote:
> On 14 April 2014 17:14, Bernhard Voelker <mail@bernhard-voelker.de> wrote:
> Most of the tests could use pid, but I am afraid there should be at
> least one of them using name. Else killing by name remains untested.

What about creating a test 'kill_by_name', and avoiding complexity
in the other tests? They all have a certain purpose each, so there's
no need to put a certain aspect into all of them.

> Of course simple kill by name test could do the job, but I am not
> hugely in favor to simplify tests until they work without
> understanding why they fail in first place.

Agreed. Unfortunately, I didn't find *the* reason for the failure yet.
BTW, I often see both "all_processes" and "name_to_number" failing here.

It seems that there are several issues.
First of all, this looks odd in the receiver program:

 > +	if (!stat(argv[1], &statbuf)) {
 > +		if (S_ISDIR(statbuf.st_mode))
 > +			xasprintf(&pid_path, "%s/%d", argv[1], getpid());
 > +	} else
 > +		xasprintf(&pid_path, "%s", argv[1]);

What if the argv[1] is stat()able but not a directory?

Anyway, I'm 40:60 against letting the receiver program decide where
the witness file is located, i.e. I'd rather use deterministic names
everywhere. E.g. in the 'name_to_number' tests, the name of the witness
files could contain the signal name rather than using unsafe mktemp.

Furthermore, the open() call would return -1 upon failure instead
of 0 as assumed here:

 > +	if (!(fd = open(pid_path, O_WRONLY | O_CREAT | O_TRUNC, mode)))
 > +		err(EXIT_FAILURE, "cannot open pid file: %s", pid_path);

Re. the "all_processes" test: I'm not sure how su(1) and the intermediate
shell change the signal handling. Maybe the reason for the failure is
somehow related to this, because $! will refer to the shell's PID rather
than to that of the receiver program.
Anyway, maybe it's better to use $TS_CMD_SU instead of the local 'su',
or even adding a dedicated setuid() helper.

As already said, these are just side notes.

Have a nice day,
Berny