[PATCH 0/7] kill: add regression tests

[PATCH 0/7] kill: add regression tests

[Sami Kerola]

Hello,

Couple days ago I promised to deliver some kill regression tests so here
they come.

The promise: http://www.spinics.net/lists/util-linux-ng/msg09099.html

Sami Kerola (7):
  kill: make options --pid and --queue mutually exclusive
  kill: remove unnecessary indirection
  tests: add signal receiver program
  tests: check kill is converting signals names correctly
  tests: check various ways to specify kill signal
  tests: check kill print pid option
  tests: check kill all user processes

 misc-utils/kill.c                  |  10 ++-
 tests/commands.sh                  |   4 ++
 tests/expected/kill/all_processes  |   5 ++
 tests/expected/kill/name_to_number |   1 +
 tests/expected/kill/options        |   1 +
 tests/expected/kill/print_pid      |   1 +
 tests/helpers/Makemodule.am        |   3 +
 tests/helpers/test_sigreceive.c    | 126 +++++++++++++++++++++++++++++++++++++
 tests/ts/kill/all_processes        |  57 +++++++++++++++++
 tests/ts/kill/name_to_number       |  61 ++++++++++++++++++
 tests/ts/kill/options              |  63 +++++++++++++++++++
 tests/ts/kill/print_pid            |  56 +++++++++++++++++
 12 files changed, 385 insertions(+), 3 deletions(-)
 create mode 100644 tests/expected/kill/all_processes
 create mode 100644 tests/expected/kill/name_to_number
 create mode 100644 tests/expected/kill/options
 create mode 100644 tests/expected/kill/print_pid
 create mode 100644 tests/helpers/test_sigreceive.c
 create mode 100755 tests/ts/kill/all_processes
 create mode 100755 tests/ts/kill/name_to_number
 create mode 100755 tests/ts/kill/options
 create mode 100755 tests/ts/kill/print_pid

-- 
1.9.2

[PATCH 1/7] kill: make options --pid and --queue mutually exclusive

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 misc-utils/kill.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/misc-utils/kill.c b/misc-utils/kill.c
index d461b36..e678661 100644
--- a/misc-utils/kill.c
+++ b/misc-utils/kill.c
@@ -381,6 +381,10 @@ static char **parse_arguments(int argc, char **argv, struct kill_control *ctl)
 			ctl->do_pid = 1;
 			if (ctl->do_kill)
 				errx(EXIT_FAILURE, _("%s and %s are mutually exclusive"), "--pid", "--signal");
