[LTP] [PATCH 0/5] zram cleanup

[LTP] [PATCH 0/5] zram cleanup

[Petr Vorel]

Hi,

this is a cleanup which began as
https://patchwork.ozlabs.org/project/ltp/patch/20210114183226.794-1-pvorel@suse.cz/

More TODO:
* sometimes timeout on zram01, when all 4 filesystems are supported.
Maybe simple LTP_TIMEOUT_MUL=$dev_num
or LTP_TIMEOUT_MUL=$((dev_num/2)) # will be 2 only for 4 filesystems

* -i parameter does not work
If we want to even bother (maybe somebody would like to stress zram
using -i), the problem is: that zram_load() and zram_cleanup() are
setup/cleanup functions, but would have to be called for each iteration.

To fix this I can change them to be normal test functions and
zram_cleanup() also a cleanup function. But it's not a first time when I
miss in our API setup and cleanup to be run in each iteration
(.setup_iterations = foo, TST_SETUP_ITERATIONS=foo).

* maybe move section with checking /proc/meminfo to setup as well?

Kind regards,
Petr

Petr Vorel (5):
  zram: Require root
  zram: Calculate dev_num variable
  zram01.sh: Check properly mkfs.* dependencies
  zram: Move zram_compress_alg() to zram02.sh
  zram: Move test specific functions out of zram_lib.sh

 .../kernel/device-drivers/zram/zram01.sh      |  99 ++++++++++++++--
 .../kernel/device-drivers/zram/zram02.sh      |  77 ++++++++++--
 .../kernel/device-drivers/zram/zram_lib.sh    | 111 +++---------------
 3 files changed, 168 insertions(+), 119 deletions(-)

-- 
2.30.0

[LTP] [PATCH 1/5] zram: Require root

[Petr Vorel]

Calling modprobe, rmmod and working with zram device requires root.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/kernel/device-drivers/zram/zram_lib.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/testcases/kernel/device-drivers/zram/zram_lib.sh b/testcases/kernel/device-drivers/zram/zram_lib.sh
index bdbf2453a..6fa6552ca 100755
--- a/testcases/kernel/device-drivers/zram/zram_lib.sh
+++ b/testcases/kernel/device-drivers/zram/zram_lib.sh
@@ -7,6 +7,7 @@ dev_makeswap=-1
 dev_mounted=-1
 
 TST_NEEDS_TMPDIR=1
+TST_NEEDS_ROOT=1
 TST_SETUP="zram_load"
 TST_CLEANUP="zram_cleanup"
 TST_NEEDS_DRIVERS="zram"
-- 
2.30.0

[LTP] [PATCH 2/5] zram: Calculate dev_num variable

[Petr Vorel]

Instead of requiring to be set.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/kernel/device-drivers/zram/zram01.sh   |  8 +++-----
 testcases/kernel/device-drivers/zram/zram02.sh   |  8 +++-----
 testcases/kernel/device-drivers/zram/zram_lib.sh | 12 ++++++++++--
 3 files changed, 16 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..24bd37e1e 100755
