util-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/9] pull: kill tests
@ 2014-04-15 11:15 Sami Kerola
  2014-04-15 11:15 ` [PATCH 1/9] kill: make options --pid and --queue mutually exclusive Sami Kerola
                   ` (9 more replies)
  0 siblings, 10 replies; 18+ messages in thread
From: Sami Kerola @ 2014-04-15 11:15 UTC (permalink / raw)
  To: util-linux; +Cc: kerolasa

Here are the kill tests again.  Kudos to Bernhard whiping me to right
direction after first submission.  Changes since previous submission.

* in test scripts wait for test_sigreceiver is more pessimistic
* the test_sigreceiver tells to mother proces when it's ready be killed
  with a witness file
* in test scripts $(jobs -p) was changed to $!
* kill --verbose option is added
* lib/procutils got a fix


The following changes since commit b233dcb6ffa9a880882d58f85162ff97cede050f:

  libsmartcols: use buffer struct in table_print.c (2014-04-14 16:41:20 +0200)

are available in the git repository at:

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

for you to fetch changes up to 81143d7ad2bd5ac8fe86e0738f3e7a21e2c6f06c:

  lib/procutils: reset errno before strtol() call (2014-04-15 11:54:21 +0100)

----------------------------------------------------------------
Sami Kerola (9):
      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
      kill: add --verbose option to display what is killed
      lib/procutils: reset errno before strtol() call

 lib/procutils.c                    |   3 +-
 misc-utils/kill.c                  |  21 +++--
 tests/commands.sh                  |   3 +
 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    | 153 +++++++++++++++++++++++++++++++++++++
 tests/ts/kill/all_processes        |  61 +++++++++++++++
 tests/ts/kill/name_to_number       |  64 ++++++++++++++++
 tests/ts/kill/options              |  66 ++++++++++++++++
 tests/ts/kill/print_pid            |  58 ++++++++++++++
 13 files changed, 434 insertions(+), 6 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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 1/9] kill: make options --pid and --queue mutually exclusive
  2014-04-15 11:15 [PATCH 0/9] pull: kill tests Sami Kerola
@ 2014-04-15 11:15 ` Sami Kerola
  2014-04-15 11:15 ` [PATCH 2/9] kill: remove unnecessary indirection Sami Kerola
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Sami Kerola @ 2014-04-15 11:15 UTC (permalink / raw)
  To: util-linux; +Cc: kerolasa

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


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 2/9] kill: remove unnecessary indirection
  2014-04-15 11:15 [PATCH 0/9] pull: kill tests Sami Kerola
  2014-04-15 11:15 ` [PATCH 1/9] kill: make options --pid and --queue mutually exclusive Sami Kerola
@ 2014-04-15 11:15 ` Sami Kerola
  2014-04-15 11:15 ` [PATCH 3/9] tests: add signal receiver program Sami Kerola
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Sami Kerola @ 2014-04-15 11:15 UTC (permalink / raw)
  To: util-linux; +Cc: kerolasa

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


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 3/9] tests: add signal receiver program
  2014-04-15 11:15 [PATCH 0/9] pull: kill tests Sami Kerola
  2014-04-15 11:15 ` [PATCH 1/9] kill: make options --pid and --queue mutually exclusive Sami Kerola
  2014-04-15 11:15 ` [PATCH 2/9] kill: remove unnecessary indirection Sami Kerola
@ 2014-04-15 11:15 ` Sami Kerola
  2014-04-15 12:20   ` Sami Kerola
  2014-04-15 11:15 ` [PATCH 4/9] tests: check kill is converting signals names correctly Sami Kerola
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 18+ messages in thread
From: Sami Kerola @ 2014-04-15 11:15 UTC (permalink / raw)
  To: util-linux; +Cc: kerolasa, Bernhard Voelker

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

CC: Bernhard Voelker <mail@bernhard-voelker.de>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 tests/helpers/Makemodule.am     |   3 +
 tests/helpers/test_sigreceive.c | 153 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 156 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..a84f95a
