[LTP] [PATCH 0/7] zram cleanup, tst_set_timeout(timeout)

[LTP] [PATCH 0/7] zram cleanup, tst_set_timeout(timeout)

[Petr Vorel]

Cc: Cyril Hrubis <chrubis@suse.cz>,
    Li Wang <liwang@redhat.com>,
    Joerg Vehlow <joerg.vehlow@aox-tech.de>,
    Joerg Vehlow <lkml@jv-coder.de>

Petr Vorel (7):
  zram: Calculate dev_num variable
  zram01.sh: Generate test setup variables in setup
  zram: Add zram_compress_alg() to zram02.sh
  zram: Move test specific functions out of zram_lib.sh
  tst_test.sh: Introduce tst_set_timeout(timeout)
  tst_test.sh: Run cleanup also after test timeout
  zram: Increase timeout according to used devices

 doc/test-writing-guidelines.txt               | 16 ++-
 .../kernel/device-drivers/zram/zram01.sh      | 99 +++++++++++++++----
 .../kernel/device-drivers/zram/zram02.sh      | 48 +++++++--
 .../kernel/device-drivers/zram/zram_lib.sh    | 85 ++++------------
 testcases/lib/tst_test.sh                     | 34 +++++--
 5 files changed, 175 insertions(+), 107 deletions(-)

-- 
2.30.1

[LTP] [PATCH 1/7] zram: Calculate dev_num variable

[Petr Vorel]

Instead of requiring to be set.

Reviewed-by: Li Wang <liwang@redhat.com>
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Same as v2.

 testcases/kernel/device-drivers/zram/zram01.sh   |  8 +++-----
 testcases/kernel/device-drivers/zram/zram02.sh   |  8 +++-----
 testcases/kernel/device-drivers/zram/zram_lib.sh | 16 ++++++++++++++--
 3 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/testcases/kernel/device-drivers/zram/zram01.sh b/testcases/kernel/device-drivers/zram/zram01.sh
index 8de2c0cad..a795ff89f 100755
--- a/testcases/kernel/device-drivers/zram/zram01.sh
+++ b/testcases/kernel/device-drivers/zram/zram01.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 # Copyright (c) 2015 Oracle and/or its affiliates. All Rights Reserved.
-# Copyright (c) 2019 Petr Vorel <pvorel@suse.cz>
+# Copyright (c) 2019-2021 Petr Vorel <pvorel@suse.cz>
 # Author: Alexey Kodanev <alexey.kodanev@oracle.com>
 #
 # Test creates several zram devices with different filesystems on them.
@@ -11,10 +11,8 @@ TST_TESTFUNC="do_test"
 TST_NEEDS_CMDS="awk bc dd"
 . zram_lib.sh
 
-# Test will create the following number of zram devices:
-dev_num=4
-# This is a list of parameters for zram devices.
-# Number of items must be equal to 'dev_num' parameter.
+# List of parameters for zram devices.
+# For each number the test creates own zram device.
 zram_max_streams="2 3 5 8"
 
 FS_SIZE="402653184"
diff --git a/testcases/kernel/device-drivers/zram/zram02.sh b/testcases/kernel/device-drivers/zram/zram02.sh
index f97cf646c..b4d706568 100755
--- a/testcases/kernel/device-drivers/zram/zram02.sh
+++ b/testcases/kernel/device-drivers/zram/zram02.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 # Copyright (c) 2015 Oracle and/or its affiliates. All Rights Reserved.
-# Copyright (c) 2019 Petr Vorel <pvorel@suse.cz>
+# Copyright (c) 2019-2021 Petr Vorel <pvorel@suse.cz>
 # Author: Alexey Kodanev <alexey.kodanev@oracle.com>
 #
 # Test checks that we can create swap zram device.
@@ -9,10 +9,8 @@ TST_CNT=5
 TST_TESTFUNC="do_test"
 . zram_lib.sh
 
-# Test will create the following number of zram devices:
-dev_num=1
-# This is a list of parameters for zram devices.
-# Number of items must be equal to 'dev_num' parameter.
+# List of parameters for zram devices.
+# For each number the test creates own zram device.
 zram_max_streams="2"
 
 # The zram sysfs node 'disksize' value can be either in bytes,
diff --git a/testcases/kernel/device-drivers/zram/zram_lib.sh b/testcases/kernel/device-drivers/zram/zram_lib.sh
index 6fa6552ca..a7e8b9f5b 100755
--- a/testcases/kernel/device-drivers/zram/zram_lib.sh
+++ b/testcases/kernel/device-drivers/zram/zram_lib.sh
@@ -34,7 +34,19 @@ zram_cleanup()
 
 zram_load()
 {
+	local tmp
+
+	dev_num=0
+	for tmp in $zram_max_streams; do
+		dev_num=$((dev_num+1))
+	done
+
+	if [ $dev_num -le 0 ]; then
+		tst_brk TBROK "dev_num must be > 0"
+	fi
+
 	tst_res TINFO "create '$dev_num' zram device(s)"
+
 	modprobe zram num_devices=$dev_num || \
 		tst_brk TBROK "failed to insert zram module"
 
@@ -42,9 +54,9 @@ zram_load()
 
 	if [ "$dev_num_created" -ne "$dev_num" ]; then
 		tst_brk TFAIL "unexpected num of devices: $dev_num_created"
-	else
-		tst_res TPASS "test succeeded"
 	fi
+
+	tst_res TPASS "all zram devices successfully created"
 }
 
 zram_max_streams()