--- a/testcases/kernel/device-drivers/zram/zram_lib.sh
+++ b/testcases/kernel/device-drivers/zram/zram_lib.sh
@@ -34,7 +34,15 @@ zram_cleanup()
 
 zram_load()
 {
+	local tmp
+
+	dev_num=0
+	for tmp in $zram_max_streams; do
+		dev_num=$((dev_num+1))
+	done
+
 	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 +50,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.0

[LTP] [PATCH 3/5] zram01.sh: Check properly mkfs.* dependencies

[Petr Vorel]

Move check for supported system to setup().

Lower down number of created zram when missing filesystems
instead of previous fixed number and using ext2 as a fallback.
That requires to quit early when looping over other variables.

Keep fallback to ext2, but only if no other fs available and there is
mkfs.ext2 available. There is no much point to run ext2 fallback more
than once.

This also fixes problem 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

Quit at setup in case there is no fs support. But this can lead to
skipping zram_compress_alg(). This will be solved next commit by moving
zram_compress_alg() to zram02.sh.

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

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 .../kernel/device-drivers/zram/zram01.sh      | 44 ++++++++++++++++++-
 .../kernel/device-drivers/zram/zram_lib.sh    | 18 ++++----
 2 files changed, 53 insertions(+), 9 deletions(-)

diff --git a/testcases/kernel/device-drivers/zram/zram01.sh b/testcases/kernel/device-drivers/zram/zram01.sh
index a795ff89f..ef4ff4d75 100755
--- a/testcases/kernel/device-drivers/zram/zram01.sh
+++ b/testcases/kernel/device-drivers/zram/zram01.sh
@@ -10,10 +10,12 @@ TST_CNT=7
 TST_TESTFUNC="do_test"
 TST_NEEDS_CMDS="awk bc dd"
 . 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"
+fallback_filesystem="ext2"
 
 FS_SIZE="402653184"
 FS_TYPE="btrfs"
@@ -22,7 +24,7 @@ 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"
+	FS_TYPE="$fallback_filesystem"
 fi
 
 # The zram sysfs node 'disksize' value can be either in bytes,
@@ -36,6 +38,46 @@ 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"
 
+check_fs_support()
+{
+	local fs unsupported
+	local msg="missing kernel support or mkfs for filesystems:"
+	dev_num=0
+
+	for fs in $zram_filesystems; do
+		if tst_supported_fs $fs 2> /dev/null; then
+			dev_num=$((dev_num+1))
+		else
+			unsupported="$unsupported $fs"
+		fi
+	done
+
+	if [ $dev_num -eq 0 -a "$fallback_filesystem" != "$FS_TYPE" ]; then
+		if tst_supported_fs $fallback_filesystem 2> /dev/null; then
+			dev_num=1
+		fi
+	fi
+
+	if [ $dev_num -eq 0 ]; then
+		tst_res TINFO "filesystems on the system"
+		tst_supported_fs > /dev/null
+
+		msg="$msg $zram_filesystems"
+		if [ "$fallback_filesystem" != "$FS_TYPE" ]; then
+			msg="$msg $fallback_filesystem"
+		fi
+		tst_brk TCONF "$msg"
+	fi
+
+	[ "$unsupported" ] && tst_res TINFO "$msg$unsupported"
+}
+
+setup()
+{
+	check_fs_support
+	zram_load
+}
+
 zram_fill_fs()
 {
 	for i in $(seq 0 $(($dev_num - 1))); do
diff --git a/testcases/kernel/device-drivers/zram/zram_lib.sh b/testcases/kernel/device-drivers/zram/zram_lib.sh
index 24bd37e1e..b2dce1d7d 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
 
 	tst_res TINFO "create '$dev_num' zram device(s)"
 
@@ -125,6 +127,7 @@ zram_set_disksizes()
 
 		i=$(($i + 1))
 		tst_res TINFO "$sys_path = '$ds' ($i/$dev_num)"
+		[ $i -eq $dev_num ] && break
 	done
 
 	tst_res TPASS "test succeeded"
@@ -151,6 +154,7 @@ zram_set_memlimit()
 
 		i=$(($i + 1))
 		tst_res TINFO "$sys_path = '$ds' ($i/$dev_num)"
+		[ $i -eq $dev_num ] && break
 	done
 
 	tst_res TPASS "test succeeded"
@@ -187,13 +191,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
@@ -202,6 +203,7 @@ zram_makefs()
 		fi
 
 		i=$(($i + 1))
+		[ $i -eq $dev_num ] && break
 	done
 
 	tst_res TPASS "zram_makefs succeeded"
-- 
2.30.0

[LTP] [PATCH 4/5] zram: Move 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(). Move to zram02.sh where is no
such limitation.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/kernel/device-drivers/zram/zram01.sh | 13 ++++++-------
 testcases/kernel/device-drivers/zram/zram02.sh | 11 ++++++-----
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/testcases/kernel/device-drivers/zram/zram01.sh b/testcases/kernel/device-drivers/zram/zram01.sh
index ef4ff4d75..bb1532c1e 100755
--- a/testcases/kernel/device-drivers/zram/zram01.sh
+++ b/testcases/kernel/device-drivers/zram/zram01.sh
@@ -6,7 +6,7 @@
 # Test creates several zram devices with different filesystems on them.
 # It fills each device with zeros and checks that compression works.
 
-TST_CNT=7
+TST_CNT=6
 TST_TESTFUNC="do_test"
 TST_NEEDS_CMDS="awk bc dd"
 . zram_lib.sh
@@ -120,12 +120,11 @@ do_test()
 {
 	case $1 in
 	 1) zram_max_streams;;
-	 2) zram_compress_alg;;
-	 3) zram_set_disksizes;;
-	 4) zram_set_memlimit;;
-	 5) zram_makefs;;
-	 6) zram_mount;;
-	 7) zram_fill_fs;;
+	 2) zram_set_disksizes;;
+	 3) zram_set_memlimit;;
+	 4) zram_makefs;;
+	 5) zram_mount;;
+	 6) zram_fill_fs;;
 	esac
 }
 
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.0