--- /dev/null
+++ b/tests/helpers/test_sigreceive.c
@@ -0,0 +1,153 @@
+/*
+ * 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 <err.h>
+#include <errno.h>
+#include <signal.h>
+#include <stdio.h>
+#include <sys/select.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <unistd.h>
+#include <xalloc.h>
+
+char *pid_path;
+int fd;
+
+static __attribute__ ((__noreturn__))
+void exiter(int sig)
+{
+	close(fd);
+	unlink(pid_path);
+	_exit(sig);
+}
+
+int main(int argc, char **argv)
+{
+	struct sigaction sigact;
+	fd_set rfds;
+	struct timeval timeout;
+	struct stat statbuf;
+	mode_t mode = S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH;
+
+	if (argc != 2) {
+		fputs("Usage: test_sigreceive <file|directory>\n", stderr);
+		return -1;
+	}
+
+	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]);
+
+	sigemptyset(&sigact.sa_mask);
+	sigact.sa_flags = 0;
+	sigact.sa_handler = exiter;
+	timeout.tv_sec = 5;
+	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);
+
+	if (!(fd = open(pid_path, O_WRONLY | O_CREAT | O_TRUNC, mode)))
+		err(EXIT_FAILURE, "cannot open pid file: %s", pid_path);
+	select(STDIN_FILENO, &rfds, NULL, NULL, &timeout);
+	exiter(0);
+}
-- 
1.9.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 4/9] tests: check kill is converting signals names correctly
  2014-04-15 11:15 [PATCH 0/9] pull: kill tests Sami Kerola
                   ` (2 preceding siblings ...)
  2014-04-15 11:15 ` [PATCH 3/9] tests: add signal receiver program Sami Kerola
@ 2014-04-15 11:15 ` Sami Kerola
  2014-04-15 11:15 ` [PATCH 5/9] tests: check various ways to specify kill signal Sami Kerola
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Sami Kerola @ 2014-04-15 11:15 UTC (permalink / raw)
  To: util-linux; +Cc: kerolasa, Bernhard Voelker

CC: Bernhard Voelker <mail@bernhard-voelker.de>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 tests/commands.sh                  |  2 ++
 tests/expected/kill/name_to_number |  1 +
 tests/ts/kill/name_to_number       | 64 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 67 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..33bb1ab 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"
 
@@ -47,6 +48,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..6e10b8e
--- /dev/null
+++ b/tests/ts/kill/name_to_number
@@ -0,0 +1,64 @@
+#!/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"
+all_ok=true
+
+TS_CWD="${0%/*}"
+
+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
+	HELPER_SYMLINK="$TS_CWD/$(mktemp -u XXXXXXXXXXXXXXX)"
+	ln -s "$TS_HELPER_SIGRECEIVE" "$HELPER_SYMLINK"
+	"$HELPER_SYMLINK" "$TS_CWD" >> "$TS_OUTPUT" 2>&1 &
+	TEST_PID=$!
+	# test_sigreceive needs time to start up
+	up=0
+	for i in 0.01 0.1 1 1 1 1; do
+		test -f "$TS_CWD/$TEST_PID" && { up=1; break; }
+		sleep $i
+	done
+	test $up = 0 && echo "$SIG sigreceive ${HELPER_SYMLINK##*/} helper did not start" >> "$TS_OUTPUT"
+	"$TS_CMD_KILL" -$SIG ${HELPER_SYMLINK##*/} >> "$TS_OUTPUT" 2>&1
+	wait $TEST_PID
+	if [ $? -ne $EXPECTED ]; then
+		echo "$SIG returned $? while $EXPECTED was expected" >> "$TS_OUTPUT"
+		all_ok=false
+	fi
+	rm -f "$HELPER_SYMLINK"
+done
+
+if $all_ok; then
+	echo 'all ok' >> $TS_OUTPUT
+fi
+
+ts_finalize
-- 
1.9.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 5/9] tests: check various ways to specify kill signal
  2014-04-15 11:15 [PATCH 0/9] pull: kill tests Sami Kerola
                   ` (3 preceding siblings ...)
  2014-04-15 11:15 ` [PATCH 4/9] tests: check kill is converting signals names correctly Sami Kerola
@ 2014-04-15 11:15 ` Sami Kerola
  2014-04-15 11:15 ` [PATCH 6/9] tests: check kill print pid option Sami Kerola
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Sami Kerola @ 2014-04-15 11:15 UTC (permalink / raw)
  To: util-linux; +Cc: kerolasa, Bernhard Voelker

CC: Bernhard Voelker <mail@bernhard-voelker.de>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 tests/expected/kill/options |  1 +
 tests/ts/kill/options       | 66 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 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..70ae7da
--- /dev/null
+++ b/tests/ts/kill/options
@@ -0,0 +1,66 @@
+#!/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"
+all_ok=true
+
+TS_CWD="${0%/*}"
+HELPER_SYMLINK="$TS_CWD/$(mktemp -u XXXXXXXXXXXXXXX)"
+ln -s "$TS_HELPER_SIGRECEIVE" "$HELPER_SYMLINK"
+trap "rm -f $HELPER_SYMLINK" 0
+
+try_option()
+{
+	"$HELPER_SYMLINK" "$TS_CWD" >> "$TS_OUTPUT" 2>&1 &
+	TEST_PID=$!
+	# test_sigreceive needs time to start up
+	up=0
+	for i in 0.01 0.1 1 1 1 1; do
+		test -f "$TS_CWD/$TEST_PID" && { up=1; break; }
+		sleep $i
+	done
+	test $up = 1 || echo "$@ sigreceive helper did not start" >> "$TS_OUTPUT"
+	"$TS_CMD_KILL" "$@" $TEST_PID >> "$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
+}
+
+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
+
+if $all_ok; then
+	echo 'all ok' >> "$TS_OUTPUT"
+fi
+
+ts_finalize
-- 
1.9.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 6/9] tests: check kill print pid option
  2014-04-15 11:15 [PATCH 0/9] pull: kill tests Sami Kerola
                   ` (4 preceding siblings ...)
  2014-04-15 11:15 ` [PATCH 5/9] tests: check various ways to specify kill signal Sami Kerola
@ 2014-04-15 11:15 ` Sami Kerola
  2014-04-15 11:15 ` [PATCH 7/9] tests: check kill all user processes Sami Kerola
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Sami Kerola @ 2014-04-15 11:15 UTC (permalink / raw)
  To: util-linux; +Cc: kerolasa, Bernhard Voelker

CC: Bernhard Voelker <mail@bernhard-voelker.de>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 tests/expected/kill/print_pid |  1 +
 tests/ts/kill/print_pid       | 58 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 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..ddb4973
