[LTP] [PATCH 1/2] shell: Fix timeout process termination for zsh

[LTP] [PATCH 1/2] shell: Fix timeout process termination for zsh

[Joerg Vehlow]

From: Joerg Vehlow <joerg.vehlow@aox-tech.de>

NOTE: This fix should be part of the release,
because it fixes something broken in this release only!

On zsh an exit in the trap for SIGTERM is ignored,
but wait returns with TERM exit status (143).
This can only improve the situation for other processe,
e.g. if the wait is killed by the SIGTERM from the main process,
it will still terminate the timeout process now.

Fixes: a30410f6ad77 ("shell: Prevent orphan timeout sleep processes")

Signed-off-by: Joerg Vehlow <joerg.vehlow@aox-tech.de>
---
 testcases/lib/tst_test.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index 3a5651c01..1b25f4c44 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -472,6 +472,7 @@ _tst_timeout_process()
 	sleep_pid=$!
 	trap "kill $sleep_pid; exit" TERM
 	wait $sleep_pid
+	[ $? -eq 143 ] && exit
 	trap - TERM
 	_tst_kill_test
 }
-- 
2.25.1

[LTP] [PATCH 2/2] shell: Extend timeout tests, to run on multiple shells

[Joerg Vehlow]

From: Joerg Vehlow <joerg.vehlow@aox-tech.de>

There are some differences especially in signal handling
between the shells, so execute the tests on as many
shells as possible.

Signed-off-by: Joerg Vehlow <joerg.vehlow@aox-tech.de>
---
 lib/newlib_tests/shell/test_timeout.sh | 242 +++++++++++++++++--------
 1 file changed, 169 insertions(+), 73 deletions(-)

diff --git a/lib/newlib_tests/shell/test_timeout.sh b/lib/newlib_tests/shell/test_timeout.sh
index b05680cb1..152f51c32 100755
--- a/lib/newlib_tests/shell/test_timeout.sh
+++ b/lib/newlib_tests/shell/test_timeout.sh
@@ -5,6 +5,15 @@
 
 PATH="$(dirname $0):$(dirname $0)/../../../testcases/lib/:$PATH"
 
+SHELLS="
+bash
+dash
+busybox sh
+zsh
+ash
+ksh
+"
+
 # Test cases are separated by newlines.
 # Every test has the following fields in this order:
 # file
@@ -33,12 +42,23 @@ timeout04.sh|     |0|  |  2|0|0|1|Verify that timeout is enforced
 timeout02.sh|    2|1| 2|   |1|0|0|Test termination of timeout process
 "
 
+run_shell()
+{
+	local shell=$1
+	shift
+
+	eval $shell "$@"
+	return $?
+}
+
 # Executes a test
 # Parameter:
-#  - test:    The test to execute
-#  - timeout: The timeout multiplicator (optional)
-#  - use_cat: Pipe the output of the command through cat (optional)
-#             If this is used, the exit code is NOT returned!
+#  - shell:    The shell to be used while executing the test
+#  - shellarg: First parameter of shell (e.g. for busybox sh)
+#  - test:     The test to execute
+#  - timeout:  The timeout multiplicator (optional)
+#  - use_cat:  Pipe the output of the command through cat (optional)
+#              If this is used, the exit code is NOT returned!
 #
 # The function returns the following global variables:
 # - test_exit:     The exit code of the test
@@ -47,11 +67,13 @@ timeout02.sh|    2|1| 2|   |1|0|0|Test termination of timeout process
 # - test_passed:   The number of passed tests parsed from the summary
 # - test_failed:   The number of failed tests parsed from the summary
 # - test_broken:   The number of broken tests parsed from the summary