-- 
2.30.1

[LTP] [PATCH 2/7] zram01.sh: Generate test setup variables in setup

[Petr Vorel]

Generate variables in setup, based on output of tst_supported_fs.
This is more clean approach and it fixes various things:

* Error when there is no filesystem support and also mkfs.ext2
  (fallback) not installed:
/opt/ltp/testcases/bin/zram01.sh: line 198: mkfs.ext2: not found
Instead quit if there is no fs support. But this can lead to skipping
zram_compress_alg(), it will be solved next commit by moving
zram_compress_alg() to zram02.sh.

* Having ext2 as fallback could lead to run it more than once.
There is no much point to do that.

* Drop tst_require_cmds mkfs check, because mkfs is not actually needed.

Improvements:

* Test all suitable filesystems (will need increase timeout).

Reviewed-by: Li Wang <liwang@redhat.com>
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Changes v2->v3:
* drop filter "| grep -v -e fat -e ntfs -e fuse" (Li prefers to keep it,
  not against it, it's just why to avoid testing it when we don't do in
  other tests?)
* rename functions: s/check_space_for_btrfs/get_btrfs_size/
  s/generate_vars/initialize_vars/ (Li)
* drop unneeded check [ $i -eq $dev_num ] && break (which is good :)


 .../kernel/device-drivers/zram/zram01.sh      | 62 ++++++++++++++-----
 .../kernel/device-drivers/zram/zram_lib.sh    | 15 +++--
 2 files changed, 53 insertions(+), 24 deletions(-)

diff --git a/testcases/kernel/device-drivers/zram/zram01.sh b/testcases/kernel/device-drivers/zram/zram01.sh
index a795ff89f..54f7d0ebd 100755
--- a/testcases/kernel/device-drivers/zram/zram01.sh
+++ b/testcases/kernel/device-drivers/zram/zram01.sh
@@ -8,23 +8,25 @@
 
 TST_CNT=7
 TST_TESTFUNC="do_test"
-TST_NEEDS_CMDS="awk bc dd"
+TST_NEEDS_CMDS="awk bc dd grep"
 . zram_lib.sh
+TST_SETUP="setup"
 
-# List of parameters for zram devices.
-# For each number the test creates own zram device.
-zram_max_streams="2 3 5 8"
-
-FS_SIZE="402653184"
-FS_TYPE="btrfs"
+check_space_for_btrfs()
+{
+	local ram_size
 
-RAM_SIZE=$(awk '/MemTotal:/ {print $2}' /proc/meminfo)
-if [ "$RAM_SIZE" -lt 1048576 ]; then
-	tst_res TINFO "not enough space for Btrfs"
-	FS_SIZE="26214400"
-	FS_TYPE="ext2"
-fi
+	ram_size=$(awk '/MemTotal:/ {print $2}' /proc/meminfo)
+	if [ "$ram_size" -lt 1048576 ]; then
+		tst_res TINFO "not enough space for Btrfs"
+		return 1
+	fi
+	return 0
+}
 
+# List of parameters for zram devices.
+# For each number the test creates own zram device.
+# NOTE about size:
 # The zram sysfs node 'disksize' value can be either in bytes,
 # or you can use mem suffixes. But in some old kernels, mem
 # suffixes are not supported, for example, in RHEL6.6GA's kernel
@@ -32,9 +34,37 @@ fi
 # not support mem suffixes, in some newer kernels, they use
 # memparse() which supports mem suffixes. So here we just use
 # bytes to make sure everything works correctly.