--- /dev/null
+++ b/tests/ts/kill/print_pid
@@ -0,0 +1,58 @@
+#!/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"
+all_ok=true
+
+TS_CWD="${0%/*}"
+HELPER_SYMLINK="$TS_CWD/$(mktemp -u XXXXXXXXXXXXXXX)"
+ln -s "$TS_HELPER_SIGRECEIVE" "$HELPER_SYMLINK"
+trap "rm -f $HELPER_SYMLINK" 0
+
+"$HELPER_SYMLINK" "$TS_CWD" >> "$TS_OUTPUT" 2>&1 &
+TEST_PID=$!
+# test_sigreceive needs time to start up
+up=0
+for i in 0.01 0.1 1 1 1 1; do
+	test -f "$TS_CWD/$TEST_PID" && { up=1; break; }
+	sleep $i
+done
+KILL_PID=$("$TS_CMD_KILL" -p ${HELPER_SYMLINK##*/} 2>> "$TS_OUTPUT" 2>&1)
+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 returned ${?}" >> "$TS_OUTPUT"
+		all_ok=false
+	fi
+fi
+
+if $all_ok; then
+	echo 'all ok' >> "$TS_OUTPUT"
+fi
+
+ts_finalize
-- 
1.9.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 7/9] tests: check kill all user processes
  2014-04-15 11:15 [PATCH 0/9] pull: kill tests Sami Kerola
                   ` (5 preceding siblings ...)
  2014-04-15 11:15 ` [PATCH 6/9] tests: check kill print pid option Sami Kerola
@ 2014-04-15 11:15 ` Sami Kerola
  2014-04-16 10:13   ` Karel Zak
  2014-04-15 11:15 ` [PATCH 8/9] kill: add --verbose option to display what is killed Sami Kerola
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 18+ messages in thread
From: Sami Kerola @ 2014-04-15 11:15 UTC (permalink / raw)
  To: util-linux; +Cc: kerolasa, Bernhard Voelker

CC: Bernhard Voelker <mail@bernhard-voelker.de>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 tests/commands.sh                 |  1 +
 tests/expected/kill/all_processes |  5 ++++
 tests/ts/kill/all_processes       | 61 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 67 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 33bb1ab..8ba0325 100644
--- a/tests/commands.sh
+++ b/tests/commands.sh
@@ -66,6 +66,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..c85f856
--- /dev/null
+++ b/tests/ts/kill/all_processes
@@ -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="all_processes"
+
+. "$TS_TOPDIR/functions.sh"
+ts_init "$*"
+
+ts_skip_nonroot
+
+ts_check_test_command "$TS_CMD_KILL"
+ts_check_test_command "$TS_CMD_SU"
+
+if ! getent passwd nobody >/dev/null; then
+	ts_skip "no nobody user"
+fi
+
+TEST_PID=''
+TS_CWD="${0%/*}"
+HELPER_SYMLINK="$TS_CWD/$(mktemp -u XXXXXXXXXXXXXXX)"
+ln -s "$TS_HELPER_SIGRECEIVE" "$HELPER_SYMLINK"
+trap "rm -f $HELPER_SYMLINK" 0
+
+su nobody -s /bin/sh -c "$HELPER_SYMLINK $TS_CWD/nobody" >> "$TS_OUTPUT" 2>&1 &
+# test_sigreceive needs time to start up
+for i in 0.01 0.1 1 1 1 1; do
+	test -f "$TS_CWD/nobody" && { up=1; break; }
+	sleep $i
+done
+
+echo "test 1" >> "$TS_OUTPUT"
+"$TS_CMD_KILL" ${HELPER_SYMLINK##*/} >> "$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 ${HELPER_SYMLINK##*/} >> "$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 ${HELPER_SYMLINK##*/} >> "$TS_OUTPUT" 2>&1
+if [ $? -ne 1 ]; then
+	echo "kill -a -p did not return 1" >> "$TS_OUTPUT"
+fi
+
+sed -i "s/${HELPER_SYMLINK##*/}/${TS_HELPER_SIGRECEIVE##*/}/" "$TS_OUTPUT"
+
+ts_finalize
-- 
1.9.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 8/9] kill: add --verbose option to display what is killed
  2014-04-15 11:15 [PATCH 0/9] pull: kill tests Sami Kerola
                   ` (6 preceding siblings ...)
  2014-04-15 11:15 ` [PATCH 7/9] tests: check kill all user processes Sami Kerola
@ 2014-04-15 11:15 ` Sami Kerola
  2014-04-15 11:15 ` [PATCH 9/9] lib/procutils: reset errno before strtol() call Sami Kerola
  2014-04-28 10:03 ` [PATCH 0/9] pull: kill tests Karel Zak
  9 siblings, 0 replies; 18+ messages in thread
From: Sami Kerola @ 2014-04-15 11:15 UTC (permalink / raw)
  To: util-linux; +Cc: kerolasa

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

diff --git a/misc-utils/kill.c b/misc-utils/kill.c
index 9566f14..b30b94f 100644
--- a/misc-utils/kill.c
+++ b/misc-utils/kill.c
@@ -78,7 +78,8 @@ struct kill_control {
 		check_all:1,
 		do_kill:1,
 		do_pid:1,
-		use_sigval:1;
+		use_sigval:1,
+		verbose:1;
 };
 
 struct signv {
@@ -314,6 +315,7 @@ static void __attribute__((__noreturn__)) usage(FILE *out)
 	fputs(_(" -p, --pid              print pids without signaling them\n"), out);
 	fputs(_(" -l, --list [=<signal>] list signal names, or convert one to a name\n"), out);
 	fputs(_(" -L, --table            list signal names and numbers\n"), out);
+	fputs(_("     --verbose          print pids that will be signaled\n"), out);
 
 	fputs(USAGE_SEPARATOR, out);
 	fputs(USAGE_HELP, out);
@@ -345,7 +347,10 @@ static char **parse_arguments(int argc, char **argv, struct kill_control *ctl)
 		}
 		if (!strcmp(arg, "-h") || !strcmp(arg, "--help"))
 			usage(stdout);
-
+		if (!strcmp(arg, "--verbose")) {
+			ctl->verbose = 1;
+			continue;
+		}
 		if (!strcmp(arg, "-a") || !strcmp(arg, "--all")) {
 			ctl->check_all = 1;
 			continue;
@@ -442,6 +447,8 @@ static int kill_verbose(const struct kill_control *ctl)
 {
 	int rc = 0;
 
+	if (ctl->verbose)
+		printf(_("sending signal %d to pid %d\n"), ctl->numsig, ctl->pid);
 	if (ctl->do_pid) {
 		printf("%ld\n", (long) ctl->pid);
 		return 0;
-- 
1.9.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 9/9] lib/procutils: reset errno before strtol() call
  2014-04-15 11:15 [PATCH 0/9] pull: kill tests Sami Kerola
                   ` (7 preceding siblings ...)
  2014-04-15 11:15 ` [PATCH 8/9] kill: add --verbose option to display what is killed Sami Kerola
@ 2014-04-15 11:15 ` Sami Kerola
  2014-04-28 10:03 ` [PATCH 0/9] pull: kill tests Karel Zak
  9 siblings, 0 replies; 18+ messages in thread
From: Sami Kerola @ 2014-04-15 11:15 UTC (permalink / raw)
  To: util-linux; +Cc: kerolasa

When going through /proc the last entry made readdir() to alter errno,
which made the strtol() to think something went wrong, resulting kill(1)
tests to fail when running in --parallel mode.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 lib/procutils.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/procutils.c b/lib/procutils.c
index d633261..31b77ff 100644
--- a/lib/procutils.c
+++ b/lib/procutils.c
@@ -86,7 +86,7 @@ int proc_next_tid(struct proc_tasks *tasks, pid_t *tid)
 
 		if (!isdigit((unsigned char) *d->d_name))
 			continue;
-
+		errno = 0;
 		*tid = (pid_t) strtol(d->d_name, &end, 10);
 		if (errno || d->d_name == end || (end && *end))
 			return -1;
@@ -183,6 +183,7 @@ int proc_next_pid(struct proc_processes *ps, pid_t *pid)
 		}
 
 		p = NULL;
+		errno = 0;
 		*pid = (pid_t) strtol(d->d_name, &p, 10);
 		if (errno || d->d_name == p || (p && *p))
 			return errno ? -errno : -1;
-- 
1.9.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/9] tests: add signal receiver program
  2014-04-15 11:15 ` [PATCH 3/9] tests: add signal receiver program Sami Kerola
@ 2014-04-15 12:20   ` Sami Kerola
  0 siblings, 0 replies; 18+ messages in thread
From: Sami Kerola @ 2014-04-15 12:20 UTC (permalink / raw)
  To: util-linux

On 15 April 2014 12:15, Sami Kerola <kerolasa@iki.fi> wrote:
> +       if (!(fd = open(pid_path, O_WRONLY | O_CREAT | O_TRUNC, mode)))

Karel,

Bernhard's comment about open() returning -1 when failing is fixed in my github.

https://github.com/kerolasa/lelux-utiliteetit/commit/125d5bebeab0727cd53f51d04cbe24ed9b18de5b#diff-437d9e619dae92130efbc0f412050818R149

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 7/9] tests: check kill all user processes
  2014-04-15 11:15 ` [PATCH 7/9] tests: check kill all user processes Sami Kerola
@ 2014-04-16 10:13   ` Karel Zak
  2014-04-20  9:53     ` Sami Kerola
  0 siblings, 1 reply; 18+ messages in thread
From: Karel Zak @ 2014-04-16 10:13 UTC (permalink / raw)
  To: Sami Kerola; +Cc: util-linux, Bernhard Voelker

On Tue, Apr 15, 2014 at 12:15:33PM +0100, Sami Kerola wrote:
> +ts_check_test_command "$TS_CMD_KILL"
> +ts_check_test_command "$TS_CMD_SU"

 ts_check_test_command "TS_HELPER_SIGRECEIVE"

> +if ! getent passwd nobody >/dev/null; then
> +	ts_skip "no nobody user"
> +fi
> +
> +TEST_PID=''
> +TS_CWD="${0%/*}"

Please, use $TS_OUTDIR. The tests should not write to another places.
The output/ is the directory that clean up our build system.

> +HELPER_SYMLINK="$TS_CWD/$(mktemp -u XXXXXXXXXXXXXXX)"
> +ln -s "$TS_HELPER_SIGRECEIVE" "$HELPER_SYMLINK"
> +trap "rm -f $HELPER_SYMLINK" 0
> +
> +su nobody -s /bin/sh -c "$HELPER_SYMLINK $TS_CWD/nobody" >> "$TS_OUTPUT" 2>&1 &

I don't understand this idea, on my system Mr.Nobody can not write 
to my directories.

It would be also better to add --setgit and --setuid to the helper to
avoid extra su(1) process, then you can use TEST_PID=$!

> +# test_sigreceive needs time to start up
> +for i in 0.01 0.1 1 1 1 1; do
> +	test -f "$TS_CWD/nobody" && { up=1; break; }
> +	sleep $i
> +done

The question is if we really need to use the PID files. Would be
possible to use test_sigreceive that does not write anything?

You can for example to use 

   awk '/SigCgt/ { print $2}' /proc/<pid>/status 
   
to check if the signal handlers are already initialized (the final
mask is 800000027ffbfeff on my system).

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 7/9] tests: check kill all user processes
  2014-04-16 10:13   ` Karel Zak