-run_test()
+exec_test()
 {
-	local test=$1
-	local timeout=$2
-	local use_cat=$3
+	local shell=$1
+	local shellarg=$2
+	local test=$3
+	local timeout=$4
+	local use_cat=$5
 	local tmpfile start end;
 
 	tmpfile=$(mktemp -t ltp_timeout_XXXXXXXX)
@@ -70,9 +92,9 @@ run_test()
 	# the sleep process was never killed, when a test finished
 	# before the timeout.
 	if [ "$use_cat" = "1" ]; then
-		LTP_TIMEOUT_MUL=$timeout $test 2>&1 | cat >$tmpfile
+		LTP_TIMEOUT_MUL=$timeout $shell $shellarg $test 2>&1 | cat >$tmpfile
 	else
-		LTP_TIMEOUT_MUL=$timeout $test 1>$tmpfile 2>&1
+		LTP_TIMEOUT_MUL=$timeout $shell $shellarg $test 1>$tmpfile 2>&1
 	fi
 	test_exit=$?
 	set +m
@@ -82,6 +104,9 @@ run_test()
 	test_output=$(cat $tmpfile)
 	rm $tmpfile
 
+	test_passed=-1
+	test_failed=-1
+	test_broken=-1
 	eval $(echo "$test_output" | awk '
 		BEGIN {sum=0}
 		$1 == "Summary:" {
@@ -96,83 +121,154 @@ run_test()
 	')
 }
 
-echo "Testing timeout in shell API"
-echo
+do_test()
+{
+	local failed test_nr old_ifs shell CLEANED_DATA test_max
+	local file cur_fails timeout use_cat max_runtime exp_exit
+	local exp_passed exp_failed exp_broken description
+	
+	shell="$1"
+	shellarg="$2"
 
-failed=0
-test_nr=0
+	failed=0
+	test_nr=0
 
-old_ifs="$IFS"
-IFS=$(printf "\n\b")
+	old_ifs="$IFS"
+	IFS=$(printf "\n\b")
 
-# Remove comments and empty lines
-CLEANED_DATA=$(echo "$DATA" | sed '/^\s*#/d;/^\s*$/d')
-test_max=$(echo "$CLEANED_DATA" | wc -l)
-for d in $CLEANED_DATA; do
+	# Remove comments and empty lines
+	CLEANED_DATA=$(echo "$DATA" | sed '/^\s*#/d;/^\s*$/d')
+	test_max=$(echo "$CLEANED_DATA" | wc -l)
+	for d in $CLEANED_DATA; do
+		IFS="$old_ifs"
+
+		file=$(echo $d | cut -d'|' -f1 | xargs)
+		timeout=$(echo $d | cut -d'|' -f2 | xargs)
+		use_cat=$(echo $d | cut -d'|' -f3 | xargs)
+		max_runtime=$(echo $d | cut -d'|' -f4 | xargs)
+		max_runtime=${max_runtime:--1}
+		exp_exit=$(echo $d | cut -d'|' -f5 | xargs)
+		exp_exit=${exp_exit:--1}
+		exp_passed=$(echo $d | cut -d'|' -f6 | xargs)
+		exp_passed=${exp_passed:--1}
+		exp_failed=$(echo $d | cut -d'|' -f7 | xargs)
+		exp_failed=${exp_failed:--1}
+		exp_broken=$(echo $d | cut -d'|' -f8 | xargs)
+		exp_broken=${exp_broken:--1}
+		description=$(echo $d | cut -d'|' -f9)
+
+		test_nr=$(($test_nr + 1))
+
+		cur_fails=0
+
+		if [ -z "$description" ]; then
+			description="$file (LTP_TIMEOUT_MUL='$timeout')"
+		fi
+
+		echo "=== $test_nr/$test_max $description ==="
+		exec_test "$shell" "$shellarg" "$file" "$timeout" "$use_cat"
+
+		if [ $max_runtime -ne -1 ] && [ $test_duration -gt $max_runtime ]; then
+			echo "FAILED (runtime: $test_duration, expected less than $max_runtime)"
+			cur_fails=$((cur_fails + 1))
+		fi
+
+		if [ $exp_passed -ne -1 ] && [ $exp_passed -ne $test_passed ]; then
+			echo "FAILED (passes: $test_passed, expected $exp_passed)"
+			cur_fails=$((cur_fails + 1))
+		fi
+
+		if [ $exp_failed -ne -1 ] && [ $exp_failed -ne $test_failed ]; then
+			echo "FAILED (failed: $test_failed, expected $exp_failed)"
+			cur_fails=$((cur_fails + 1))
+		fi
+
+		if [ $exp_broken -ne -1 ] && [ $exp_broken -ne $test_broken ]; then
+			echo "FAILED (broken: $test_broken, expected $exp_broken)"
+			cur_fails=$((cur_fails + 1))
+		fi
+
+		if [ $exp_exit -ne -1 ] && [ $test_exit -ne $exp_exit ]; then
+			echo "FAILED (exit code: $test_exit, expected $exp_exit)"
+			cur_fails=$((cur_fails + 1))
+		fi
+
+		if [ $cur_fails -gt 0 ]; then
+			failed=$((failed + 1))
+			echo "--------"
+			echo "$test_output"
+			echo "--------"
+		else
+			echo "PASSED"
+		fi
+		echo
+	done
 	IFS="$old_ifs"
 
-	file=$(echo $d | cut -d'|' -f1 | xargs)
-	timeout=$(echo $d | cut -d'|' -f2 | xargs)
-	use_cat=$(echo $d | cut -d'|' -f3 | xargs)
-	max_runtime=$(echo $d | cut -d'|' -f4 | xargs)
-	max_runtime=${max_runtime:--1}
-	exp_exit=$(echo $d | cut -d'|' -f5 | xargs)
-	exp_exit=${exp_exit:--1}
-	exp_passed=$(echo $d | cut -d'|' -f6 | xargs)
-	exp_passed=${exp_passed:--1}
-	exp_failed=$(echo $d | cut -d'|' -f7 | xargs)
-	exp_failed=${exp_failed:--1}
-	exp_broken=$(echo $d | cut -d'|' -f8 | xargs)
-	exp_broken=${exp_broken:--1}
-	description=$(echo $d | cut -d'|' -f9)
-
-	test_nr=$(($test_nr + 1))
-
-	cur_fails=0
-
-	if [ -z "$description" ]; then
-		description="$file (LTP_TIMEOUT_MUL='$timeout')"
-	fi
+	echo "Failed tests: $failed"
+	return $failed
+}
 
-	echo "=== $test_nr/$test_max $description ==="
-	run_test "$file" "$timeout" "$use_cat"
 
-	if [ $max_runtime -ne -1 ] && [ $test_duration -gt $max_runtime ]; then
-		echo "FAILED (runtime: $test_duration, expected less than $max_runtime)"
-		cur_fails=$((cur_fails + 1))
+print_results()
+{
+	echo
+	if [ -n "$raw_shell" ]; then
+		result=$(printf "%s\n%-15s %s" "$result" "$raw_shell" "INTERRUPTED")
+		failed_shells=$((failed_shells + 1))
 	fi
-
-	if [ $exp_passed -ne -1 ] && [ $exp_passed -ne $test_passed ]; then
-		echo "FAILED (passes: $test_passed, expected $exp_passed)"
-		cur_fails=$((cur_fails + 1))
+	echo
+	echo "----------------------------------------"
+	echo
+	echo "Summary:"
+	echo "$result"
+	echo
+	if [ $failed_shells -ne 0 ]; then
+		echo "A total number of $total_fails failed for $failed_shells shells"
+	else
+		echo "All tests passed"
 	fi
+}
 
-	if [ $exp_failed -ne -1 ] && [ $exp_failed -ne $test_failed ]; then
-		echo "FAILED (failed: $test_failed, expected $exp_failed)"
-		cur_fails=$((cur_fails + 1))
-	fi
+# For some reason at least in zsh, it can happen, that the whole
+# testrunner is killed, when the test result is piped through cat.
+# If the test was aborted using CTRL^C or kill, the output can be ignored,
+# otherwise these messages should never be visible.
+trap 'echo; echo "Test unexpectedly killed by SIGINT."; print_results; exit 1' INT
+trap 'echo; echo "Test unexpectedly killed by SIGTERM."; print_results; exit 1' TERM
 
-	if [ $exp_broken -ne -1 ] && [ $exp_broken -ne $test_broken ]; then
-		echo "FAILED (broken: $test_broken, expected $exp_broken)"
-		cur_fails=$((cur_fails + 1))
-	fi
+old_ifs="$IFS"
+IFS=$(printf "\n\b")
 
-	if [ $exp_exit -ne -1 ] && [ $test_exit -ne $exp_exit ]; then
-		echo "FAILED (exit code: $test_exit, expected $exp_exit)"
-		cur_fails=$((cur_fails + 1))
-	fi
+failed_shells=0
+total_fails=0
 
-	if [ $cur_fails -gt 0 ]; then
-		failed=$((failed + 1))
-		echo "--------"
-		echo "$test_output"
-		echo "--------"
+# Remove comments and empty lines
+CLEANED_SHELLS=$(echo "$SHELLS" | sed '/^\s*#/d;/^\s*$/d')
+shell_max=$(echo "$CLEANED_SHELLS" | wc -l)
+shell_nr=0
+result=""
+for raw_shell in $CLEANED_SHELLS; do
+	shell_nr=$(( shell_nr + 1 ))
+	echo "($shell_nr/$shell_max) Testing timeout in shell API with '$raw_shell'"
+	shellarg=$(echo "$raw_shell" | cut -sd' ' -f2)
+	shell=$(echo "$raw_shell" | cut -d' ' -f1)
+	res="BROKEN"
+	if ! $shell $shellarg -c true 2>/dev/null; then
+		echo "SKIPED: Shell not found"
+		res="SKIPED"
 	else
-		echo "PASSED"
+		res="PASSED"
+		do_test "$shell" "$shellarg"
+		if [ $? -ne 0 ]; then
+			res="FAILED ($?)"
+			total_fails=$((total_fails + $?))
+			failed_shells=$((failed_shells + 1))
+		fi
 	fi
-	echo
+	result=$(printf "%s\n%-15s %s" "$result" "$raw_shell" "$res")
 done
-IFS="$old_ifs"
+raw_shell=""
 
-echo "Failed tests: $failed"
-exit $failed
+print_results
+exit $failed_shells
-- 
2.25.1

[LTP] [PATCH 2/2] shell: Extend timeout tests, to run on multiple shells

[Cyril Hrubis]

Hi!
> There are some differences especially in signal handling
> between the shells, so execute the tests on as many
> shells as possible.

I'm not sure that we want to support anything but bash, dash and
busybox and even these three are enough trouble.

One of my friends once told me that it's easier to write a portable
shell than portable shell code and it looks like he was right...

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] shell: Extend timeout tests, to run on multiple shells

[Joerg Vehlow]

HI Cyril,

On 5/19/2021 12:28 PM, Cyril Hrubis wrote:
> Hi!
>> There are some differences especially in signal handling
>> between the shells, so execute the tests on as many
>> shells as possible.
> I'm not sure that we want to support anything but bash, dash and
> busybox and even these three are enough trouble.
>
> One of my friends once told me that it's easier to write a portable
> shell than portable shell code and it looks like he was right...
>
In general I would say: YES
But if at some point in the future there are features used, that really 
only work for one shell,
we can still reduce the tested shells for this script.

Btw: ksh is not really support, it complains about all local variables,
because it only allows them in "function <name>"-style functions.
But the timeout code still works even there.

J?rg

[LTP] [PATCH 2/2] shell: Extend timeout tests, to run on multiple shells

[Petr Vorel]

> HI Cyril,

> On 5/19/2021 12:28 PM, Cyril Hrubis wrote:
> > Hi!
> > > There are some differences especially in signal handling
> > > between the shells, so execute the tests on as many
> > > shells as possible.
> > I'm not sure that we want to support anything but bash, dash and
> > busybox and even these three are enough trouble.

> > One of my friends once told me that it's easier to write a portable
> > shell than portable shell code and it looks like he was right...

> In general I would say: YES
> But if at some point in the future there are features used, that really only
> work for one shell,
> we can still reduce the tested shells for this script.

> Btw: ksh is not really support, it complains about all local variables,
> because it only allows them in "function <name>"-style functions.
> But the timeout code still works even there.

Although I generally agree that scrips should be portable to some extent
(it's already hard trying to keep it POSIX), it should be for shells which are
used on Linux distros as a default shell (/bin/sh). That's why besides the
default bash on most of distros we also care about dash (Debian/Ubuntu by
default) and busybox sh implementation (embedded distros and Alpine).
I'm not aware of any distro using ksh, csh or zsh as a default.

Trying to remember when I met ksh as a default, I guess it was on some old
Solaris, more than decade ago :).

We state this in shell shoding style chapter [1]. Maybe I should have added it
into Supported kernel ... page [2], but not many people run shell tests anyway.

Kind regards,
Petr

[1] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#132-shell-coding-style
[2] https://github.com/linux-test-project/ltp/wiki/Supported-kernel,-libc,-toolchain-versions

> J?rg

[LTP] [PATCH 2/2] shell: Extend timeout tests, to run on multiple shells

[Petr Vorel]

Hi all,

I wonder what is the state of this patchset?
Do we still consider it?
Apart from reducing shells (i.e. which distro has ash, which is not
alias to other shell in the list) I'm not against non-default shells, but I
don't like how whole test gets complicated by this.

Also we're reinventing wheel with printing results, checking whether test exist
etc. Maybe using test API for this?

Kind regards,
Petr

[LTP] [PATCH 2/2] shell: Extend timeout tests, to run on multiple shells

[Joerg Vehlow]

Hi Petr,

On 6/21/2021 9:25 AM, Petr Vorel wrote:
> Hi all,
>
> I wonder what is the state of this patchset?
> Do we still consider it?
I still think this should be included, but we can drop shells if you like.
> Apart from reducing shells (i.e. which distro has ash, which is not
> alias to other shell in the list) I'm not against non-default shells, but I
> don't like how whole test gets complicated by this.
>
> Also we're reinventing wheel with printing results, checking whether test exist
> etc. Maybe using test API for this?
I know that this is reimplementing a lot of stuff, but I think I tried 
using the lib (a bit strange using the object under test to test it, but 
would be ok for me), but failed to do so.
I guess this was because it uses traps and the set -m stuff, but I am 
not sure anymore.

J?rg

[LTP] [PATCH 2/2] shell: Extend timeout tests, to run on multiple shells

[Petr Vorel]

Hi Joerg,

> Hi Petr,

> On 6/21/2021 9:25 AM, Petr Vorel wrote:
> > Hi all,

> > I wonder what is the state of this patchset?
> > Do we still consider it?
> I still think this should be included, but we can drop shells if you like.
> > Apart from reducing shells (i.e. which distro has ash, which is not
> > alias to other shell in the list) I'm not against non-default shells, but I
> > don't like how whole test gets complicated by this.

> > Also we're reinventing wheel with printing results, checking whether test exist
> > etc. Maybe using test API for this?
> I know that this is reimplementing a lot of stuff, but I think I tried using
> the lib (a bit strange using the object under test to test it, but would be
> ok for me), but failed to do so.
> I guess this was because it uses traps and the set -m stuff, but I am not
> sure anymore.
Yes, using lib for testing it's a bit strange. But test_timeout.sh is already
messy and patch increases it even more. I'd prefer to implement the test
coverage in a cleaner way. Probably to create some very basic testing library,
a wrapper of actual tests.

There was some effort [1], based on my previous work, which concentrated on
having metadata of expected output. That allows to run tests which TBROK by
purpose (quite a lot of tests). I plan to get back to it, but but first I'd like
to create make target for running tests [2].

Kind regards,
Petr

> J?rg

[1] https://patchwork.ozlabs.org/project/ltp/patch/ce675759672af52bea02c11d51bd7d10f0bcb5cb.1566500817.git.clanig@suse.com/
[2] https://patchwork.ozlabs.org/project/ltp/patch/20210618191252.12403-1-pvorel@suse.cz/

[LTP] [PATCH 1/2] shell: Fix timeout process termination for zsh

[Li Wang]

On Wed, May 19, 2021 at 2:31 PM Joerg Vehlow <lkml@jv-coder.de> wrote:
>
> From: Joerg Vehlow <joerg.vehlow@aox-tech.de>
>
> NOTE: This fix should be part of the release,
> because it fixes something broken in this release only!
>
> On zsh an exit in the trap for SIGTERM is ignored,

I'm wondering if this is zsh feature or a bug on a specific version?
if the latter, we probably have no need to fix it in LTP.
Btw, which zsh version do you use?

Odd, I tried on my laptop(Fedora34) with zsh-5.5.1-6.el8_1.2.x86_64,
but could NOT reproduce it.

my reproducer:
-----------------

# cat test.sh

echo "pid is $$" # send TERM to pid in another terminal

sleep 100 &
sleep_pid=$!

trap "kill $sleep_pid; exit;" TERM

wait $sleep_pid
[ $? -eq 143 ] && echo "FAIL"



> but wait returns with TERM exit status (143).
> This can only improve the situation for other processe,
> e.g. if the wait is killed by the SIGTERM from the main process,
> it will still terminate the timeout process now.
>
> Fixes: a30410f6ad77 ("shell: Prevent orphan timeout sleep processes")
>
> Signed-off-by: Joerg Vehlow <joerg.vehlow@aox-tech.de>
> ---
>  testcases/lib/tst_test.sh | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
> index 3a5651c01..1b25f4c44 100644
> --- a/testcases/lib/tst_test.sh
> +++ b/testcases/lib/tst_test.sh
> @@ -472,6 +472,7 @@ _tst_timeout_process()
>         sleep_pid=$!
>         trap "kill $sleep_pid; exit" TERM
>         wait $sleep_pid
> +       [ $? -eq 143 ] && exit
>         trap - TERM
>         _tst_kill_test
>  }
> --
> 2.25.1
>