+#ifdef HAVE_SIGQUEUE
+			if (ctl->use_sigval)
+				errx(EXIT_FAILURE, _("%s and %s are mutually exclusive"), "--pid", "--queue");
+#endif
 			continue;
 		}
 		if (!strcmp(arg, "-s") || !strcmp(arg, "--signal")) {
@@ -399,6 +403,8 @@ static char **parse_arguments(int argc, char **argv, struct kill_control *ctl)
 		if (!strcmp(arg, "-q") || !strcmp(arg, "--queue")) {
 			if (argc < 2)
 				errx(EXIT_FAILURE, _("option '%s' requires an argument"), arg);
+			if (ctl->do_pid)
+				errx(EXIT_FAILURE, _("%s and %s are mutually exclusive"), "--pid", "--queue");
 			argc--, argv++;
 			arg = *argv;
 			if ((ctl->numsig = arg_to_signum(arg, 0)) < 0)
-- 
1.9.2

Re: [PATCH 1/7] kill: make options --pid and --queue mutually exclusive

[Bernhard Voelker]

On 04/12/2014 12:28 PM, Sami Kerola wrote:
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
> ---
>  misc-utils/kill.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/misc-utils/kill.c b/misc-utils/kill.c
> index d461b36..e678661 100644
> --- a/misc-utils/kill.c
> +++ b/misc-utils/kill.c
> @@ -381,6 +381,10 @@ static char **parse_arguments(int argc, char **argv, struct kill_control *ctl)
>  			ctl->do_pid = 1;
>  			if (ctl->do_kill)
>  				errx(EXIT_FAILURE, _("%s and %s are mutually exclusive"), "--pid", "--signal");
> +#ifdef HAVE_SIGQUEUE
> +			if (ctl->use_sigval)
> +				errx(EXIT_FAILURE, _("%s and %s are mutually exclusive"), "--pid", "--queue");
> +#endif
>  			continue;
>  		}
>  		if (!strcmp(arg, "-s") || !strcmp(arg, "--signal")) {
> @@ -399,6 +403,8 @@ static char **parse_arguments(int argc, char **argv, struct kill_control *ctl)
>  		if (!strcmp(arg, "-q") || !strcmp(arg, "--queue")) {
>  			if (argc < 2)
>  				errx(EXIT_FAILURE, _("option '%s' requires an argument"), arg);
> +			if (ctl->do_pid)
> +				errx(EXIT_FAILURE, _("%s and %s are mutually exclusive"), "--pid", "--queue");
>  			argc--, argv++;
>  			arg = *argv;
>  			if ((ctl->numsig = arg_to_signum(arg, 0)) < 0)
> 

Ain't this a good case for
  err_exclusive_options(c, longopts, excl, excl_st);
?

Have a nice day,
Berny

Re: [PATCH 1/7] kill: make options --pid and --queue mutually exclusive

[Sami Kerola]

On 12 April 2014 16:57, Bernhard Voelker <mail@bernhard-voelker.de> wrote:
> On 04/12/2014 12:28 PM, Sami Kerola wrote:
>> +                     if (ctl->use_sigval)
>> +                             errx(EXIT_FAILURE, _("%s and %s are mutually exclusive"), "--pid", "--queue");
>
> Ain't this a good case for
>   err_exclusive_options(c, longopts, excl, excl_st);

Hello Berny,

Yes if kill would use getopts_long(). The kill should probably have
long options, but that is a different change than mutual option
exclusion.

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

[PATCH 2/7] kill: remove unnecessary indirection

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 misc-utils/kill.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/misc-utils/kill.c b/misc-utils/kill.c
index e678661..9566f14 100644
--- a/misc-utils/kill.c
+++ b/misc-utils/kill.c
@@ -434,8 +434,6 @@ static char **parse_arguments(int argc, char **argv, struct kill_control *ctl)
 	}
 	if (!*argv)
 		errx(EXIT_FAILURE, _("not enough arguments"));
-	if (ctl->do_pid)
-		ctl->numsig = -1;
 	return argv;
 }
 
@@ -444,7 +442,7 @@ static int kill_verbose(const struct kill_control *ctl)
 {
 	int rc = 0;
 
-	if (ctl->numsig < 0) {
+	if (ctl->do_pid) {
 		printf("%ld\n", (long) ctl->pid);
 		return 0;
 	}
-- 
1.9.2

[PATCH 3/7] tests: add signal receiver program

[Sami Kerola]

Target to kill with a check that will be wrote later.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 tests/helpers/Makemodule.am     |   3 +
 tests/helpers/test_sigreceive.c | 126 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 129 insertions(+)
 create mode 100644 tests/helpers/test_sigreceive.c

diff --git a/tests/helpers/Makemodule.am b/tests/helpers/Makemodule.am
index 9724dae..0b927e9 100644
--- a/tests/helpers/Makemodule.am
+++ b/tests/helpers/Makemodule.am
@@ -10,3 +10,6 @@ test_pathnames_SOURCES = tests/helpers/test_pathnames.c
 
 check_PROGRAMS += test_sysinfo
 test_sysinfo_SOURCES = tests/helpers/test_sysinfo.c
+
+check_PROGRAMS += test_sigreceive
+test_sigreceive_SOURCES = tests/helpers/test_sigreceive.c
diff --git a/tests/helpers/test_sigreceive.c b/tests/helpers/test_sigreceive.c
new file mode 100644
index 0000000..a2d3f53
--- /dev/null
+++ b/tests/helpers/test_sigreceive.c
@@ -0,0 +1,126 @@
+/*
+ * test_sigreceive - wait for signal and exit with value of it
+ *
+ * Written by Sami Kerola <kerolasa@iki.fi>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * 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.
+ */
+
+#include <signal.h>
+#include <sys/select.h>
+#include <unistd.h>
+
+static __attribute__ ((__noreturn__))
+void exiter(int sig)
+{
+	_exit(sig);
+}
+
+int main(void)
+{
+	struct sigaction sigact;
+	fd_set rfds;
+	struct timeval timeout;
+
+	sigemptyset(&sigact.sa_mask);
+	sigact.sa_flags = 0;
+	sigact.sa_handler = exiter;
+	timeout.tv_sec = 2;
+	timeout.tv_usec = 0;
+
+	sigaction(SIGHUP, &sigact, NULL);
+	sigaction(SIGINT, &sigact, NULL);
+	sigaction(SIGQUIT, &sigact, NULL);
+	sigaction(SIGILL, &sigact, NULL);
+#ifdef SIGTRAP
+	sigaction(SIGTRAP, &sigact, NULL);
+#endif
+	sigaction(SIGABRT, &sigact, NULL);
+#ifdef SIGIOT
+	sigaction(SIGIOT, &sigact, NULL);
+#endif
+#ifdef SIGEMT
+	sigaction(SIGEMT, &sigact, NULL);
+#endif
+#ifdef SIGBUS
+	sigaction(SIGBUS, &sigact, NULL);
+#endif
+	sigaction(SIGFPE, &sigact, NULL);
+	sigaction(SIGKILL, &sigact, NULL);
+	sigaction(SIGUSR1, &sigact, NULL);
+	sigaction(SIGSEGV, &sigact, NULL);
+	sigaction(SIGUSR2, &sigact, NULL);
+	sigaction(SIGPIPE, &sigact, NULL);
+	sigaction(SIGALRM, &sigact, NULL);
+	sigaction(SIGTERM, &sigact, NULL);
+#ifdef SIGSTKFLT
+	sigaction(SIGSTKFLT, &sigact, NULL);
+#endif
+	sigaction(SIGCHLD, &sigact, NULL);
+#ifdef SIGCLD
+	sigaction(SIGCLD, &sigact, NULL);
+#endif
+	sigaction(SIGCONT, &sigact, NULL);
+	sigaction(SIGSTOP, &sigact, NULL);
+	sigaction(SIGTSTP, &sigact, NULL);
+	sigaction(SIGTTIN, &sigact, NULL);
+	sigaction(SIGTTOU, &sigact, NULL);
+#ifdef SIGURG
+	sigaction(SIGURG, &sigact, NULL);
+#endif
+#ifdef SIGXCPU
+	sigaction(SIGXCPU, &sigact, NULL);
+#endif
+#ifdef SIGXFSZ
+	sigaction(SIGXFSZ, &sigact, NULL);
+#endif
+#ifdef SIGVTALRM
+	sigaction(SIGVTALRM, &sigact, NULL);
+#endif
+#ifdef SIGPROF
+	sigaction(SIGPROF, &sigact, NULL);
+#endif
+#ifdef SIGWINCH
+	sigaction(SIGWINCH, &sigact, NULL);
+#endif
+#ifdef SIGIO
+	sigaction(SIGIO, &sigact, NULL);
+#endif
+#ifdef SIGPOLL
+	sigaction(SIGPOLL, &sigact, NULL);
+#endif
+#ifdef SIGINFO
+	sigaction(SIGINFO, &sigact, NULL);
+#endif
+#ifdef SIGLOST
+	sigaction(SIGLOST, &sigact, NULL);
+#endif
+#ifdef SIGPWR
+	sigaction(SIGPWR, &sigact, NULL);
+#endif
+#ifdef SIGUNUSED
+	sigaction(SIGUNUSED, &sigact, NULL);
+#endif
+#ifdef SIGSYS
+	sigaction(SIGSYS, &sigact, NULL);
+#endif
+#ifdef SIGRTMIN
+	sigaction(SIGRTMIN+0, &sigact, NULL);
+	sigaction(SIGRTMAX+0, &sigact, NULL);
+#endif
+	FD_ZERO(&rfds);
+	FD_SET(0, &rfds);
+	select(STDIN_FILENO, &rfds, NULL, NULL, &timeout);
+	return 0;
+}
-- 
1.9.2

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

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 tests/commands.sh                  |  3 ++
 tests/expected/kill/name_to_number |  1 +
 tests/ts/kill/name_to_number       | 61 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+)
 create mode 100644 tests/expected/kill/name_to_number
 create mode 100755 tests/ts/kill/name_to_number

diff --git a/tests/commands.sh b/tests/commands.sh
index 68048fa..c67aa91 100644
--- a/tests/commands.sh
+++ b/tests/commands.sh
@@ -21,6 +21,7 @@ TS_HELPER_MD5="$top_builddir/test_md5"
 TS_HELPER_MORE=${TS_HELPER_MORE-"$top_builddir/test_more"}
 TS_HELPER_PARTITIONS="$top_builddir/sample-partitions"
 TS_HELPER_PATHS="$top_builddir/test_pathnames"
+TS_HELPER_SIGRECEIVE="$top_builddir/test_sigreceive"
 TS_HELPER_STRUTILS="$top_builddir/test_strutils"
 TS_HELPER_SYSINFO="$top_builddir/test_sysinfo"
 
@@ -37,6 +38,7 @@ TS_CMD_EJECT=${TS_CMD_EJECT-"$top_builddir/eject"}
 TS_CMD_FALLOCATE=${TS_CMD_FALLOCATE-"$top_builddir/fallocate"}
 TS_CMD_FDISK=${TS_CMD_FDISK-"$top_builddir/fdisk"}
 TS_CMD_FINDMNT=${TS_CMD_FINDMNT-"$top_builddir/findmnt"}
+TS_CMD_FLOCK=${TS_CMD_FLOCK:-"$top_builddir/flock"}
 TS_CMD_FSCKCRAMFS=${TS_CMD_FSCKCRAMFS:-"$top_builddir/test_fsck.cramfs"}
 TS_CMD_FSCKMINIX=${TS_CMD_FSCKMINIX:-"$top_builddir/fsck.minix"}
 TS_CMD_GETOPT=${TS_CMD_GETOPT-"$top_builddir/getopt"}
@@ -47,6 +49,7 @@ TS_CMD_IPCMK=${TS_CMD_IPCMK-"$top_builddir/ipcmk"}
 TS_CMD_IPCRM=${TS_CMD_IPCRM-"$top_builddir/ipcrm"}
 TS_CMD_IPCS=${TS_CMD_IPCS:-"$top_builddir/ipcs"}
 TS_CMD_ISOSIZE=${TS_CMD_ISOSIZE-"$top_builddir/isosize"}
+TS_CMD_KILL=${TS_CMD_KILL-"$top_builddir/kill"}
 TS_CMD_LAST=${TS_CMD_LAST-"$top_builddir/last"}
 TS_CMD_LINE=${TS_CMD_LINE-"$top_builddir/line"}
 TS_CMD_LOOK=${TS_CMD_LOOK-"$top_builddir/look"}
diff --git a/tests/expected/kill/name_to_number b/tests/expected/kill/name_to_number
new file mode 100644
index 0000000..d48ce72
--- /dev/null
+++ b/tests/expected/kill/name_to_number
@@ -0,0 +1 @@
+all ok
diff --git a/tests/ts/kill/name_to_number b/tests/ts/kill/name_to_number
new file mode 100755
index 0000000..b6db93a
--- /dev/null
+++ b/tests/ts/kill/name_to_number
@@ -0,0 +1,61 @@
+#!/bin/bash
+
+# This file is part of util-linux.
+#
+# This file is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This file is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+TS_TOPDIR="${0%/*}/../.."
+TS_DESC="name_to_number"
+
+. $TS_TOPDIR/functions.sh
+ts_init "$*"
+
+ts_check_test_command "$TS_CMD_KILL"
+ts_check_test_command "$TS_CMD_FLOCK"
+all_ok=true
+
+# parallel kill checks could kill wrong sigreceiver
+exec 42<$TS_HELPER_SIGRECEIVE
+$TS_CMD_FLOCK 42
+
+for SIG in $($TS_CMD_KILL -L); do
+	if [ "x${SIG//[0-9]/}" = "x" ]; then
+		EXPECTED=$SIG
+		continue
+	fi
+	if [ "x$SIG" = "xSTOP" ] || [ "x$SIG" = "xKILL" ]; then
+		continue
+	fi
+	if [ "x$SIG" = "xRTMIN" ]; then
+		SIG="$SIG+0"
+	fi
+	if [ "x$SIG" = "xRTMAX" ]; then
+		SIG="$SIG-0"
+	fi
+	$TS_HELPER_SIGRECEIVE &
+	TEST_PID=$(jobs -p)
+	# test_sigreceive needs time to start up
+	sleep 0.01
+	$TS_CMD_KILL -$SIG ${TS_HELPER_SIGRECEIVE##*/} >> $TS_OUTPUT 2>&1
+	wait $TEST_PID
+	if [ $? -ne $EXPECTED ]; then
+		echo "$SIG returned $? while $EXPECTED was expected" >> $TS_OUTPUT
+		all_ok=false
+	fi
+done
+
+$TS_CMD_FLOCK -u 42
+
+if $all_ok; then
+	echo 'all ok' >> $TS_OUTPUT
+fi
+
+ts_finalize
-- 
1.9.2

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

[Bernhard Voelker]

On 04/12/2014 12:28 PM, Sami Kerola wrote:
> +	$TS_HELPER_SIGRECEIVE &
> +	TEST_PID=$(jobs -p)
> +	# test_sigreceive needs time to start up
> +	sleep 0.01
> +	$TS_CMD_KILL -$SIG ${TS_HELPER_SIGRECEIVE##*/} >> $TS_OUTPUT 2>&1
> +	wait $TEST_PID
> +	if [ $? -ne $EXPECTED ]; then
> +		echo "$SIG returned $? while $EXPECTED was expected" >> $TS_OUTPUT
> +		all_ok=false
> +	fi

This is racy. Although this might work in most cases, one can not
determine that the SIGRECEIVE process had enough time to startup
and register all signal handlers before the signal arrives.
Usually this would fail more likely with high system load,
e.g. on build servers with massive parallel builds.

The only chance is that the SIGRECEIVE program indicates that it
has fully come up, e.g. by creating a file which did not exist
beforehand. The second part of avoiding unwanted behavior is to
wait for such a witness file to appear in the parent test script
... but avoiding to wait infinitely (because other unknown reasons
may have prevented the creation of the witness file).

In the test-suite of coreutils, there are a few complex (and yet
necessary) snippets to do this:
a) test case waiting for the inspected program to be ready:
  http://git.savannah.gnu.org/cgit/coreutils.git/tree/tests/tail-2/retry.sh#n45
b) utility function to retry with increasing delay until a
maximum number has reached:
  http://git.savannah.gnu.org/cgit/coreutils.git/tree/init.cfg#n597

Maybe a solution like the above is too heavy for this situation (unless
it could be reused in other tests), so something like the following may
suffice here - assuming that $TS_HELPER_SIGRECEIVE will successfully
perform a 'creat("witnessfile", mode)':

  rm -f witnessfile   || fail=1
  test -f witnessfile && fail=1
  $TS_HELPER_SIGRECEIVE & TEST_PID=$!
  up=0
  while i in 0.01 0.1 1 2 ; do
    test -f witnessfile && { up=1; break; }
    sleep $i
  done
  test $up = 1 || fail=1
  $TS_CMD_KILL -$SIG ${TS_HELPER_SIGRECEIVE##*/} >> $TS_OUTPUT 2>&1
  ...

WDYT?

Thank you & have a nice day,
Berny

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

[Sami Kerola]

On 13 April 2014 23:22, Bernhard Voelker <mail@bernhard-voelker.de> wrote:
> On 04/12/2014 12:28 PM, Sami Kerola wrote:
>> +     $TS_HELPER_SIGRECEIVE &
>> +     TEST_PID=$(jobs -p)
>> +     # test_sigreceive needs time to start up
>> +     sleep 0.01
>> +     $TS_CMD_KILL -$SIG ${TS_HELPER_SIGRECEIVE##*/} >> $TS_OUTPUT 2>&1
>> +     wait $TEST_PID
>> +     if [ $? -ne $EXPECTED ]; then
>> +             echo "$SIG returned $? while $EXPECTED was expected" >> $TS_OUTPUT
>> +             all_ok=false
>> +     fi
>
> This is racy. Although this might work in most cases, one can not
> determine that the SIGRECEIVE process had enough time to startup
> and register all signal handlers before the signal arrives.
> Usually this would fail more likely with high system load,
> e.g. on build servers with massive parallel builds.
>
> The only chance is that the SIGRECEIVE program indicates that it
> has fully come up, e.g. by creating a file which did not exist
> beforehand. The second part of avoiding unwanted behavior is to
> wait for such a witness file to appear in the parent test script
> ... but avoiding to wait infinitely (because other unknown reasons
> may have prevented the creation of the witness file).
>
> In the test-suite of coreutils, there are a few complex (and yet
> necessary) snippets to do this:
> a) test case waiting for the inspected program to be ready:
>   http://git.savannah.gnu.org/cgit/coreutils.git/tree/tests/tail-2/retry.sh#n45
> b) utility function to retry with increasing delay until a
> maximum number has reached:
>   http://git.savannah.gnu.org/cgit/coreutils.git/tree/init.cfg#n597
>
> Maybe a solution like the above is too heavy for this situation (unless
> it could be reused in other tests), so something like the following may
> suffice here - assuming that $TS_HELPER_SIGRECEIVE will successfully
> perform a 'creat("witnessfile", mode)':
>
>   rm -f witnessfile   || fail=1
>   test -f witnessfile && fail=1
>   $TS_HELPER_SIGRECEIVE & TEST_PID=$!
>   up=0
>   while i in 0.01 0.1 1 2 ; do
>     test -f witnessfile && { up=1; break; }
>     sleep $i
>   done
>   test $up = 1 || fail=1
>   $TS_CMD_KILL -$SIG ${TS_HELPER_SIGRECEIVE##*/} >> $TS_OUTPUT 2>&1
>   ...
>
> WDYT?

Hi Berny,

I wish I had though witnessfile before sending the patches, thanks for
the idea. I have feeling it might solve various parallel test issues I
had while doing test runs earlier. Version that used combined with
mktemp symlinks to sigreive helper to alter name of the killed program
was an attempt to make parallel runs to work, but it was flaky
probably due reasons you gave. The symlink approach with witnessfile
might allow to get rid of flock that makes the kill tests to run
sequentially. Let me try that.

Karel please for completion of the second try. I will keep these
patches untouched in

git://github.com/kerolasa/lelux-utiliteetit.git kill-tests

The witnessfile version goes this branch, once I have something to push.

git://github.com/kerolasa/lelux-utiliteetit.git kill-tests-v2

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

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

[Sami Kerola]

On 14 April 2014 08:42, Sami Kerola <kerolasa@iki.fi> wrote:
> On 13 April 2014 23:22, Bernhard Voelker <mail@bernhard-voelker.de> wrote:
>> This is racy. Although this might work in most cases, one can not
>> determine that the SIGRECEIVE process had enough time to startup
>> and register all signal handlers before the signal arrives.
>> Usually this would fail more likely with high system load,
>> e.g. on build servers with massive parallel builds.
>>
>> The only chance is that the SIGRECEIVE program indicates that it
>> has fully come up, e.g. by creating a file which did not exist
>> beforehand. The second part of avoiding unwanted behavior is to
>> wait for such a witness file to appear in the parent test script
>> ... but avoiding to wait infinitely (because other unknown reasons
>> may have prevented the creation of the witness file).
>>
>> In the test-suite of coreutils, there are a few complex (and yet
>> necessary) snippets to do this:
>> a) test case waiting for the inspected program to be ready:
>>   http://git.savannah.gnu.org/cgit/coreutils.git/tree/tests/tail-2/retry.sh#n45
>> b) utility function to retry with increasing delay until a
>> maximum number has reached:
>>   http://git.savannah.gnu.org/cgit/coreutils.git/tree/init.cfg#n597
>>
>> Maybe a solution like the above is too heavy for this situation (unless
>> it could be reused in other tests), so something like the following may
>> suffice here - assuming that $TS_HELPER_SIGRECEIVE will successfully
>> perform a 'creat("witnessfile", mode)':
>>
>>   rm -f witnessfile   || fail=1
>>   test -f witnessfile && fail=1
>>   $TS_HELPER_SIGRECEIVE & TEST_PID=$!
>>   up=0
>>   while i in 0.01 0.1 1 2 ; do
>>     test -f witnessfile && { up=1; break; }
>>     sleep $i
>>   done
>>   test $up = 1 || fail=1
>>   $TS_CMD_KILL -$SIG ${TS_HELPER_SIGRECEIVE##*/} >> $TS_OUTPUT 2>&1
>>   ...
>>
>> WDYT?
>
> Hi Berny,
>
> I wish I had though witnessfile before sending the patches, thanks for
> the idea. I have feeling it might solve various parallel test issues I
> had while doing test runs earlier. Version that used combined with
> mktemp symlinks to sigreive helper to alter name of the killed program
> was an attempt to make parallel runs to work, but it was flaky
> probably due reasons you gave. The symlink approach with witnessfile
> might allow to get rid of flock that makes the kill tests to run
> sequentially. Let me try that.
>
> Karel please for completion of the second try. I will keep these
> patches untouched in
>
> git://github.com/kerolasa/lelux-utiliteetit.git kill-tests
>
> The witnessfile version goes this branch, once I have something to push.
>
> git://github.com/kerolasa/lelux-utiliteetit.git kill-tests-v2

Hi Berny and others,

Here is new set of tests, and they almost work.

https://github.com/kerolasa/lelux-utiliteetit/tree/kill-tests-v2

The 'name_to_number' check[1] is giving random errors, and my wooden
eyes does not observe where is the problem. Or if the problem is in
kill itself, and random errors are a real bug. According to helper
witness file the signal receiver is running, but kill does not find
it. Some times the check passes without issues, but more often there
is one that kind of error, sometime two.

[1] https://github.com/kerolasa/lelux-utiliteetit/commit/516fa50baa0daf764a53063e44163e1c3328e110

Ideas, comments, help!

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

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

[Bernhard Voelker]

On 04/14/2014 05:13 PM, Sami Kerola wrote:
> Here is new set of tests, and they almost work.
>
> https://github.com/kerolasa/lelux-utiliteetit/tree/kill-tests-v2
>
> The 'name_to_number' check[1] is giving random errors, and my wooden
> eyes does not observe where is the problem. Or if the problem is in
> kill itself, and random errors are a real bug. According to helper
> witness file the signal receiver is running, but kill does not find
> it. Some times the check passes without issues, but more often there
> is one that kind of error, sometime two.
>
> [1] https://github.com/kerolasa/lelux-utiliteetit/commit/516fa50baa0daf764a53063e44163e1c3328e110
>
> Ideas, comments, help!

Hi Sami,

thanks.

Looking quickly at the tests, I think I don't understand the idea behind
all that symlinking stuff. I mean for most test cases it would suffice
to kill with the PID instead of the process name.

Re. the test failures of name_to_number: when I change the kill command
to work on $TEST_PID instead of the symlink name, then I don't get the
failures anymore.

BTW: is there a reason why you stick to using $(jobs -p) to get the
TEST_PID instead of the much simpler "$!"?

Thanks & have a nice day,
Berny

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

[Sami Kerola]

On 14 April 2014 17:14, Bernhard Voelker <mail@bernhard-voelker.de> wrote:

Hi Berny,

> Looking quickly at the tests, I think I don't understand the idea behind
> all that symlinking stuff. I mean for most test cases it would suffice
> to kill with the PID instead of the process name.
>
> Re. the test failures of name_to_number: when I change the kill command
> to work on $TEST_PID instead of the symlink name, then I don't get the
> failures anymore.

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

> BTW: is there a reason why you stick to using $(jobs -p) to get the
> TEST_PID instead of the much simpler "$!"?

Not any other reason than I did not know about "$!", which indeed is
better expression. Thank you for tip, I will push the better version
to v2 branch.

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

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

[Bernhard Voelker]

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

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

[Sami Kerola]

On 15 April 2014 10:09, Bernhard Voelker <mail@bernhard-voelker.de> wrote:
> 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.

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/

[PATCH 5/7] tests: check various ways to specify kill signal

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 tests/expected/kill/options |  1 +
 tests/ts/kill/options       | 63 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)
 create mode 100644 tests/expected/kill/options
 create mode 100755 tests/ts/kill/options

diff --git a/tests/expected/kill/options b/tests/expected/kill/options
new file mode 100644
index 0000000..d48ce72
--- /dev/null
+++ b/tests/expected/kill/options
@@ -0,0 +1 @@
+all ok
diff --git a/tests/ts/kill/options b/tests/ts/kill/options
new file mode 100755
index 0000000..6cf2251
--- /dev/null
+++ b/tests/ts/kill/options
@@ -0,0 +1,63 @@
+#!/bin/bash
+
+# This file is part of util-linux.
+#
+# This file is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This file is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+TS_TOPDIR="${0%/*}/../.."
+TS_DESC="options"
+
+. $TS_TOPDIR/functions.sh
+ts_init "$*"
+
+ts_check_test_command "$TS_CMD_KILL"
+ts_check_test_command "$TS_CMD_FLOCK"
+all_ok=true
+
+try_option()
+{
+	$TS_HELPER_SIGRECEIVE &
+	TEST_PID=$(jobs -p)
+	# test_sigreceive needs time to start up
+	sleep 0.01
+	$TS_CMD_KILL $@ ${TS_HELPER_SIGRECEIVE##*/} >> $TS_OUTPUT 2>&1
+	if [ $? -ne 0 ]; then
+		echo "kill $@ did not work" >> $TS_OUTPUT
+		all_ok=false
+	fi
+	wait $TEST_PID
+	if [ $? -ne 1 ]; then
+		echo "wait $TEST_PID for $@ did not work" >> $TS_OUTPUT
+		all_ok=false
+	fi
+}
+
+# parallel kill checks could kill wrong sigreceiver
+exec 42<$TS_HELPER_SIGRECEIVE
+$TS_CMD_FLOCK 42
+
+try_option -s 1
+try_option --signal 1
+try_option --signal HUP
+try_option --signal SIGHUP
+try_option -q HUP
+try_option --queue HUP
+try_option -1
+try_option -HUP
+try_option -SIGHUP
+
+$TS_CMD_FLOCK -u 42
+
+if $all_ok; then
+	echo 'all ok' >> $TS_OUTPUT
+fi
+
+ts_finalize
-- 
1.9.2

[PATCH 6/7] tests: check kill print pid option

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 tests/expected/kill/print_pid |  1 +
 tests/ts/kill/print_pid       | 56 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)
 create mode 100644 tests/expected/kill/print_pid
 create mode 100755 tests/ts/kill/print_pid

diff --git a/tests/expected/kill/print_pid b/tests/expected/kill/print_pid
new file mode 100644
index 0000000..d48ce72
--- /dev/null
+++ b/tests/expected/kill/print_pid
@@ -0,0 +1 @@
+all ok
diff --git a/tests/ts/kill/print_pid b/tests/ts/kill/print_pid
new file mode 100755
index 0000000..c2a1ec4
--- /dev/null
+++ b/tests/ts/kill/print_pid
@@ -0,0 +1,56 @@
+#!/bin/bash
+
+# This file is part of util-linux.
+#
+# This file is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This file is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+TS_TOPDIR="${0%/*}/../.."
+TS_DESC="print_pid"
+
+. $TS_TOPDIR/functions.sh
+ts_init "$*"
+
+ts_check_test_command "$TS_CMD_KILL"
+ts_check_test_command "$TS_CMD_FLOCK"
+all_ok=true
+
+# parallel kill checks could kill wrong sigreceiver
+exec 42<$TS_HELPER_SIGRECEIVE
+$TS_CMD_FLOCK 42
+
+$TS_HELPER_SIGRECEIVE &
+TEST_PID=$(jobs -p)
+# test_sigreceive needs time to start up
+sleep 0.01
+KILL_PID=$($TS_CMD_KILL -p ${TS_HELPER_SIGRECEIVE##*/} 2>> $TS_OUTPUT)
+if [ $? -ne 0 ]; then
+	echo "kill -p did not work" >> $TS_OUTPUT
+	all_ok=false
+fi
+if [ "x$TEST_PID" != "x$KILL_PID" ]; then
+	echo "jobs -p $TEST_PID != kill -p $KILL_PID" >> $TS_OUTPUT
+	all_ok=false
+else
+	$TS_CMD_KILL -1 $TEST_PID
+	wait $TEST_PID
+	if [ $? -ne 1 ]; then
+		echo "wait $TEST_PID for $@ did not work" >> $TS_OUTPUT
+		all_ok=false
+	fi
+fi
+
+$TS_CMD_FLOCK -u 42
+
+if $all_ok; then
+	echo 'all ok' >> $TS_OUTPUT
+fi
+
+ts_finalize
-- 
1.9.2

[PATCH 7/7] tests: check kill all user processes

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 tests/commands.sh                 |  1 +
 tests/expected/kill/all_processes |  5 ++++
 tests/ts/kill/all_processes       | 57 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+)
 create mode 100644 tests/expected/kill/all_processes
 create mode 100755 tests/ts/kill/all_processes

diff --git a/tests/commands.sh b/tests/commands.sh
index c67aa91..0da19f2 100644
--- a/tests/commands.sh
+++ b/tests/commands.sh
@@ -67,6 +67,7 @@ TS_CMD_REV=${TS_CMD_REV:-"$top_builddir/rev"}
 TS_CMD_SCRIPT=${TS_CMD_SCRIPT-"$top_builddir/script"}
 TS_CMD_SETARCH=${TS_CMD_SETARCH-"$top_builddir/setarch"}
 TS_CMD_SETSID=${TS_CMD_SETSID-"$top_builddir/setsid"}
+TS_CMD_SU=${TS_CMD_SU:-"$top_builddir/su"}
 TS_CMD_SWAPLABEL=${TS_CMD_SWAPLABEL:-"$top_builddir/swaplabel"}
 TS_CMD_SWAPOFF=${TS_CMD_SWAPOFF:-"$top_builddir/swapoff"}
 TS_CMD_SWAPON=${TS_CMD_SWAPON:-"$top_builddir/swapon"}
diff --git a/tests/expected/kill/all_processes b/tests/expected/kill/all_processes
new file mode 100644
index 0000000..c428372
--- /dev/null
+++ b/tests/expected/kill/all_processes
@@ -0,0 +1,5 @@
+test 1
+kill: cannot find process "test_sigreceive".
+test 2
+test 3
+kill: cannot find process "test_sigreceive".
diff --git a/tests/ts/kill/all_processes b/tests/ts/kill/all_processes
new file mode 100755
index 0000000..bc304ff
--- /dev/null
+++ b/tests/ts/kill/all_processes
@@ -0,0 +1,57 @@
+#!/bin/bash
+
+# This file is part of util-linux.
+#
+# This file is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This file is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+TS_TOPDIR="${0%/*}/../.."
+TS_DESC="all_processes"
+
+. $TS_TOPDIR/functions.sh
+ts_init "$*"
+
+ts_skip_nonroot
+
+ts_check_test_command "$TS_CMD_KILL"
+ts_check_test_command "$TS_CMD_FLOCK"
+ts_check_test_command "$TS_CMD_SU"
+
+if ! getent passwd nobody >/dev/null; then
+	ts_skip "no nobody user"
+fi
+
+# parallel kill checks could kill wrong sigreceiver
+exec 42<$TS_HELPER_SIGRECEIVE
+$TS_CMD_FLOCK 42
+
+su nobody -s /bin/sh -c $TS_HELPER_SIGRECEIVE &
+# test_sigreceive needs time to start up
+sleep 0.01
+
+echo "test 1" >> $TS_OUTPUT
+$TS_CMD_KILL ${TS_HELPER_SIGRECEIVE##*/} >> $TS_OUTPUT 2>&1
+if [ $? -ne 1 ]; then
+	echo "kill did not return 1" >> $TS_OUTPUT
+fi
+echo "test 2" >> $TS_OUTPUT
+$TS_CMD_KILL -a ${TS_HELPER_SIGRECEIVE##*/} >> $TS_OUTPUT 2>&1
+if [ $? -ne 0 ]; then
+	echo "kill did not return 0" >> $TS_OUTPUT
+fi
+echo "test 3" >> $TS_OUTPUT
+$TS_CMD_KILL -a -p ${TS_HELPER_SIGRECEIVE##*/} >> $TS_OUTPUT 2>&1
+if [ $? -ne 1 ]; then
+	echo "kill -a -p did not return 1" >> $TS_OUTPUT
+fi
+
+$TS_CMD_FLOCK -u 42
+
+ts_finalize
-- 
1.9.2