@ 2014-04-20  9:53     ` Sami Kerola
  2014-04-20 15:36       ` Bernhard Voelker
  2014-04-22 10:12       ` Karel Zak
  0 siblings, 2 replies; 18+ messages in thread
From: Sami Kerola @ 2014-04-20  9:53 UTC (permalink / raw)
  To: Karel Zak; +Cc: util-linux, Bernhard Voelker

On 16 April 2014 11:13, Karel Zak <kzak@redhat.com> wrote:
> On Tue, Apr 15, 2014 at 12:15:33PM +0100, Sami Kerola wrote:
>> +ts_check_test_command "$TS_CMD_KILL"
>> +ts_check_test_command "$TS_CMD_SU"
>
>  ts_check_test_command "TS_HELPER_SIGRECEIVE"

These are added.

>> +if ! getent passwd nobody >/dev/null; then
>> +     ts_skip "no nobody user"
>> +fi
>> +
>> +TEST_PID=''
>> +TS_CWD="${0%/*}"
>
> Please, use $TS_OUTDIR. The tests should not write to another places.
> The output/ is the directory that clean up our build system.

These are removed.

>> +HELPER_SYMLINK="$TS_CWD/$(mktemp -u XXXXXXXXXXXXXXX)"
>> +ln -s "$TS_HELPER_SIGRECEIVE" "$HELPER_SYMLINK"
>> +trap "rm -f $HELPER_SYMLINK" 0
>> +
>> +su nobody -s /bin/sh -c "$HELPER_SYMLINK $TS_CWD/nobody" >> "$TS_OUTPUT" 2>&1 &
>
> I don't understand this idea, on my system Mr.Nobody can not write
> to my directories.
>
> It would be also better to add --setgit and --setuid to the helper to
> avoid extra su(1) process, then you can use TEST_PID=$!

The --setuid is added, which made me to scratch my head for a moment
why it behaved the way it did, resulting to a patch fixing
lib/procutils

https://github.com/kerolasa/lelux-utiliteetit/commit/08b7b1c60eaeddbf25ef28f2a7d1d0e6778a6af8

>> +# test_sigreceive needs time to start up
>> +for i in 0.01 0.1 1 1 1 1; do
>> +     test -f "$TS_CWD/nobody" && { up=1; break; }
>> +     sleep $i
>> +done
>
> The question is if we really need to use the PID files. Would be
> possible to use test_sigreceive that does not write anything?
>
> You can for example to use
>
>    awk '/SigCgt/ { print $2}' /proc/<pid>/status
>
> to check if the signal handlers are already initialized (the final
> mask is 800000027ffbfeff on my system).

That is a much better way to determine if the process is ready to be
killed. Implemented in my git, and I have only one remaining question.
Would you prefer me to send the patch set again or is review from
remote branch OK? The biggest change is use of SigCgt, that is done
the same way in all scripts and can be seen for instance looking this
change.

https://github.com/kerolasa/lelux-utiliteetit/commit/08b7b1c60eaeddbf25ef28f2a7d1d0e6778a6af8

--- pull --
The following changes since commit 9348ef251102eefdf9e352616393778f0950720f:

  libfdisk: (gpt) implement 'fix order' commnad (2014-04-18 14:00:39 +0200)

are available in the git repository at:

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

for you to fetch changes up to 08b7b1c60eaeddbf25ef28f2a7d1d0e6778a6af8:

  lib/procutils: notice setuid() process ownership changes (2014-04-20
10:36:05 +0100)

----------------------------------------------------------------
Sami Kerola (10):
      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
      kill: add --verbose option to display what is killed
      lib/procutils: reset errno before strtol() call
      lib/procutils: notice setuid() process ownership changes

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 7/9] tests: check kill all user processes
  2014-04-20  9:53     ` Sami Kerola