--
Regards,
Li Wang

[LTP] [PATCH 1/2] shell: Fix timeout process termination for zsh

[Joerg Vehlow]

Hi Li,


On 5/19/2021 12:18 PM, Li Wang wrote:
>
> I'm wondering if this is zsh feature or a bug on a specific version?
> if the latter, we probably have no need to fix it in LTP.
> Btw, which zsh version do you use?
>
> Odd, I tried on my laptop(Fedora34) with zsh-5.5.1-6.el8_1.2.x86_64,
> but could NOT reproduce it.
>
> my reproducer:
> -----------------
>
> # cat test.sh
>
> echo "pid is $$" # send TERM to pid in another terminal
>
> sleep 100 &
> sleep_pid=$!
>
> trap "kill $sleep_pid; exit;" TERM
>
> wait $sleep_pid
> [ $? -eq 143 ] && echo "FAIL"
>

I may have something to do with subshells again...
I just tweaked your reproducer, to be a bit more like our "real 
scenario" and can reproduce it:

timeout()
{
 ??? sleep 100 &
 ??? sleep_pid=$!
 ??? trap "echo Received TERM; kill $sleep_pid; exit;" TERM
 ??? wait $sleep_pid
 ??? [ $? -eq 143 ] && echo "FAIL"
}

timeout &
pid=$!
sleep 1
kill $pid


$ bash test.sh
Received TERM

$ zsh test.sh
Received TERM
FAIL

J?rg

[LTP] [PATCH 1/2] shell: Fix timeout process termination for zsh

[Li Wang]

> I may have something to do with subshells again...
> I just tweaked your reproducer, to be a bit more like our "real
> scenario" and can reproduce it:

Yes, I can reproduce it now.

>
> timeout()
> {
>      sleep 100 &
>      sleep_pid=$!
>      trap "echo Received TERM; kill $sleep_pid; exit;" TERM
>      wait $sleep_pid
>      [ $? -eq 143 ] && echo "FAIL"
> }
>
> timeout &
> pid=$!
> sleep 1
> kill $pid
>
>
> $ bash test.sh
> Received TERM
>
> $ zsh test.sh
> Received TERM
> FAIL
>
> J?rg
>
>


-- 
Regards,
Li Wang