[LTP] [PATCH 5/5] 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.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 .../kernel/device-drivers/zram/zram01.sh      | 34 +++++++
 .../kernel/device-drivers/zram/zram02.sh      | 58 ++++++++++++
 .../kernel/device-drivers/zram/zram_lib.sh    | 92 -------------------
 3 files changed, 92 insertions(+), 92 deletions(-)

diff --git a/testcases/kernel/device-drivers/zram/zram01.sh b/testcases/kernel/device-drivers/zram/zram01.sh
index bb1532c1e..b2c384547 100755
--- a/testcases/kernel/device-drivers/zram/zram01.sh
+++ b/testcases/kernel/device-drivers/zram/zram01.sh
@@ -78,6 +78,40 @@ 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))
+		[ $i -eq $dev_num ] && break
+	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..803b8dc29 100755
--- a/testcases/kernel/device-drivers/zram/zram02.sh
+++ b/testcases/kernel/device-drivers/zram/zram02.sh
@@ -23,6 +23,64 @@ zram_max_streams="2"
 zram_sizes="107374182400" # 100GB
 zram_mem_limits="1M"
 
+zram_compress_alg()
+{
+	if tst_kvcmp -lt "3.15"; then
+		tst_res TCONF "device attribute comp_algorithm is"\
+			"introduced since kernel v3.15, the running kernel"\
+			"does not support it"
+		return
+	fi
+
+	local i=0
+
+	tst_res TINFO "test that we can set compression algorithm"
+	local algs="$(sed 's/[][]//g' /sys/block/zram0/comp_algorithm)"
+	tst_res TINFO "supported algs: $algs"
+
+	local dev_max=$(($dev_num - 1))
+
+	for i in $(seq 0 $dev_max); do
+		for alg in $algs; do
+			local sys_path="/sys/block/zram${i}/comp_algorithm"
+			echo "$alg" >  $sys_path || \
+				tst_brk TFAIL "can't set '$alg' to $sys_path"
+			tst_res TINFO "$sys_path = '$alg' ($i/$dev_max)"
+		done
+	done
+
+	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"
+}
+
 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 b2dce1d7d..39615180e 100755
--- a/testcases/kernel/device-drivers/zram/zram_lib.sh
+++ b/testcases/kernel/device-drivers/zram/zram_lib.sh
@@ -85,35 +85,6 @@ zram_max_streams()
 	tst_res TPASS "test succeeded"
 }
 
-zram_compress_alg()
-{
-	if tst_kvcmp -lt "3.15"; then
-		tst_res TCONF "device attribute comp_algorithm is"\
-			"introduced since kernel v3.15, the running kernel"\
-			"does not support it"
-		return
-	fi
-
-	local i=0
-
-	tst_res TINFO "test that we can set compression algorithm"
-	local algs="$(sed 's/[][]//g' /sys/block/zram0/comp_algorithm)"
-	tst_res TINFO "supported algs: $algs"
-
-	local dev_max=$(($dev_num - 1))
-
-	for i in $(seq 0 $dev_max); do
-		for alg in $algs; do
-			local sys_path="/sys/block/zram${i}/comp_algorithm"
-			echo "$alg" >  $sys_path || \
-				tst_brk TFAIL "can't set '$alg' to $sys_path"
-			tst_res TINFO "$sys_path = '$alg' ($i/$dev_max)"
-		done
-	done
-
-	tst_res TPASS "test succeeded"
-}
-
 zram_set_disksizes()
 {
 	local i=0
@@ -159,66 +130,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))
-		[ $i -eq $dev_num ] && break
-	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.0

[LTP] [PATCH 1/5] zram: Require root

[Cyril Hrubis]

Hi!
This one is obviously correct.

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

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/5] zram: Calculate dev_num variable

[Cyril Hrubis]

Hi!
Looks good to me.

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

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/5] zram: Require root

[Petr Vorel]

> Hi!
> This one is obviously correct.

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

Thanks! Pushing this obviously correct one :).

Kind regards,
Petr

[LTP] [PATCH 3/5] zram01.sh: Check properly mkfs.* dependencies

[Cyril Hrubis]