@ 2014-04-20 15:36       ` Bernhard Voelker
  2014-04-22 10:09         ` Karel Zak
  2014-04-22 10:12       ` Karel Zak
  1 sibling, 1 reply; 18+ messages in thread
From: Bernhard Voelker @ 2014-04-20 15:36 UTC (permalink / raw)
  To: kerolasa, Karel Zak; +Cc: util-linux

On 04/20/2014 11:53 AM, Sami Kerola wrote:
> On 16 April 2014 11:13, Karel Zak <kzak@redhat.com> wrote:
>> On Tue, Apr 15, 2014 at 12:15:33PM +0100, Sami Kerola wrote:
>>> +HELPER_SYMLINK="$TS_CWD/$(mktemp -u XXXXXXXXXXXXXXX)"
>>> +ln -s "$TS_HELPER_SIGRECEIVE" "$HELPER_SYMLINK"
>>> +trap "rm -f $HELPER_SYMLINK" 0
>>> +
>>> +su nobody -s /bin/sh -c "$HELPER_SYMLINK $TS_CWD/nobody" >> "$TS_OUTPUT" 2>&1 &
>>
>> I don't understand this idea, on my system Mr.Nobody can not write
>> to my directories.
>>
>> It would be also better to add --setgit and --setuid to the helper to
>> avoid extra su(1) process, then you can use TEST_PID=$!
> 
> The --setuid is added, [...]

yes, setuid is a great idea, yet I'm not sure
if using UID 1 is a good idea:

		if (setuid(1) < 0)
			err(TEST_SIGRECEIVE_FAILURE, "setuid failed");

Doesn't everyone hack&build under his own private user account?
So maybe that would be the best choice (if the kill-test doesn't
kill random processes of the user.
Anyone a better idea?

>>> +# test_sigreceive needs time to start up
>>> +for i in 0.01 0.1 1 1 1 1; do
>>> +     test -f "$TS_CWD/nobody" && { up=1; break; }
>>> +     sleep $i
>>> +done
>>
>> The question is if we really need to use the PID files. Would be
>> possible to use test_sigreceive that does not write anything?
>>
>> You can for example to use
>>
>>    awk '/SigCgt/ { print $2}' /proc/<pid>/status
>>
>> to check if the signal handlers are already initialized (the final
>> mask is 800000027ffbfeff on my system).


> That is a much better way to determine if the process is ready to be
> killed. Implemented in my git, and I have only one remaining question.

I'm not a big fan of magic numbers. Is there a chance to narrow down
the relevant bits?

Have a nice day,
Berny

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 7/9] tests: check kill all user processes
  2014-04-20 15:36       ` Bernhard Voelker
@ 2014-04-22 10:09         ` Karel Zak
  2014-04-22 20:55           ` Sami Kerola
  0 siblings, 1 reply; 18+ messages in thread
From: Karel Zak @ 2014-04-22 10:09 UTC (permalink / raw)
  To: Bernhard Voelker; +Cc: kerolasa, util-linux