-zram_sizes="26214400 26214400 26214400 $FS_SIZE"
-zram_mem_limits="25M 25M 25M $((FS_SIZE/1024/1024))M"
-zram_filesystems="ext3 ext4 xfs $FS_TYPE"
+initialize_vars()
+{
+	local fs limit size stream=-1
+	dev_num=0
+
+	for fs in $(tst_supported_fs); do
+		size="26214400"
+		limit="25M"
+		if [ "$fs" = "btrfs" ]; then
+			check_space_for_btrfs || continue
+			size="402653184"
+			limit="$((size/1024/1024))M"
+		fi
+
+		stream=$((stream+3))
+		dev_num=$((dev_num+1))
+		zram_filesystems="$zram_filesystems $fs"
+		zram_mem_limits="$zram_mem_limits $limit"
+		zram_sizes="$zram_sizes $size"
+		zram_max_streams="$zram_max_streams $stream"
+	done
+
+	[ $dev_num -eq 0 ] && \
+		tst_brk TCONF "no suitable filesystem"
+}
+
+setup()
+{
+	initialize_vars
+	zram_load
+}
 
 zram_fill_fs()
 {
diff --git a/testcases/kernel/device-drivers/zram/zram_lib.sh b/testcases/kernel/device-drivers/zram/zram_lib.sh
index a7e8b9f5b..c0a9f4618 100755
--- a/testcases/kernel/device-drivers/zram/zram_lib.sh
+++ b/testcases/kernel/device-drivers/zram/zram_lib.sh
@@ -36,10 +36,12 @@ zram_load()
 {
 	local tmp
 
-	dev_num=0
-	for tmp in $zram_max_streams; do
-		dev_num=$((dev_num+1))
-	done
+	if [ -z "$dev_num" ]; then
+		dev_num=0
+		for tmp in $zram_max_streams; do
+			dev_num=$((dev_num+1))
+		done
+	fi
 
 	if [ $dev_num -le 0 ]; then
 		tst_brk TBROK "dev_num must be > 0"
@@ -191,13 +193,10 @@ zram_swapoff()
 
 zram_makefs()
 {
-	tst_require_cmds mkfs
 	local i=0
+	local fs
 
 	for fs in $zram_filesystems; do
-		# if requested fs not supported default it to ext2
-		tst_supported_fs $fs 2> /dev/null || fs=ext2
-
 		tst_res TINFO "make $fs filesystem on /dev/zram$i"
 		mkfs.$fs /dev/zram$i > err.log 2>&1
 		if [ $? -ne 0 ]; then
-- 
2.30.1

[LTP] [PATCH 3/7] zram: Add zram_compress_alg() to zram02.sh

[Petr Vorel]

Quit at setup in case there is no fs support (change in previous commit)
can lead to skipping zram_compress_alg(). Add it to zram02.sh where is no
such limitation and still keep it in zram01.sh since the tests
do depend on each other, the zram_fill_fs actually checks compression
ration, which only makes sense if compression is enabled.

Reviewed-by: Li Wang <liwang@redhat.com>
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Changes v2->v3:
* keep zram_compress_alg() also in zram01.sh (Cyril)

 testcases/kernel/device-drivers/zram/zram02.sh | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/testcases/kernel/device-drivers/zram/zram02.sh b/testcases/kernel/device-drivers/zram/zram02.sh
index b4d706568..d09977ec1 100755
--- a/testcases/kernel/device-drivers/zram/zram02.sh
+++ b/testcases/kernel/device-drivers/zram/zram02.sh
@@ -5,7 +5,7 @@
 #
 # Test checks that we can create swap zram device.
 
-TST_CNT=5
+TST_CNT=6
 TST_TESTFUNC="do_test"
 . zram_lib.sh
 
@@ -27,10 +27,11 @@ do_test()
 {
 	case $1 in
 	 1) zram_max_streams;;
-	 2) zram_set_disksizes;;
-	 3) zram_set_memlimit;;
-	 4) zram_makeswap;;
-	 5) zram_swapoff;;
+	 2) zram_compress_alg;;
+	 3) zram_set_disksizes;;
+	 4) zram_set_memlimit;;
+	 5) zram_makeswap;;
+	 6) zram_swapoff;;
 	esac
 }
 
-- 
2.30.1

[LTP] [PATCH 4/7] zram: Move test specific functions out of zram_lib.sh

[Petr Vorel]

Refactoring, as code is confusing enough, when use global variables and
functions which are single test specific + there is zram_fill_fs already
in zram01.sh.

Reviewed-by: Li Wang <liwang@redhat.com>
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Changes v2->v3:
* keep zram_compress_alg() here due it's use in zram01.sh (Cyril)

 .../kernel/device-drivers/zram/zram01.sh      | 33 ++++++++++
 .../kernel/device-drivers/zram/zram02.sh      | 29 +++++++++
 .../kernel/device-drivers/zram/zram_lib.sh    | 62 -------------------
 3 files changed, 62 insertions(+), 62 deletions(-)

diff --git a/testcases/kernel/device-drivers/zram/zram01.sh b/testcases/kernel/device-drivers/zram/zram01.sh
index 54f7d0ebd..6dce89325 100755
--- a/testcases/kernel/device-drivers/zram/zram01.sh
+++ b/testcases/kernel/device-drivers/zram/zram01.sh
@@ -66,6 +66,39 @@ setup()
 	zram_load
 }
 