Hi!
> +check_fs_support()
> +{
> +	local fs unsupported
> +	local msg="missing kernel support or mkfs for filesystems:"
> +	dev_num=0
> +
> +	for fs in $zram_filesystems; do
> +		if tst_supported_fs $fs 2> /dev/null; then

Why are we silencing the output here?

> +			dev_num=$((dev_num+1))
> +		else
> +			unsupported="$unsupported $fs"
> +		fi
> +	done

Maybe we should put the Btrfs space check here as well, so that all
checks are in a single place.

> +	if [ $dev_num -eq 0 -a "$fallback_filesystem" != "$FS_TYPE" ]; then
> +		if tst_supported_fs $fallback_filesystem 2> /dev/null; then
> +			dev_num=1
> +		fi
> +	fi
> +
> +	if [ $dev_num -eq 0 ]; then
> +		tst_res TINFO "filesystems on the system"
> +		tst_supported_fs > /dev/null

And producing different output here.

> +		msg="$msg $zram_filesystems"
> +		if [ "$fallback_filesystem" != "$FS_TYPE" ]; then
> +			msg="$msg $fallback_filesystem"
> +		fi
> +		tst_brk TCONF "$msg"
> +	fi
> +
> +	[ "$unsupported" ] && tst_res TINFO "$msg$unsupported"

And here.

I guess that we end up with a bit nicer output but the code is much more
complex.


Also I'm confused how this is supposed to work, we do limit the dev_num
but the zram_filesystems is untouched? That isn't going to work right.

I guess the best shoot would be generating the zram_filesystems here on
the fly based on output of the tst_supported_fs.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 3/5] zram01.sh: Check properly mkfs.* dependencies

[Petr Vorel]

Hi Cyril,

> Hi!
> > +check_fs_support()
> > +{
> > +	local fs unsupported
> > +	local msg="missing kernel support or mkfs for filesystems:"
> > +	dev_num=0
> > +
> > +	for fs in $zram_filesystems; do
> > +		if tst_supported_fs $fs 2> /dev/null; then

> Why are we silencing the output here?

> > +			dev_num=$((dev_num+1))
> > +		else
> > +			unsupported="$unsupported $fs"
> > +		fi
> > +	done

> Maybe we should put the Btrfs space check here as well, so that all
> checks are in a single place.

> > +	if [ $dev_num -eq 0 -a "$fallback_filesystem" != "$FS_TYPE" ]; then
> > +		if tst_supported_fs $fallback_filesystem 2> /dev/null; then
> > +			dev_num=1
> > +		fi
> > +	fi
> > +
> > +	if [ $dev_num -eq 0 ]; then
> > +		tst_res TINFO "filesystems on the system"
> > +		tst_supported_fs > /dev/null

> And producing different output here.

> > +		msg="$msg $zram_filesystems"
> > +		if [ "$fallback_filesystem" != "$FS_TYPE" ]; then
> > +			msg="$msg $fallback_filesystem"
> > +		fi
> > +		tst_brk TCONF "$msg"
> > +	fi
> > +
> > +	[ "$unsupported" ] && tst_res TINFO "$msg$unsupported"

> And here.

> I guess that we end up with a bit nicer output but the code is much more
> complex.
You're right.


> Also I'm confused how this is supposed to work, we do limit the dev_num
> but the zram_filesystems is untouched? That isn't going to work right.
It actually works due this check in functions it uses:
[ $i -eq $dev_num ] && break
(maybe more readable to use -ge ).

> I guess the best shoot would be generating the zram_filesystems here on
> the fly based on output of the tst_supported_fs.
Very good idea. I still wasn't happy with the patchset and this was the missing
piece. Also generating other variables. I overlooked that only size for BTRFS is
different. In that case [ $i -eq $dev_num ] && break should not be needed.

I also introduced fallback_filesystem, because I thought that FS_TYPE and FS_SIZE
(as uppercase) are for LTP API. But they're not, that's TST_FS_TYPE (and there
is no *SIZE variable). These (if still used) should be lower case).

Thanks for your review!

Kind regards,
Petr

[LTP] [PATCH 3/5] zram01.sh: Check properly mkfs.* dependencies

[Cyril Hrubis]

Hi!
> > Also I'm confused how this is supposed to work, we do limit the dev_num
> > but the zram_filesystems is untouched? That isn't going to work right.
> It actually works due this check in functions it uses:
> [ $i -eq $dev_num ] && break
> (maybe more readable to use -ge ).

So if we have ext3 unsupported we will loop over ext3, ext4 and xfs
there and break just before btrfs?

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 3/5] zram01.sh: Check properly mkfs.* dependencies

[Petr Vorel]

> Hi!
> > > Also I'm confused how this is supposed to work, we do limit the dev_num
> > > but the zram_filesystems is untouched? That isn't going to work right.
> > It actually works due this check in functions it uses:
> > [ $i -eq $dev_num ] && break
> > (maybe more readable to use -ge ).

> So if we have ext3 unsupported we will loop over ext3, ext4 and xfs
> there and break just before btrfs?
Well, sending rewritten implementation.

Kind regards,
Petr