On Sun, Apr 20, 2014 at 05:36:29PM +0200, Bernhard Voelker wrote:
> On 04/20/2014 11:53 AM, Sami Kerola wrote:
> > On 16 April 2014 11:13, Karel Zak <kzak@redhat.com> wrote:
> >> On Tue, Apr 15, 2014 at 12:15:33PM +0100, Sami Kerola wrote:
> >>> +HELPER_SYMLINK="$TS_CWD/$(mktemp -u XXXXXXXXXXXXXXX)"
> >>> +ln -s "$TS_HELPER_SIGRECEIVE" "$HELPER_SYMLINK"
> >>> +trap "rm -f $HELPER_SYMLINK" 0
> >>> +
> >>> +su nobody -s /bin/sh -c "$HELPER_SYMLINK $TS_CWD/nobody" >> "$TS_OUTPUT" 2>&1 &
> >>
> >> I don't understand this idea, on my system Mr.Nobody can not write
> >> to my directories.
> >>
> >> It would be also better to add --setgit and --setuid to the helper to
> >> avoid extra su(1) process, then you can use TEST_PID=$!
> > 
> > The --setuid is added, [...]
> 
> yes, setuid is a great idea, yet I'm not sure
> if using UID 1 is a good idea:
> 
> 		if (setuid(1) < 0)
> 			err(TEST_SIGRECEIVE_FAILURE, "setuid failed");

 Hmm, I though about non-hardcoded uid

    --seruid <uid>

 or so...  BTW, we have

   tests/commands.sh:TS_TESTUSER=${TS_TESTUSER:-"test"}

 maybe we can use "nobody" rather than the "test", and if the
 $TS_TESTUSER does not exist then ts_skip the test. Currently the
 $TS_TESTUSER is nowhere used.