+zram_makefs()
+{
+	local i=0
+	local fs
+
+	for fs in $zram_filesystems; do
+		tst_res TINFO "make $fs filesystem on /dev/zram$i"
+		mkfs.$fs /dev/zram$i > err.log 2>&1
+		if [ $? -ne 0 ]; then
+			cat err.log
+			tst_brk TFAIL "failed to make $fs on /dev/zram$i"
+		fi
+
+		i=$(($i + 1))
+	done
+
+	tst_res TPASS "zram_makefs succeeded"
+}
+
+zram_mount()
+{
+	local i=0
+
+	for i in $(seq 0 $(($dev_num - 1))); do
+		tst_res TINFO "mount /dev/zram$i"
+		mkdir zram$i
+		ROD mount /dev/zram$i zram$i
+		dev_mounted=$i
+	done
+
+	tst_res TPASS "mount of zram device(s) succeeded"
+}
+
 zram_fill_fs()
 {
 	for i in $(seq 0 $(($dev_num - 1))); do
diff --git a/testcases/kernel/device-drivers/zram/zram02.sh b/testcases/kernel/device-drivers/zram/zram02.sh
index d09977ec1..f0421ce7f 100755
--- a/testcases/kernel/device-drivers/zram/zram02.sh
+++ b/testcases/kernel/device-drivers/zram/zram02.sh
@@ -23,6 +23,35 @@ zram_max_streams="2"
 zram_sizes="107374182400" # 100GB
 zram_mem_limits="1M"
 
+zram_makeswap()
+{
+	tst_res TINFO "make swap with zram device(s)"
+	tst_require_cmds mkswap swapon swapoff
+	local i=0
+
+	for i in $(seq 0 $(($dev_num - 1))); do
+		ROD mkswap /dev/zram$i
+		ROD swapon /dev/zram$i
+		tst_res TINFO "done with /dev/zram$i"
+		dev_makeswap=$i
+	done
+
+	tst_res TPASS "making zram swap succeeded"
+}
+
+zram_swapoff()
+{
+	tst_require_cmds swapoff
+	local i
+
+	for i in $(seq 0 $dev_makeswap); do
+		ROD swapoff /dev/zram$i
+	done
+	dev_makeswap=-1
+
+	tst_res TPASS "swapoff completed"
+}
+
 do_test()
 {
 	case $1 in
diff --git a/testcases/kernel/device-drivers/zram/zram_lib.sh b/testcases/kernel/device-drivers/zram/zram_lib.sh
index c0a9f4618..65e431e86 100755
--- a/testcases/kernel/device-drivers/zram/zram_lib.sh
+++ b/testcases/kernel/device-drivers/zram/zram_lib.sh
@@ -161,65 +161,3 @@ zram_set_memlimit()
 
 	tst_res TPASS "test succeeded"
 }
-
-zram_makeswap()
-{
-	tst_res TINFO "make swap with zram device(s)"
-	tst_require_cmds mkswap swapon swapoff
-	local i=0
-
-	for i in $(seq 0 $(($dev_num - 1))); do
-		ROD mkswap /dev/zram$i
-		ROD swapon /dev/zram$i
-		tst_res TINFO "done with /dev/zram$i"
-		dev_makeswap=$i
-	done
-
-	tst_res TPASS "making zram swap succeeded"
-}
-
-zram_swapoff()
-{
-	tst_require_cmds swapoff
-	local i
-
-	for i in $(seq 0 $dev_makeswap); do
-		ROD swapoff /dev/zram$i
-	done
-	dev_makeswap=-1
-
-	tst_res TPASS "swapoff completed"
-}
-
-zram_makefs()
-{
-	local i=0
-	local fs
-
-	for fs in $zram_filesystems; do
-		tst_res TINFO "make $fs filesystem on /dev/zram$i"
-		mkfs.$fs /dev/zram$i > err.log 2>&1
-		if [ $? -ne 0 ]; then
-			cat err.log
-			tst_brk TFAIL "failed to make $fs on /dev/zram$i"
-		fi
-
-		i=$(($i + 1))
-	done
-
-	tst_res TPASS "zram_makefs succeeded"
-}
-
-zram_mount()
-{
-	local i=0
-
-	for i in $(seq 0 $(($dev_num - 1))); do
-		tst_res TINFO "mount /dev/zram$i"
-		mkdir zram$i
-		ROD mount /dev/zram$i zram$i
-		dev_mounted=$i
-	done
-
-	tst_res TPASS "mount of zram device(s) succeeded"
-}
-- 
2.30.1

[LTP] [PATCH 5/7] tst_test.sh: Introduce tst_set_timeout(timeout)

[Petr Vorel]

to sync with C API. This allows to setup timer after test has started.
It's useful when test length depends on input decided during setup.

Suggested-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
New in v3.

 doc/test-writing-guidelines.txt | 16 ++++++++++++----
 testcases/lib/tst_test.sh       | 23 ++++++++++++++++++-----
 2 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
index dd1911ceb..50696e14a 100644
--- a/doc/test-writing-guidelines.txt
+++ b/doc/test-writing-guidelines.txt
@@ -2393,8 +2393,8 @@ tst_run
 '$TST_TEST_DATA' can be used with '$TST_CNT'. If '$TST_TEST_DATA_IFS' not specified,
 space as default value is used. Of course, it's possible to use separate functions.
 
-2.3.2 Library environment variables for shell
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+2.3.2 Library environment variables and functions for shell
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Similarily to the C library various checks and preparations can be requested
 simply by setting right '$TST_NEEDS_FOO'.
@@ -2415,11 +2415,19 @@ simply by setting right '$TST_NEEDS_FOO'.
 | 'TST_TIMEOUT'      | Maximum timeout set for the test in sec. Must be int >= 1,
                        or -1 (special value to disable timeout), default is 300.
                        Variable is meant be set in tests, not by user.
-                       It's equivalent of `tst_test.timeout` in C.
+                       It's an equivalent of `tst_test.timeout` in C, can be set
+                       via 'tst_set_timeout(timeout)' after test has started.
+|=============================================================================
+
+[options="header"]
+|=============================================================================
+| Function name              | Action done
+| 'tst_set_timeout(timeout)' | Maximum timeout set for the test in sec.
+                               See 'TST_TIMEOUT' variable.
 |=============================================================================
 
 NOTE: Network tests (see testcases/network/README.md) use additional variables
-in 'tst_net.sh'.
+and functions in 'tst_net.sh'.
 
 Checking for presence of commands
 +++++++++++++++++++++++++++++++++
diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index 69f007d89..58056e28b 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 # SPDX-License-Identifier: GPL-2.0-or-later
-# Copyright (c) Linux Test Project, 2014-2020
+# Copyright (c) Linux Test Project, 2014-2021
 # Author: Cyril Hrubis <chrubis@suse.cz>
 #
 # LTP test library for shell.
@@ -23,6 +23,14 @@ export TST_LIB_LOADED=1
 # default trap function
 trap "tst_brk TBROK 'test interrupted'" INT
 
+_tst_cleanup_timer()
+{
+	if [ -n "$_tst_setup_timer_pid" ]; then
+		kill $_tst_setup_timer_pid 2>/dev/null
+		wait $_tst_setup_timer_pid 2>/dev/null
+	fi
+}
+
 _tst_do_exit()
 {
 	local ret=0
@@ -48,10 +56,7 @@ _tst_do_exit()
 		[ "$TST_TMPDIR_RHOST" = 1 ] && tst_cleanup_rhost
 	fi
 
-	if [ -n "$_tst_setup_timer_pid" ]; then
-		kill $_tst_setup_timer_pid 2>/dev/null
-		wait $_tst_setup_timer_pid 2>/dev/null
-	fi
+	_tst_cleanup_timer
 
 	if [ $TST_FAIL -gt 0 ]; then
 		ret=$((ret|1))
@@ -459,6 +464,8 @@ _tst_setup_timer()
 
 	tst_res TINFO "timeout per run is ${h}h ${m}m ${s}s"
 
+	_tst_cleanup_timer
+
 	sleep $sec && tst_res TBROK "test killed, timeout! If you are running on slow machine, try exporting LTP_TIMEOUT_MUL > 1" && kill -9 -$pid &
 
 	_tst_setup_timer_pid=$!
@@ -492,6 +499,12 @@ tst_require_module()
 	tst_res TINFO "Found module at '$TST_MODPATH'"
 }
 
+tst_set_timeout()
+{
+	TST_TIMEOUT="$1"
+	_tst_setup_timer
+}
+
 tst_run()
 {
 	local _tst_i
-- 
2.30.1

[LTP] [PATCH 6/7] tst_test.sh: Run cleanup also after test timeout

[Petr Vorel]

Also timeout requires to run a test cleanup (e.g. zram01.sh).
Thus send first SIGINT, but keep also SIGKILL for safety reasons
(after 5 sec to give some time to the cleanup function and
_tst_check_security_modules()).

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Originally posted in 
https://patchwork.ozlabs.org/project/ltp/patch/20210202101942.31328-1-pvorel@suse.cz/

* renamed function
* use signal names instead of numbers in kill parameters

 testcases/lib/tst_test.sh | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index 58056e28b..097f672a1 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -21,7 +21,7 @@ export TST_LIB_LOADED=1
 . tst_security.sh
 
 # default trap function
-trap "tst_brk TBROK 'test interrupted'" INT
+trap "tst_brk TBROK 'test interrupted or timed out'" INT
 
 _tst_cleanup_timer()
 {
@@ -442,6 +442,14 @@ _tst_multiply_timeout()
 	return 0
 }
 
+_tst_run_timer()
+{
+	tst_res TBROK "test killed, timeout! If you are running on slow machine, try exporting LTP_TIMEOUT_MUL > 1"
+	kill -INT -$pid
+	sleep 5
+	kill -KILL -$pid
+}
+
 _tst_setup_timer()
 {
 	TST_TIMEOUT=${TST_TIMEOUT:-300}
@@ -465,8 +473,7 @@ _tst_setup_timer()
 	tst_res TINFO "timeout per run is ${h}h ${m}m ${s}s"
 
 	_tst_cleanup_timer
-
-	sleep $sec && tst_res TBROK "test killed, timeout! If you are running on slow machine, try exporting LTP_TIMEOUT_MUL > 1" && kill -9 -$pid &
+	sleep $sec && _tst_run_timer &
 
 	_tst_setup_timer_pid=$!
 }
-- 
2.30.1

[LTP] [PATCH 7/7] zram: Increase timeout according to used devices

[Petr Vorel]

to avoid unexpected timeout, which occurred even on just 4 zram devices.

On my system where run with ext{2,3,4}, xfs, btrfs, vfat, exfat, ntfs
it run for 12 min, i.e. mean 90s. Multiply by security constant 5,
expecting 450 sec for each filesystem.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Changes v2->v3:
* increase timeout based on measurement (Cyril)

 testcases/kernel/device-drivers/zram/zram_lib.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/testcases/kernel/device-drivers/zram/zram_lib.sh b/testcases/kernel/device-drivers/zram/zram_lib.sh
index 65e431e86..fe9c915c3 100755
--- a/testcases/kernel/device-drivers/zram/zram_lib.sh
+++ b/testcases/kernel/device-drivers/zram/zram_lib.sh
@@ -47,6 +47,8 @@ zram_load()
 		tst_brk TBROK "dev_num must be > 0"
 	fi
 
+	tst_set_timeout $((dev_num*450))
+
 	tst_res TINFO "create '$dev_num' zram device(s)"
 
 	modprobe zram num_devices=$dev_num || \
-- 
2.30.1

[LTP] [PATCH 5/7] tst_test.sh: Introduce tst_set_timeout(timeout)

[Li Wang]

Hi Petr,

On Tue, Mar 2, 2021 at 6:02 AM Petr Vorel <pvorel@suse.cz> wrote:

> to sync with C API. This allows to setup timer after test has started.
> It's useful when test length depends on input decided during setup.
>
> Suggested-by: Cyril Hrubis <chrubis@suse.cz>
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> New in v3.
>
>  doc/test-writing-guidelines.txt | 16 ++++++++++++----
>  testcases/lib/tst_test.sh       | 23 ++++++++++++++++++-----
>  2 files changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/doc/test-writing-guidelines.txt
> b/doc/test-writing-guidelines.txt
> index dd1911ceb..50696e14a 100644
> --- a/doc/test-writing-guidelines.txt
> +++ b/doc/test-writing-guidelines.txt
> @@ -2393,8 +2393,8 @@ tst_run
>  '$TST_TEST_DATA' can be used with '$TST_CNT'. If '$TST_TEST_DATA_IFS' not
> specified,
>  space as default value is used. Of course, it's possible to use separate
> functions.
>
> -2.3.2 Library environment variables for shell
> -^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +2.3.2 Library environment variables and functions for shell
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
>  Similarily to the C library various checks and preparations can be
> requested
>  simply by setting right '$TST_NEEDS_FOO'.
> @@ -2415,11 +2415,19 @@ simply by setting right '$TST_NEEDS_FOO'.
>  | 'TST_TIMEOUT'      | Maximum timeout set for the test in sec. Must be
> int >= 1,
>                         or -1 (special value to disable timeout), default
> is 300.
>                         Variable is meant be set in tests, not by user.
> -                       It's equivalent of `tst_test.timeout` in C.
> +                       It's an equivalent of `tst_test.timeout` in C, can
> be set
> +                       via 'tst_set_timeout(timeout)' after test has
> started.
>
> +|=============================================================================
> +
> +[options="header"]
>
> +|=============================================================================
> +| Function name              | Action done
> +| 'tst_set_timeout(timeout)' | Maximum timeout set for the test in sec.
> +                               See 'TST_TIMEOUT' variable.
>
>  |=============================================================================
>
>  NOTE: Network tests (see testcases/network/README.md) use additional
> variables
> -in 'tst_net.sh'.
> +and functions in 'tst_net.sh'.
>
>  Checking for presence of commands
>  +++++++++++++++++++++++++++++++++
> diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
> index 69f007d89..58056e28b 100644
> --- a/testcases/lib/tst_test.sh
> +++ b/testcases/lib/tst_test.sh
> @@ -1,6 +1,6 @@
>  #!/bin/sh
>  # SPDX-License-Identifier: GPL-2.0-or-later
> -# Copyright (c) Linux Test Project, 2014-2020
> +# Copyright (c) Linux Test Project, 2014-2021
>  # Author: Cyril Hrubis <chrubis@suse.cz>
>  #
>  # LTP test library for shell.
> @@ -23,6 +23,14 @@ export TST_LIB_LOADED=1
>  # default trap function
>  trap "tst_brk TBROK 'test interrupted'" INT
>
> +_tst_cleanup_timer()
> +{
> +       if [ -n "$_tst_setup_timer_pid" ]; then
> +               kill $_tst_setup_timer_pid 2>/dev/null
> +               wait $_tst_setup_timer_pid 2>/dev/null
> +       fi
> +}
> +
>  _tst_do_exit()
>  {
>         local ret=0
> @@ -48,10 +56,7 @@ _tst_do_exit()
>                 [ "$TST_TMPDIR_RHOST" = 1 ] && tst_cleanup_rhost
>         fi
>
> -       if [ -n "$_tst_setup_timer_pid" ]; then
> -               kill $_tst_setup_timer_pid 2>/dev/null
> -               wait $_tst_setup_timer_pid 2>/dev/null
> -       fi
> +       _tst_cleanup_timer
>
>         if [ $TST_FAIL -gt 0 ]; then
>                 ret=$((ret|1))
> @@ -459,6 +464,8 @@ _tst_setup_timer()
>
>         tst_res TINFO "timeout per run is ${h}h ${m}m ${s}s"
>
> +       _tst_cleanup_timer
> +
>         sleep $sec && tst_res TBROK "test killed, timeout! If you are
> running on slow machine, try exporting LTP_TIMEOUT_MUL > 1" && kill -9
> -$pid &
>
>         _tst_setup_timer_pid=$!
> @@ -492,6 +499,12 @@ tst_require_module()
>         tst_res TINFO "Found module at '$TST_MODPATH'"
>  }
>
> +tst_set_timeout()
> +{
> +       TST_TIMEOUT="$1"
>

Not sure if we should check "$1" is valid again before using it.

I guess in most scenarios, the function is invoked by tests, so
just needs to guarantee $1 > $TST_TIMEOUT, otherwise, it
looks meaningless to reset TST_TIMEOUT?
(especially to avoid people set a smaller value by a typo)



> +       _tst_setup_timer
> +}
> +
>  tst_run()
>  {
>         local _tst_i
> --
> 2.30.1
>
>

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20210302/b2aa7a21/attachment.htm>

[LTP] [PATCH 6/7] tst_test.sh: Run cleanup also after test timeout

[Li Wang]

On Tue, Mar 2, 2021 at 6:02 AM Petr Vorel <pvorel@suse.cz> wrote:

> Also timeout requires to run a test cleanup (e.g. zram01.sh).
> Thus send first SIGINT, but keep also SIGKILL for safety reasons
> (after 5 sec to give some time to the cleanup function and
> _tst_check_security_modules()).
>
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> Originally posted in
>
> https://patchwork.ozlabs.org/project/ltp/patch/20210202101942.31328-1-pvorel@suse.cz/
>
> * renamed function
> * use signal names instead of numbers in kill parameters
>
>  testcases/lib/tst_test.sh | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
> index 58056e28b..097f672a1 100644
> --- a/testcases/lib/tst_test.sh
> +++ b/testcases/lib/tst_test.sh
> @@ -21,7 +21,7 @@ export TST_LIB_LOADED=1
>  . tst_security.sh
>
>  # default trap function
> -trap "tst_brk TBROK 'test interrupted'" INT
> +trap "tst_brk TBROK 'test interrupted or timed out'" INT
>
>  _tst_cleanup_timer()
>  {
> @@ -442,6 +442,14 @@ _tst_multiply_timeout()
>         return 0
>  }
>
> +_tst_run_timer()
>

Hmm, this name is not good than before, or rename to _tst_kill_timer_pid(),
_tst_stop_timer()?



> +{
> +       tst_res TBROK "test killed, timeout! If you are running on slow
> machine, try exporting LTP_TIMEOUT_MUL > 1"
> +       kill -INT -$pid
> +       sleep 5
> +       kill -KILL -$pid
> +}
> +
>  _tst_setup_timer()
>  {
>         TST_TIMEOUT=${TST_TIMEOUT:-300}
> @@ -465,8 +473,7 @@ _tst_setup_timer()
>         tst_res TINFO "timeout per run is ${h}h ${m}m ${s}s"
>
>         _tst_cleanup_timer
> -
> -       sleep $sec && tst_res TBROK "test killed, timeout! If you are
> running on slow machine, try exporting LTP_TIMEOUT_MUL > 1" && kill -9
> -$pid &
> +       sleep $sec && _tst_run_timer &
>
>         _tst_setup_timer_pid=$!
>  }
> --
> 2.30.1
>
>

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20210302/f675fe6c/attachment-0001.htm>

[LTP] [PATCH 7/7] zram: Increase timeout according to used devices

[Li Wang]

On Tue, Mar 2, 2021 at 6:02 AM Petr Vorel <pvorel@suse.cz> wrote:

> to avoid unexpected timeout, which occurred even on just 4 zram devices.
>
> On my system where run with ext{2,3,4}, xfs, btrfs, vfat, exfat, ntfs
> it run for 12 min, i.e. mean 90s. Multiply by security constant 5,
> expecting 450 sec for each filesystem.
>
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
>

Reviewed-by: Li Wang <liwang@redhat.com>

Apart from the two places(patch 5/7, 6/7) that need tweaking/discussion,
the patches look good.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20210302/d2d2695b/attachment.htm>

[LTP] [PATCH 5/7] tst_test.sh: Introduce tst_set_timeout(timeout)

[Cyril Hrubis]

Hi!
> > +tst_set_timeout()
> > +{
> > +       TST_TIMEOUT="$1"
> >
> 
> Not sure if we should check "$1" is valid again before using it.
> 
> I guess in most scenarios, the function is invoked by tests, so
> just needs to guarantee $1 > $TST_TIMEOUT, otherwise, it
> looks meaningless to reset TST_TIMEOUT?
> (especially to avoid people set a smaller value by a typo)

I can image where it may make sense to set the timeout smaller than
default dynamically. If we had a test that consists of many iterations
whose number depends on the actuall system we run on (the same as the
supported filesystem) but if the iterations are rather quick we may end
up in a situation where we run only one iteration and we will attempt to
set a timeout smaller than default in the setup() which wouldn't be
wrong.

Hence I would check that the value is greater than zero here instead.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 5/7] tst_test.sh: Introduce tst_set_timeout(timeout)

[Petr Vorel]

Hi Li,

...
> > +tst_set_timeout()
> > +{
> > +       TST_TIMEOUT="$1"


> Not sure if we should check "$1" is valid again before using it.
General check for int is done in _tst_setup_timer, but that's not what you mean.

> I guess in most scenarios, the function is invoked by tests, so
> just needs to guarantee $1 > $TST_TIMEOUT, otherwise, it
> looks meaningless to reset TST_TIMEOUT?
> (especially to avoid people set a smaller value by a typo)

I thought it's not necessary. TST_TIMEOUT is meant to be set in the test. But we
currently don't check for that (it can be wrongly set as environment variable)
and it'd be difficult to check that (I'd prefer not grep $TST_TEST_PATH, because
that would not work for shell libraries (e.g. zram_lib.sh).

The main reason for me is that C API allows to set lower value in the code, thus
I didn't want to prevent it in the shell => it'd be a diverse from C API.
We could add warning: "don't set TST_TIMEOUT" in docs and print warning/reset it
in runltp and runltp-ng, but that would not protect when running test itself.
It's really hard trying to have at least similar API with two fairly different
languages :).

Kind regards,
Petr

[LTP] [PATCH 6/7] tst_test.sh: Run cleanup also after test timeout

[Petr Vorel]

Hi Li,

...
> >  _tst_cleanup_timer()
> >  {
> > @@ -442,6 +442,14 @@ _tst_multiply_timeout()
> >         return 0
> >  }

> > +_tst_run_timer()


> Hmm, this name is not good than before, or rename to _tst_kill_timer_pid(),
> _tst_stop_timer()?

Good point. I slightly prefer _tst_stop_timer, but no hard feeling about it.


Kind regards,
Petr

> > +{
> > +       tst_res TBROK "test killed, timeout! If you are running on slow
> > machine, try exporting LTP_TIMEOUT_MUL > 1"
> > +       kill -INT -$pid
> > +       sleep 5
> > +       kill -KILL -$pid
> > +}
> > +

[LTP] [PATCH 5/7] tst_test.sh: Introduce tst_set_timeout(timeout)

[Petr Vorel]

Hi Cyril,

> Hi!
> > > +tst_set_timeout()
> > > +{
> > > +       TST_TIMEOUT="$1"


> > Not sure if we should check "$1" is valid again before using it.

> > I guess in most scenarios, the function is invoked by tests, so
> > just needs to guarantee $1 > $TST_TIMEOUT, otherwise, it
> > looks meaningless to reset TST_TIMEOUT?
> > (especially to avoid people set a smaller value by a typo)

> I can image where it may make sense to set the timeout smaller than
> default dynamically. If we had a test that consists of many iterations
> whose number depends on the actuall system we run on (the same as the
> supported filesystem) but if the iterations are rather quick we may end
> up in a situation where we run only one iteration and we will attempt to
> set a timeout smaller than default in the setup() which wouldn't be
> wrong.
+1, I couldn't agree more.

> Hence I would check that the value is greater than zero here instead.
I'd allow also to disable timeout with -1. And the rest of the checks
(i.e. int -1 or > 0) is in _tst_setup_timer().

Kind regards,
Petr

[LTP] [PATCH 5/7] tst_test.sh: Introduce tst_set_timeout(timeout)

[Li Wang]

Hi Petr, Cyril,

On Tue, Mar 2, 2021 at 6:26 PM Petr Vorel <pvorel@suse.cz> wrote:

> Hi Cyril,
>
> > Hi!
> > > > +tst_set_timeout()
> > > > +{
> > > > +       TST_TIMEOUT="$1"
>
>
> > > Not sure if we should check "$1" is valid again before using it.
>
> > > I guess in most scenarios, the function is invoked by tests, so
> > > just needs to guarantee $1 > $TST_TIMEOUT, otherwise, it
> > > looks meaningless to reset TST_TIMEOUT?
> > > (especially to avoid people set a smaller value by a typo)
>
> > I can image where it may make sense to set the timeout smaller than
> > default dynamically. If we had a test that consists of many iterations
> > whose number depends on the actuall system we run on (the same as the
> > supported filesystem) but if the iterations are rather quick we may end
> > up in a situation where we run only one iteration and we will attempt to
> > set a timeout smaller than default in the setup() which wouldn't be
> > wrong.
> +1, I couldn't agree more.
>

Sounds reasonable. Thanks for the explanation on this.

>
> > Hence I would check that the value is greater than zero here instead.
> I'd allow also to disable timeout with -1. And the rest of the checks
> (i.e. int -1 or > 0) is in _tst_setup_timer().
>

Allow setting -1 is good to me.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20210302/4a32d9ca/attachment.htm>

[LTP] [PATCH 7/7] zram: Increase timeout according to used devices

[Cyril Hrubis]

Hi!

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 6/7] tst_test.sh: Run cleanup also after test timeout

[Cyril Hrubis]

Hi!
> > >  _tst_cleanup_timer()
> > >  {
> > > @@ -442,6 +442,14 @@ _tst_multiply_timeout()
> > >         return 0
> > >  }
> 
> > > +_tst_run_timer()
> 
> 
> > Hmm, this name is not good than before, or rename to _tst_kill_timer_pid(),
> > _tst_stop_timer()?
> 
> Good point. I slightly prefer _tst_stop_timer, but no hard feeling about it.

Or _tst_kill_test()?

> > > +{
> > > +       tst_res TBROK "test killed, timeout! If you are running on slow
> > > machine, try exporting LTP_TIMEOUT_MUL > 1"
> > > +       kill -INT -$pid
> > > +       sleep 5
> > > +       kill -KILL -$pid

Maybe we should change the messages to reflect what is happening and
maybe we should check if the test is still running before sending
SIGKILL with kill -0 $pid?

	tst_res TBROK "Test timeouted, sending SIGINT, ...."
	kill -INT -$pid

	sleep 5

	if kill -0 $pid 2>&1 > /dev/null; then
		tst_res TBROK "Test still running, sending SIGKILL"
		kill -KILL -$pid
	fi

We can also bussy loop wait for the process to terminate, e.g. loop 10
times with sleep 1 in the body and break the loop if kill -0 $pid
returns failure.

> > > +}
> > > +

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 6/7] tst_test.sh: Run cleanup also after test timeout

[Petr Vorel]

Hi Cyril, Li,

> > > > +_tst_run_timer()

> > > Hmm, this name is not good than before, or rename to _tst_kill_timer_pid(),
> > > _tst_stop_timer()?

> > Good point. I slightly prefer _tst_stop_timer, but no hard feeling about it.

> Or _tst_kill_test()?
+1

> > > > +{
> > > > +       tst_res TBROK "test killed, timeout! If you are running on slow
> > > > machine, try exporting LTP_TIMEOUT_MUL > 1"
> > > > +       kill -INT -$pid
> > > > +       sleep 5
> > > > +       kill -KILL -$pid

> Maybe we should change the messages to reflect what is happening and
> maybe we should check if the test is still running before sending
> SIGKILL with kill -0 $pid?

> 	tst_res TBROK "Test timeouted, sending SIGINT, ...."
> 	kill -INT -$pid

> 	sleep 5

> 	if kill -0 $pid 2>&1 > /dev/null; then
> 		tst_res TBROK "Test still running, sending SIGKILL"
> 		kill -KILL -$pid
> 	fi

> We can also bussy loop wait for the process to terminate, e.g. loop 10
> times with sleep 1 in the body and break the loop if kill -0 $pid
> returns failure.
Busy loop wait 10 times + final -KILL make sense to me. I'm going to merge first
five commits and send v2 this + the last commit.

Kind regards,
Petr