On 15 April 2014 10:09, Bernhard Voelker <mail@xxxxxxxxxxxxxxxxxxx> wrote: > On 04/14/2014 09:00 PM, Sami Kerola wrote: >> >> On 14 April 2014 17:14, Bernhard Voelker <mail@xxxxxxxxxxxxxxxxxxx> 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. Probably not needed. Since this commit I have not had a single failure. https://github.com/kerolasa/lelux-utiliteetit/commit/81143d7ad2bd5ac8fe86e0738f3e7a21e2c6f06c >> 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. Possibly because of the errno. > 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: Using directory and pid name is deterministic enough. Alternatively the test can force what is expected witness file name, just as the su(1) version of the test does. >> + 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. Yes, I found pid determination too clunky as well so the explicit witness file path was added to signalreceiver. Due the large number of changes to these patches I will resubmit them all again. -- Sami Kerola http://www.iki.fi/kerolasa/ -- To unsubscribe from this list: send the line "unsubscribe util-linux" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