> >>    awk '/SigCgt/ { print $2}' /proc/<pid>/status
> >>
> >> to check if the signal handlers are already initialized (the final
> >> mask is 800000027ffbfeff on my system).
> 
> 
> > That is a much better way to determine if the process is ready to be
> > killed. Implemented in my git, and I have only one remaining question.
> 
> I'm not a big fan of magic numbers. Is there a chance to narrow down
> the relevant bits?

 Good point, hardcoded mask is probably bad idea as we use #ifdefs for
 the signals in the code.

 We don't have to test whole mask, just test for the last signal used
 in the test_sigreceive.c, for example if the last signal specified
 by sigaction() in the code is SIGHUP (=1), then


    sigmask=$((16#$( awk '/SigCgt/ { print $2}' /proc/$TEST_PID/status) ))

    if [ $(( $sigmask & 1 )) == 1 ]; then
       echo "yes, test program is ready"
    fi

 is enough.


 Note that now the code add SIGHUP as the first thing to the mask.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 7/9] tests: check kill all user processes
  2014-04-20  9:53     ` Sami Kerola
  2014-04-20 15:36       ` Bernhard Voelker
@ 2014-04-22 10:12       ` Karel Zak
  1 sibling, 0 replies; 18+ messages in thread
From: Karel Zak @ 2014-04-22 10:12 UTC (permalink / raw)
  To: kerolasa; +Cc: util-linux, Bernhard Voelker

On Sun, Apr 20, 2014 at 10:53:47AM +0100, Sami Kerola wrote:
> Would you prefer me to send the patch set again or is review from
> remote branch OK? The biggest change is use of SigCgt, that is done
> the same way in all scripts and can be seen for instance looking this
> change.

 Keep the things in your repo, that's good enough. Thanks!

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 7/9] tests: check kill all user processes
  2014-04-22 10:09         ` Karel Zak
@ 2014-04-22 20:55           ` Sami Kerola
  0 siblings, 0 replies; 18+ messages in thread
From: Sami Kerola @ 2014-04-22 20:55 UTC (permalink / raw)
  To: Karel Zak; +Cc: Bernhard Voelker, util-linux

On 22 April 2014 11:09, Karel Zak <kzak@redhat.com> wrote:
> On Sun, Apr 20, 2014 at 05:36:29PM +0200, Bernhard Voelker wrote:
>> On 04/20/2014 11:53 AM, Sami Kerola wrote:
>> > On 16 April 2014 11:13, Karel Zak <kzak@redhat.com> wrote:
>> >> On Tue, Apr 15, 2014 at 12:15:33PM +0100, Sami Kerola wrote:
>> >>> +HELPER_SYMLINK="$TS_CWD/$(mktemp -u XXXXXXXXXXXXXXX)"
>> >>> +ln -s "$TS_HELPER_SIGRECEIVE" "$HELPER_SYMLINK"
>> >>> +trap "rm -f $HELPER_SYMLINK" 0
>> >>> +
>> >>> +su nobody -s /bin/sh -c "$HELPER_SYMLINK $TS_CWD/nobody" >> "$TS_OUTPUT" 2>&1 &
>> >>
>> >> I don't understand this idea, on my system Mr.Nobody can not write
>> >> to my directories.
>> >>
>> >> It would be also better to add --setgit and --setuid to the helper to
>> >> avoid extra su(1) process, then you can use TEST_PID=$!
>> >
>> > The --setuid is added, [...]
>>
>> yes, setuid is a great idea, yet I'm not sure
>> if using UID 1 is a good idea:
>>
>>               if (setuid(1) < 0)
>>                       err(TEST_SIGRECEIVE_FAILURE, "setuid failed");

My thinking was that anything but zero uid is fine, as the test is
required to be run as root.  So hardcoding with a numeric value of 1 felt
a bit better than relying that an user account might be available.  That
said mr nobody is probably part of every single linux since early days.

>  Hmm, I though about non-hardcoded uid
>
>     --seruid <uid>
>
>  or so...  BTW, we have
>
>    tests/commands.sh:TS_TESTUSER=${TS_TESTUSER:-"test"}
>
>  maybe we can use "nobody" rather than the "test", and if the
>  $TS_TESTUSER does not exist then ts_skip the test. Currently the
>  $TS_TESTUSER is nowhere used.

The test_sigreceiver --setuid requires now an argument that is either a
login name or uid.

>> >>    awk '/SigCgt/ { print $2}' /proc/<pid>/status
>> >>
>> >> to check if the signal handlers are already initialized (the final
>> >> mask is 800000027ffbfeff on my system).
>>
>> > That is a much better way to determine if the process is ready to be
>> > killed. Implemented in my git, and I have only one remaining question.
>>
>> I'm not a big fan of magic numbers. Is there a chance to narrow down
>> the relevant bits?
>
>  Good point, hardcoded mask is probably bad idea as we use #ifdefs for
>  the signals in the code.
>
>  We don't have to test whole mask, just test for the last signal used
>  in the test_sigreceive.c, for example if the last signal specified
>  by sigaction() in the code is SIGHUP (=1), then
>
>     sigmask=$((16#$( awk '/SigCgt/ { print $2}' /proc/$TEST_PID/status) ))
>
>     if [ $(( $sigmask & 1 )) == 1 ]; then
>        echo "yes, test program is ready"
>     fi
>
>  is enough.

I had a look of awk manual to see if it could do the bit operation so
that rather awkward looking shell expression could be avoided, which
turned out to be (IMHO) nice way to do the SIGHUP bit check.

>  Note that now the code add SIGHUP as the first thing to the mask.

Move to bottom of the helper, with a comment it should stay there.

Here are the changes to previous submissions, that are also available in
branch kill-tests-v4


diff --git a/tests/commands.sh b/tests/commands.sh
index 33bb1ab..4b5a7d9 100644
--- a/tests/commands.sh
+++ b/tests/commands.sh
@@ -1,5 +1,5 @@
 # Misc settings
-TS_TESTUSER=${TS_TESTUSER:-"test"}
+TS_TESTUSER=${TS_TESTUSER:-"nobody"}

 # helpers
 TS_HELPER_BYTESWAP="$top_builddir/test_byteswap"
diff --git a/tests/helpers/Makemodule.am b/tests/helpers/Makemodule.am
index 0b927e9..6ac5b7f 100644
--- a/tests/helpers/Makemodule.am
+++ b/tests/helpers/Makemodule.am
@@ -13,3 +13,4 @@ test_sysinfo_SOURCES = tests/helpers/test_sysinfo.c

 check_PROGRAMS += test_sigreceive
 test_sigreceive_SOURCES = tests/helpers/test_sigreceive.c
+test_sigreceive_LDADD = $(LDADD) libcommon.la
diff --git a/tests/helpers/test_sigreceive.c b/tests/helpers/test_sigreceive.c
index 6424fbf..c7a8949 100644
--- a/tests/helpers/test_sigreceive.c
+++ b/tests/helpers/test_sigreceive.c
@@ -19,17 +19,21 @@

 #include <err.h>
 #include <getopt.h>
+#include <pwd.h>
 #include <signal.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <sys/select.h>
+#include <sys/types.h>
 #include <unistd.h>

+#include "strutils.h"
+
 #define TEST_SIGRECEIVE_FAILURE 0

 static void __attribute__((__noreturn__)) usage(FILE *out)
 {
-	fputs("Usage: test_sigreceive [-s|--setuid]\n", out);
+	fputs("Usage: test_sigreceive [-s|--setuid <login|uid>]\n", out);
 	exit(TEST_SIGRECEIVE_FAILURE);
 }

@@ -44,18 +48,18 @@ int main(int argc, char **argv)
 	struct sigaction sigact;
 	fd_set rfds;
 	struct timeval timeout;
+	char *user = NULL;
 	int c;

 	static const struct option longopts[] = {
-		{"setuid", no_argument, NULL, 's'},
+		{"setuid", required_argument, NULL, 's'},
 		{NULL, 0, NULL, 0}
 	};

-	while ((c = getopt_long(argc, argv, "hs", longopts, NULL)) != -1)
+	while ((c = getopt_long(argc, argv, "s:h", longopts, NULL)) != -1)
 		switch (c) {
 		case 's':
-			if (setuid(1) < 0)
-				err(TEST_SIGRECEIVE_FAILURE, "setuid failed");
+			user = optarg;
 			break;
 		case 'h':
 			usage(stdout);
@@ -63,13 +67,25 @@ int main(int argc, char **argv)
 			usage(stderr);
 		}

+	if (user) {
+		struct passwd *pw;
+		uid_t uid;
+
+		pw = getpwnam(user);
+		if (pw)
+			uid = pw->pw_uid;
+		else
+			uid = strtou32_or_err(user, "failed to parse uid");
+		if (setuid(uid) < 0)
+			err(TEST_SIGRECEIVE_FAILURE, "setuid failed");
+	}
+
 	sigemptyset(&sigact.sa_mask);
 	sigact.sa_flags = 0;
 	sigact.sa_handler = exiter;
 	timeout.tv_sec = 5;
 	timeout.tv_usec = 0;

-	sigaction(SIGHUP, &sigact, NULL);
 	sigaction(SIGINT, &sigact, NULL);
 	sigaction(SIGQUIT, &sigact, NULL);
 	sigaction(SIGILL, &sigact, NULL);
@@ -87,7 +103,6 @@ int main(int argc, char **argv)
 	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);
@@ -102,7 +117,6 @@ int main(int argc, char **argv)
 	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);
@@ -146,12 +160,15 @@ int main(int argc, char **argv)
 	sigaction(SIGSYS, &sigact, NULL);
 #endif
 #ifdef SIGRTMIN
-	sigaction(SIGRTMIN+0, &sigact, NULL);
-	sigaction(SIGRTMAX+0, &sigact, NULL);
+	sigaction(SIGRTMIN, &sigact, NULL);
+	sigaction(SIGRTMAX, &sigact, NULL);
 #endif
+	/* Keep SIGHUP last, the bit it flips tells to check script the
+	 * helper is ready to be killed.  */
+	sigaction(SIGHUP, &sigact, NULL);

 	FD_ZERO(&rfds);
-	FD_SET(0, &rfds);
+	FD_SET(STDIN_FILENO, &rfds);
 	select(STDIN_FILENO, &rfds, NULL, NULL, &timeout);

 	exiter(TEST_SIGRECEIVE_FAILURE);
diff --git a/tests/ts/kill/all_processes b/tests/ts/kill/all_processes
index 196866d..8d62381 100755
--- a/tests/ts/kill/all_processes
+++ b/tests/ts/kill/all_processes
@@ -27,14 +27,15 @@ HELPER_SYMLINK="$TS_CWD/$(mktemp -u XXXXXXXXXXXXXXX)"
 ln -s "$TS_HELPER_SIGRECEIVE" "$HELPER_SYMLINK"
 trap "rm -f $HELPER_SYMLINK" 0

-"$HELPER_SYMLINK" -s >> "$TS_OUTPUT" 2>&1 &
+"$HELPER_SYMLINK" -s "$TS_TESTUSER" >> "$TS_OUTPUT" 2>&1 &
 TEST_PID=$!

 # test_sigreceive needs time to start up
 for i in 0.01 0.1 1 1 1 1; do
 	awk 'BEGIN { retval=1 }
 	$1 ~ /^SigCgt/ {
-		if ($2 == "800000027ffbfeff") {
+		lbyte = strtonum("0x" substr($2, 16, 16))
+		if (and(lbyte, 1)) {
 			retval=0
 		}
 	} END {
diff --git a/tests/ts/kill/name_to_number b/tests/ts/kill/name_to_number
index c9afff1..a6a70fc 100755
--- a/tests/ts/kill/name_to_number
+++ b/tests/ts/kill/name_to_number
@@ -46,7 +46,8 @@ for SIG in $($TS_CMD_KILL -L); do
 	for i in 0.01 0.1 1 1 1 1; do
 		awk 'BEGIN { retval=1 }
 		$1 ~ /^SigCgt/ {
-			if ($2 == "800000027ffbfeff") {
+			lbyte = strtonum("0x" substr($2, 16, 16))
+			if (and(lbyte, 1)) {
 				retval=0
 			}
 		} END {
diff --git a/tests/ts/kill/options b/tests/ts/kill/options
index f040a1b..33a9b9b 100755
--- a/tests/ts/kill/options
+++ b/tests/ts/kill/options
@@ -36,7 +36,8 @@ try_option()
 	for i in 0.01 0.1 1 1 1 1; do
 		awk 'BEGIN { retval=1 }
 		$1 ~ /^SigCgt/ {
-			if ($2 == "800000027ffbfeff") {
+			lbyte = strtonum("0x" substr($2, 16, 16))
+			if (and(lbyte, 1)) {
 				retval=0
 			}
 		} END {
diff --git a/tests/ts/kill/print_pid b/tests/ts/kill/print_pid
index efe47e9..46bbb13 100755
--- a/tests/ts/kill/print_pid
+++ b/tests/ts/kill/print_pid
@@ -34,7 +34,8 @@ up=0
 for i in 0.01 0.1 1 1 1 1; do
 	awk 'BEGIN { retval=1 }
 	$1 ~ /^SigCgt/ {
-		if ($2 == "800000027ffbfeff") {
+		lbyte = strtonum("0x" substr($2, 16, 16))
+		if (and(lbyte, 1)) {
 			retval=0
 		}
 	} END {

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

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/9] pull: kill tests
  2014-04-15 11:15 [PATCH 0/9] pull: kill tests Sami Kerola
                   ` (8 preceding siblings ...)
  2014-04-15 11:15 ` [PATCH 9/9] lib/procutils: reset errno before strtol() call Sami Kerola
@ 2014-04-28 10:03 ` Karel Zak
  9 siblings, 0 replies; 18+ messages in thread
From: Karel Zak @ 2014-04-28 10:03 UTC (permalink / raw)
  To: Sami Kerola; +Cc: util-linux

On Tue, Apr 15, 2014 at 12:15:26PM +0100, Sami Kerola wrote:
> Sami Kerola (9):
>       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
>       kill: add --verbose option to display what is killed
>       lib/procutils: reset errno before strtol() call

 Merged. I have moved the awk stuff to kill_functions.sh to avoid
 duplicate code in the tests.

 Thanks!

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2014-04-28 10:04 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-04-15 11:15 [PATCH 0/9] pull: kill tests Sami Kerola
2014-04-15 11:15 ` [PATCH 1/9] kill: make options --pid and --queue mutually exclusive Sami Kerola
2014-04-15 11:15 ` [PATCH 2/9] kill: remove unnecessary indirection Sami Kerola
2014-04-15 11:15 ` [PATCH 3/9] tests: add signal receiver program Sami Kerola
2014-04-15 12:20   ` Sami Kerola
2014-04-15 11:15 ` [PATCH 4/9] tests: check kill is converting signals names correctly Sami Kerola
2014-04-15 11:15 ` [PATCH 5/9] tests: check various ways to specify kill signal Sami Kerola
2014-04-15 11:15 ` [PATCH 6/9] tests: check kill print pid option Sami Kerola
2014-04-15 11:15 ` [PATCH 7/9] tests: check kill all user processes Sami Kerola
2014-04-16 10:13   ` Karel Zak
2014-04-20  9:53     ` Sami Kerola
2014-04-20 15:36       ` Bernhard Voelker
2014-04-22 10:09         ` Karel Zak
2014-04-22 20:55           ` Sami Kerola
2014-04-22 10:12       ` Karel Zak
2014-04-15 11:15 ` [PATCH 8/9] kill: add --verbose option to display what is killed Sami Kerola
2014-04-15 11:15 ` [PATCH 9/9] lib/procutils: reset errno before strtol() call Sami Kerola
2014-04-28 10:03 ` [PATCH 0/9] pull: kill tests Karel Zak

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).