[LTP] [PATCH 0/5] shell: Add TST_FORMAT_DEVICE support

[LTP] [PATCH 0/5] shell: Add TST_FORMAT_DEVICE support

[Petr Vorel]

Hi,

it's nice to sync with C API, but I don't expect that many tests would
need formatting device.  The main reason is to preparation for
TST_ALL_FILESYSTEMS support (useful for NFS tests).

Kind regards,
Petr

Petr Vorel (5):
  tst_test.sh: Add $TST_DEFAULT_FS_TYPE
  tst_test.sh: tst_mkfs(): Add support for extra opts
  tst_test.sh: Add $TST_FORMAT_DEVICE and related vars
  df01.sh: Use TST_FORMAT_DEVICE=1
  shell: Add test for TST_FORMAT_DEVICE=1

 doc/shell-test-api.txt                      |  9 +++++++-
 lib/newlib_tests/shell/tst_format_device.sh | 24 ++++++++++++++++++++
 testcases/commands/df/df01.sh               |  7 ++----
 testcases/commands/mkfs/mkfs01.sh           |  4 ++--
 testcases/lib/tst_test.sh                   | 25 ++++++++++++---------
 5 files changed, 51 insertions(+), 18 deletions(-)
 create mode 100755 lib/newlib_tests/shell/tst_format_device.sh

-- 
2.34.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 1/5] tst_test.sh: Add $TST_DEFAULT_FS_TYPE

[Petr Vorel]

To sync with C API which also has defined the default filesystem.

That required to whitelist ) added after variable to allow use in help:
(currently $TST_DEFAULT_FS_TYPE).

Use it in df01.sh, mkfs01.sh (the only tests which works with
filesystems).

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/commands/df/df01.sh     |  4 +---
 testcases/commands/mkfs/mkfs01.sh |  4 ++--
 testcases/lib/tst_test.sh         | 11 ++++++-----
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/testcases/commands/df/df01.sh b/testcases/commands/df/df01.sh
index b821452e60..0d09e290ce 100755
--- a/testcases/commands/df/df01.sh
+++ b/testcases/commands/df/df01.sh
@@ -23,13 +23,11 @@ usage: $0 [-f <ext2|ext3|ext4|vfat|...>]
 
 OPTIONS
 -f	Specify the type of filesystem to be built.  If not
-	specified, the default filesystem type (currently ext2)
+	specified, the default filesystem type (currently $TST_DEFAULT_FS_TYPE)
 	is used.
 EOF
 }
 
-TST_FS_TYPE=ext2
-
 parse_args()
 {
 	TST_FS_TYPE="$2"
diff --git a/testcases/commands/mkfs/mkfs01.sh b/testcases/commands/mkfs/mkfs01.sh
index 17c7fb9e4a..e2896858f9 100755
--- a/testcases/commands/mkfs/mkfs01.sh
+++ b/testcases/commands/mkfs/mkfs01.sh
@@ -23,7 +23,7 @@ usage: $0 [-f <ext2|ext3|ext4|vfat|...>]
 
 OPTIONS
 -f	Specify the type of filesystem to be built.  If not
-	specified, the default filesystem type (currently ext2)
+	specified, the default filesystem type (currently $TST_DEFAULT_FS_TYPE)
 	is used.
 EOF
 }
@@ -45,7 +45,7 @@ setup()
 mkfs_verify_type()
 {
 	if [ -z "$1" ]; then
-		blkid $2 -t TYPE="ext2" >/dev/null
+		blkid $2 -t TYPE="$TST_DEFAULT_FS_TYPE" >/dev/null
 	else
 		if [ "$1" = "msdos" ]; then
 			blkid $2 -t TYPE="vfat" >/dev/null
diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index 30614974c3..eb3bf0df12 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -17,6 +17,8 @@ export TST_ITERATIONS=1
 export TST_TMPDIR_RHOST=0
 export TST_LIB_LOADED=1
 
+export TST_DEFAULT_FS_TYPE="ext2"
+
 . tst_ansi_color.sh
 . tst_security.sh
 
@@ -338,15 +340,13 @@ tst_umount()
 tst_mkfs()
 {
 	local fs_type=${1:-$TST_FS_TYPE}
+	[ -z "$fs_type" ] && fs_type="$TST_DEFAULT_FS_TYPE"
+
 	local device=${2:-$TST_DEVICE}
 	[ $# -ge 1 ] && shift
 	[ $# -ge 1 ] && shift
 	local fs_opts="$@"
 
-	if [ -z "$fs_type" ]; then
-		tst_brk TBROK "No fs_type specified"
-	fi
-
 	if [ -z "$device" ]; then
 		tst_brk TBROK "No device specified"
 	fi
@@ -599,7 +599,7 @@ tst_run()
 	local _tst_name
 
 	if [ -n "$TST_TEST_PATH" ]; then
-		for _tst_i in $(grep '^[^#]*\bTST_' "$TST_TEST_PATH" | sed 's/.*TST_//; s/[="} \t\/:`].*//'); do
+		for _tst_i in $(grep '^[^#]*\bTST_' "$TST_TEST_PATH" | sed 's/.*TST_//; s/[="} \t\/:`)].*//'); do
 			case "$_tst_i" in
 			DISABLE_APPARMOR|DISABLE_SELINUX);;
 			SETUP|CLEANUP|TESTFUNC|ID|CNT|MIN_KVER);;
@@ -614,6 +614,7 @@ tst_run()
 			NET_SKIP_VARIABLE_INIT|NEEDS_CHECKPOINTS);;
 			CHECKPOINT_WAIT|CHECKPOINT_WAKE);;
 			CHECKPOINT_WAKE2|CHECKPOINT_WAKE_AND_WAIT);;
+			DEFAULT_FS_TYPE);;
 			*) tst_res TWARN "Reserved variable TST_$_tst_i used!";;
 			esac
 		done
-- 
2.34.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 2/5] tst_test.sh: tst_mkfs(): Add support for extra opts

[Petr Vorel]

Allow to pass options after device name
(to sync with C API).

NOTE: not adding it to legacy shell API (thus no need to change
test_robind.sh).

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/lib/tst_test.sh | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index eb3bf0df12..4707613158 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -343,9 +343,8 @@ tst_mkfs()
 	[ -z "$fs_type" ] && fs_type="$TST_DEFAULT_FS_TYPE"
 
 	local device=${2:-$TST_DEVICE}
-	[ $# -ge 1 ] && shift
-	[ $# -ge 1 ] && shift
-	local fs_opts="$@"
+	local fs_opts="$3"
+	local extra_opts="$4"
 
 	if [ -z "$device" ]; then
 		tst_brk TBROK "No device specified"
@@ -353,8 +352,8 @@ tst_mkfs()
 
 	tst_require_cmds mkfs.$fs_type
 
-	tst_res TINFO "Formatting $device with $fs_type extra opts='$fs_opts'"
-	ROD_SILENT mkfs.$fs_type $fs_opts $device
+	tst_res TINFO "Formatting $device with $fs_type opts='$fs_opts' extra opts='$extra_opts'"
+	ROD_SILENT mkfs.$fs_type $fs_opts $device $extra_opts
 }
 
 # Detect whether running under hypervisor: Microsoft Hyper-V
-- 
2.34.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 3/5] tst_test.sh: Add $TST_FORMAT_DEVICE and related vars

[Petr Vorel]

i.e.: TST_DEV_EXTRA_OPTS, TST_DEV_FS_OPTS, TST_FS_TYPE
(to sync with C API).

Also stop using pattern 'TST_NEEDS_FOO', sometimes 'NEEDS' just does not
fit to variable name.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 doc/shell-test-api.txt    | 9 ++++++++-
 testcases/lib/tst_test.sh | 7 ++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/doc/shell-test-api.txt b/doc/shell-test-api.txt
index e082808f7e..6605c65b47 100644
--- a/doc/shell-test-api.txt
+++ b/doc/shell-test-api.txt
@@ -189,11 +189,18 @@ space as default value is used. Of course, it's possible to use separate functio
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
 Similarily to the C library various checks and preparations can be requested
-simply by setting right '$TST_NEEDS_FOO'.
+simply by setting right '$TST_FOO'.
 
 [options="header"]
 |=============================================================================
 | Variable name            | Action done
+| 'TST_DEV_EXTRA_OPTS'     | Pass additional 'mkfs' options when use 'TST_FORMAT_DEVICE'.
+| 'TST_DEV_FS_OPTS'        | Pass extra 'mkfs' options after the device name
+|                          | when use 'TST_FORMAT_DEVICE'.
+| 'TST_FORMAT_DEVICE'      | Format a block device with a filesystem. See also
+|                          | 'TST_DEV_EXTRA_OPTS', 'TST_DEV_FS_OPTS', 'TST_FS_TYPE'.
+| 'TST_FS_TYPE'            | Overwrite the default filesystem to be used.
+|                          | Implies 'TST_NEEDS_DEVICE=1' (no need to set it).
 | 'TST_NEEDS_ROOT'         | Exit the test with 'TCONF' unless executed under root.
 |                          | Alternatively the 'tst_require_root' command can be used.
 | 'TST_NEEDS_TMPDIR'       | Create test temporary directory and cd into it.
diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index 4707613158..49eee8d00f 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -613,7 +613,7 @@ tst_run()
 			NET_SKIP_VARIABLE_INIT|NEEDS_CHECKPOINTS);;
 			CHECKPOINT_WAIT|CHECKPOINT_WAKE);;
 			CHECKPOINT_WAKE2|CHECKPOINT_WAKE_AND_WAIT);;
-			DEFAULT_FS_TYPE);;
+			DEFAULT_FS_TYPE|DEV_EXTRA_OPTS|DEV_FS_OPTS|FORMAT_DEVICE);;
 			*) tst_res TWARN "Reserved variable TST_$_tst_i used!";;
 			esac
 		done
@@ -658,6 +658,7 @@ tst_run()
 
 	_tst_setup_timer
 
+	[ "$TST_FORMAT_DEVICE" = 1 ] && TST_NEEDS_DEVICE=1
 	[ "$TST_NEEDS_DEVICE" = 1 ] && TST_NEEDS_TMPDIR=1
 
 	if [ "$TST_NEEDS_TMPDIR" = 1 ]; then
@@ -689,6 +690,10 @@ tst_run()
 
 	[ -n "$TST_NEEDS_MODULE" ] && tst_require_module "$TST_NEEDS_MODULE"
 
+	if [ "$TST_FORMAT_DEVICE" = 1 ]; then
+		tst_mkfs "$TST_FS_TYPE" "$TST_DEVICE" "$TST_DEV_FS_OPTS" "$TST_DEV_EXTRA_OPTS"
+	fi
+
 	[ -n "$TST_NEEDS_CHECKPOINTS" ] && _tst_init_checkpoints
 
 	if [ -n "$TST_SETUP" ]; then
-- 
2.34.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 4/5] df01.sh: Use TST_FORMAT_DEVICE=1

[Petr Vorel]

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/commands/df/df01.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/testcases/commands/df/df01.sh b/testcases/commands/df/df01.sh
index 0d09e290ce..b82f396adc 100755
--- a/testcases/commands/df/df01.sh
+++ b/testcases/commands/df/df01.sh
@@ -13,7 +13,7 @@ TST_OPTS="f:"
 TST_USAGE=usage
 TST_PARSE_ARGS=parse_args
 TST_NEEDS_ROOT=1
-TST_NEEDS_DEVICE=1
+TST_FORMAT_DEVICE=1
 . tst_test.sh
 
 usage()
@@ -35,7 +35,6 @@ parse_args()
 
 setup()
 {
-	tst_mkfs
 	tst_mount
 	DF_FS_TYPE=$(mount | grep "$TST_DEVICE" | awk 'NR==1{print $5}')
 }
-- 
2.34.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 5/5] shell: Add test for TST_FORMAT_DEVICE=1

[Petr Vorel]

Unfortunately GitHub Actions don't have loop devices, thus cannot be run
in CI:

tst_format_device 1 TINFO: timeout per run is 0h 5m 0s
/__w/ltp/ltp/lib/tst_device.c:139: TINFO: No free devices found

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 lib/newlib_tests/shell/tst_format_device.sh | 24 +++++++++++++++++++++
 1 file changed, 24 insertions(+)
 create mode 100755 lib/newlib_tests/shell/tst_format_device.sh

diff --git a/lib/newlib_tests/shell/tst_format_device.sh b/lib/newlib_tests/shell/tst_format_device.sh
new file mode 100755
index 0000000000..b02f8b122b
--- /dev/null
+++ b/lib/newlib_tests/shell/tst_format_device.sh
@@ -0,0 +1,24 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (c) 2022 Petr Vorel <pvorel@suse.cz>
+
+TST_FORMAT_DEVICE=1
+TST_TESTFUNC=test
+TST_CNT=2
+TST_DEV_FS_OPTS="-b 1024 -O quota"
+TST_DEV_EXTRA_OPTS="5m"
+TST_NEEDS_CMDS="df lsblk"
+. tst_test.sh
+
+test1()
+{
+	tst_res TPASS "device formatted"
+}
+
+test2()
+{
+	tst_check_cmds df || return
+	EXPECT_PASS "df $TST_DEVICE | grep -q /dev"
+}
+
+tst_run
-- 
2.34.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/5] tst_test.sh: Add $TST_DEFAULT_FS_TYPE

[Cyril Hrubis]

Hi!
> diff --git a/testcases/commands/df/df01.sh b/testcases/commands/df/df01.sh
> index b821452e60..0d09e290ce 100755
> --- a/testcases/commands/df/df01.sh
> +++ b/testcases/commands/df/df01.sh
> @@ -23,13 +23,11 @@ usage: $0 [-f <ext2|ext3|ext4|vfat|...>]
>  
>  OPTIONS
>  -f	Specify the type of filesystem to be built.  If not
> -	specified, the default filesystem type (currently ext2)
> +	specified, the default filesystem type (currently $TST_DEFAULT_FS_TYPE)
>  	is used.
>  EOF
>  }
>  
> -TST_FS_TYPE=ext2
> -
>  parse_args()
>  {
>  	TST_FS_TYPE="$2"
> diff --git a/testcases/commands/mkfs/mkfs01.sh b/testcases/commands/mkfs/mkfs01.sh
> index 17c7fb9e4a..e2896858f9 100755
> --- a/testcases/commands/mkfs/mkfs01.sh
> +++ b/testcases/commands/mkfs/mkfs01.sh
> @@ -23,7 +23,7 @@ usage: $0 [-f <ext2|ext3|ext4|vfat|...>]
>  
>  OPTIONS
>  -f	Specify the type of filesystem to be built.  If not
> -	specified, the default filesystem type (currently ext2)
> +	specified, the default filesystem type (currently $TST_DEFAULT_FS_TYPE)
>  	is used.
>  EOF
>  }
> @@ -45,7 +45,7 @@ setup()
>  mkfs_verify_type()
>  {
>  	if [ -z "$1" ]; then
> -		blkid $2 -t TYPE="ext2" >/dev/null
> +		blkid $2 -t TYPE="$TST_DEFAULT_FS_TYPE" >/dev/null
>  	else
>  		if [ "$1" = "msdos" ]; then
>  			blkid $2 -t TYPE="vfat" >/dev/null
> diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
> index 30614974c3..eb3bf0df12 100644
> --- a/testcases/lib/tst_test.sh
> +++ b/testcases/lib/tst_test.sh
> @@ -17,6 +17,8 @@ export TST_ITERATIONS=1
>  export TST_TMPDIR_RHOST=0
>  export TST_LIB_LOADED=1
>  
> +export TST_DEFAULT_FS_TYPE="ext2"

Actually if we wanted to sync with the C library it should be:

if [ -z "$TST_FS_TYPE" ]; then
	if [ -n "$LTP_DEV_FS_TYPE" ]; then
		export TST_FS_TYPE="$LTP_DEV_FS_TYPE"
	else
		export TST_FS_TYPE="ext2"
	fi
fi

That's equivalent of the .dev_fs_type in the tst_test structure as we
do:

	if (tst_test->dev_fs_type)
		tdev.fs_type = tst_test->dev_fs_type;
	else
		tdev.fs_type = tst_dev_fs_type();

And the tst_dev_fs_type() returns either $LTP_DEV_FS_TYPE or "ext2" if
it's not defined.

>  . tst_ansi_color.sh
>  . tst_security.sh
>  
> @@ -338,15 +340,13 @@ tst_umount()
>  tst_mkfs()
>  {
>  	local fs_type=${1:-$TST_FS_TYPE}
> +	[ -z "$fs_type" ] && fs_type="$TST_DEFAULT_FS_TYPE"
> +
>  	local device=${2:-$TST_DEVICE}
>  	[ $# -ge 1 ] && shift
>  	[ $# -ge 1 ] && shift
>  	local fs_opts="$@"
>  
> -	if [ -z "$fs_type" ]; then
> -		tst_brk TBROK "No fs_type specified"
> -	fi
> -
>  	if [ -z "$device" ]; then
>  		tst_brk TBROK "No device specified"
>  	fi
> @@ -599,7 +599,7 @@ tst_run()
>  	local _tst_name
>  
>  	if [ -n "$TST_TEST_PATH" ]; then
> -		for _tst_i in $(grep '^[^#]*\bTST_' "$TST_TEST_PATH" | sed 's/.*TST_//; s/[="} \t\/:`].*//'); do
> +		for _tst_i in $(grep '^[^#]*\bTST_' "$TST_TEST_PATH" | sed 's/.*TST_//; s/[="} \t\/:`)].*//'); do
>  			case "$_tst_i" in
>  			DISABLE_APPARMOR|DISABLE_SELINUX);;
>  			SETUP|CLEANUP|TESTFUNC|ID|CNT|MIN_KVER);;
> @@ -614,6 +614,7 @@ tst_run()
>  			NET_SKIP_VARIABLE_INIT|NEEDS_CHECKPOINTS);;
>  			CHECKPOINT_WAIT|CHECKPOINT_WAKE);;
>  			CHECKPOINT_WAKE2|CHECKPOINT_WAKE_AND_WAIT);;
> +			DEFAULT_FS_TYPE);;
>  			*) tst_res TWARN "Reserved variable TST_$_tst_i used!";;
>  			esac
>  		done
> -- 
> 2.34.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/5] tst_test.sh: tst_mkfs(): Add support for extra opts

[Cyril Hrubis]

Hi!
> Allow to pass options after device name
> (to sync with C API).
> 
> NOTE: not adding it to legacy shell API (thus no need to change
> test_robind.sh).
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  testcases/lib/tst_test.sh | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
> index eb3bf0df12..4707613158 100644
> --- a/testcases/lib/tst_test.sh
> +++ b/testcases/lib/tst_test.sh
> @@ -343,9 +343,8 @@ tst_mkfs()
>  	[ -z "$fs_type" ] && fs_type="$TST_DEFAULT_FS_TYPE"
>  
>  	local device=${2:-$TST_DEVICE}
> -	[ $# -ge 1 ] && shift
> -	[ $# -ge 1 ] && shift
> -	local fs_opts="$@"
> +	local fs_opts="$3"
> +	local extra_opts="$4"

Doesn't this actually break the examples described in the
doc/shell-test-api.txt?

There we do have:

tst_mkfs ext4 /dev/device -T largefile

Which obviously parses as "-T" in the fs_opts and "largefile" in the
extra_opts.

I do not think that the support for extra opts can be added this way
without redesigning the tst_mkfs API. Maybe it would make more sense to
just pass the parameters in the same way we end up passing them on a
command line, i.e. we would only interpret the first parameter passed to
tst_mkfs as a filesystem type and pass the rest as $@.

>  	if [ -z "$device" ]; then
>  		tst_brk TBROK "No device specified"
> @@ -353,8 +352,8 @@ tst_mkfs()
>  
>  	tst_require_cmds mkfs.$fs_type
>  
> -	tst_res TINFO "Formatting $device with $fs_type extra opts='$fs_opts'"
> -	ROD_SILENT mkfs.$fs_type $fs_opts $device
> +	tst_res TINFO "Formatting $device with $fs_type opts='$fs_opts' extra opts='$extra_opts'"
> +	ROD_SILENT mkfs.$fs_type $fs_opts $device $extra_opts
>  }
>  
>  # Detect whether running under hypervisor: Microsoft Hyper-V
> -- 
> 2.34.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 3/5] tst_test.sh: Add $TST_FORMAT_DEVICE and related vars

[Cyril Hrubis]

On Wed, Jan 26, 2022 at 07:12:08PM +0100, Petr Vorel wrote:
> i.e.: TST_DEV_EXTRA_OPTS, TST_DEV_FS_OPTS, TST_FS_TYPE
> (to sync with C API).
> 
> Also stop using pattern 'TST_NEEDS_FOO', sometimes 'NEEDS' just does not
> fit to variable name.
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  doc/shell-test-api.txt    | 9 ++++++++-
>  testcases/lib/tst_test.sh | 7 ++++++-
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/doc/shell-test-api.txt b/doc/shell-test-api.txt
> index e082808f7e..6605c65b47 100644
> --- a/doc/shell-test-api.txt
> +++ b/doc/shell-test-api.txt
> @@ -189,11 +189,18 @@ space as default value is used. Of course, it's possible to use separate functio
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  
>  Similarily to the C library various checks and preparations can be requested
> -simply by setting right '$TST_NEEDS_FOO'.
> +simply by setting right '$TST_FOO'.
>  
>  [options="header"]
>  |=============================================================================
>  | Variable name            | Action done
> +| 'TST_DEV_EXTRA_OPTS'     | Pass additional 'mkfs' options when use 'TST_FORMAT_DEVICE'.
> +| 'TST_DEV_FS_OPTS'        | Pass extra 'mkfs' options after the device name
> +|                          | when use 'TST_FORMAT_DEVICE'.
> +| 'TST_FORMAT_DEVICE'      | Format a block device with a filesystem. See also
> +|                          | 'TST_DEV_EXTRA_OPTS', 'TST_DEV_FS_OPTS', 'TST_FS_TYPE'.
> +| 'TST_FS_TYPE'            | Overwrite the default filesystem to be used.
                                  ^
                                Override?
> +|                          | Implies 'TST_NEEDS_DEVICE=1' (no need to set it).
>  | 'TST_NEEDS_ROOT'         | Exit the test with 'TCONF' unless executed under root.
>  |                          | Alternatively the 'tst_require_root' command can be used.
>  | 'TST_NEEDS_TMPDIR'       | Create test temporary directory and cd into it.
> diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
> index 4707613158..49eee8d00f 100644
> --- a/testcases/lib/tst_test.sh
> +++ b/testcases/lib/tst_test.sh
> @@ -613,7 +613,7 @@ tst_run()
>  			NET_SKIP_VARIABLE_INIT|NEEDS_CHECKPOINTS);;
>  			CHECKPOINT_WAIT|CHECKPOINT_WAKE);;
>  			CHECKPOINT_WAKE2|CHECKPOINT_WAKE_AND_WAIT);;
> -			DEFAULT_FS_TYPE);;
> +			DEFAULT_FS_TYPE|DEV_EXTRA_OPTS|DEV_FS_OPTS|FORMAT_DEVICE);;
>  			*) tst_res TWARN "Reserved variable TST_$_tst_i used!";;
>  			esac
>  		done
> @@ -658,6 +658,7 @@ tst_run()
>  
>  	_tst_setup_timer
>  
> +	[ "$TST_FORMAT_DEVICE" = 1 ] && TST_NEEDS_DEVICE=1
>  	[ "$TST_NEEDS_DEVICE" = 1 ] && TST_NEEDS_TMPDIR=1
>  
>  	if [ "$TST_NEEDS_TMPDIR" = 1 ]; then
> @@ -689,6 +690,10 @@ tst_run()
>  
>  	[ -n "$TST_NEEDS_MODULE" ] && tst_require_module "$TST_NEEDS_MODULE"
>  
> +	if [ "$TST_FORMAT_DEVICE" = 1 ]; then
> +		tst_mkfs "$TST_FS_TYPE" "$TST_DEVICE" "$TST_DEV_FS_OPTS" "$TST_DEV_EXTRA_OPTS"

This would work with the proposed change to tst_mkfs if we swapped the
$TST_DEV_FS_OPTS with $TST_DEVICE.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 3/5] tst_test.sh: Add $TST_FORMAT_DEVICE and related vars

[Petr Vorel]

Hi Cyril,

> > +++ b/doc/shell-test-api.txt
...
> > +| 'TST_FS_TYPE'            | Overwrite the default filesystem to be used.
>                                   ^
>                                 Override?

+1, thanks!

> > diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
...
> > +	if [ "$TST_FORMAT_DEVICE" = 1 ]; then
> > +		tst_mkfs "$TST_FS_TYPE" "$TST_DEVICE" "$TST_DEV_FS_OPTS" "$TST_DEV_EXTRA_OPTS"

> This would work with the proposed change to tst_mkfs if we swapped the
> $TST_DEV_FS_OPTS with $TST_DEVICE.

Do you mean to swap here? i.e.:
tst_mkfs "$TST_FS_TYPE" "$TST_DEV_FS_OPTS" "$TST_DEVICE" "$TST_DEV_EXTRA_OPTS"

But in tst_mkfs it has always been the second parameter:
local device=${2:-$TST_DEVICE}

What am I missing?

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 3/5] tst_test.sh: Add $TST_FORMAT_DEVICE and related vars

[Cyril Hrubis]

Hi!
> Do you mean to swap here? i.e.:
> tst_mkfs "$TST_FS_TYPE" "$TST_DEV_FS_OPTS" "$TST_DEVICE" "$TST_DEV_EXTRA_OPTS"
> 
> But in tst_mkfs it has always been the second parameter:
> local device=${2:-$TST_DEVICE}
> 
> What am I missing?

The email that explains why we can't add dev_extra_ops to the tst_mkfs
the same way we do in C?

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/5] tst_test.sh: tst_mkfs(): Add support for extra opts

[Petr Vorel]

Hi Cyril,

> > +++ b/testcases/lib/tst_test.sh
> > @@ -343,9 +343,8 @@ tst_mkfs()
> >  	[ -z "$fs_type" ] && fs_type="$TST_DEFAULT_FS_TYPE"

> >  	local device=${2:-$TST_DEVICE}
> > -	[ $# -ge 1 ] && shift
> > -	[ $# -ge 1 ] && shift
> > -	local fs_opts="$@"
> > +	local fs_opts="$3"
> > +	local extra_opts="$4"

> Doesn't this actually break the examples described in the
> doc/shell-test-api.txt?

> There we do have:

> tst_mkfs ext4 /dev/device -T largefile

> Which obviously parses as "-T" in the fs_opts and "largefile" in the
> extra_opts.
Good catch, I forget to check examples in docs.

-T largefile would have to be quoted:
tst_mkfs ext4 /dev/device "-T largefile"


> I do not think that the support for extra opts can be added this way
> without redesigning the tst_mkfs API. Maybe it would make more sense to
> just pass the parameters in the same way we end up passing them on a
> command line, i.e. we would only interpret the first parameter passed to
> tst_mkfs as a filesystem type and pass the rest as $@.

But then you'd have to always pass a device ($TST_DEVICE).
And the problem is that I wanted to specify the options before loading
tst_test.sh (together with TST_FORMAT_DEVICE=1). At this point TST_DEVICE
is empty string. Therefore I'd prefer to quote fs-options to be passed to
tst_mkfs as single option.

OTOH mostly only fs-size is the parameter, not really used now.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/5] tst_test.sh: Add $TST_DEFAULT_FS_TYPE

[Petr Vorel]

Hi Cyril,

> > +export TST_DEFAULT_FS_TYPE="ext2"

> Actually if we wanted to sync with the C library it should be:

> if [ -z "$TST_FS_TYPE" ]; then
> 	if [ -n "$LTP_DEV_FS_TYPE" ]; then
> 		export TST_FS_TYPE="$LTP_DEV_FS_TYPE"
> 	else
> 		export TST_FS_TYPE="ext2"
> 	fi
> fi

Ah, I forget on $LTP_DEV_FS_TYPE, thanks.

nit: or shorter
if [ -z "$TST_FS_TYPE" ]; then
	export TST_FS_TYPE="${LTP_DEV_FS_TYPE:-ext2}"
fi

> That's equivalent of the .dev_fs_type in the tst_test structure as we
> do:

> 	if (tst_test->dev_fs_type)
> 		tdev.fs_type = tst_test->dev_fs_type;
> 	else
> 		tdev.fs_type = tst_dev_fs_type();

> And the tst_dev_fs_type() returns either $LTP_DEV_FS_TYPE or "ext2" if
> it's not defined.


BTW I meant TST_DEFAULT_FS_TYPE="ext2" as equivalent of
#define DEFAULT_FS_TYPE "ext2"
from include/tst_defaults.h.

Because df01.sh help uses hardcoded ext2:

@@ -23,13 +23,11 @@ usage: $0 [-f <ext2|ext3|ext4|vfat|...>]

 OPTIONS
 -f	Specify the type of filesystem to be built.  If not
-	specified, the default filesystem type (currently ext2)
+	specified, the default filesystem type (currently $TST_DEFAULT_FS_TYPE)
 	is used.

Thus it'd be:

export TST_DEFAULT_FS_TYPE="ext2"
if [ -z "$TST_FS_TYPE" ]; then
	export TST_FS_TYPE="${LTP_DEV_FS_TYPE:-$TST_DEFAULT_FS_TYPE}"
fi

But it's very minor, shell is different than C, we don't have to have variable
for everything.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 3/5] tst_test.sh: Add $TST_FORMAT_DEVICE and related vars

[Petr Vorel]

> Hi!
> > Do you mean to swap here? i.e.:
> > tst_mkfs "$TST_FS_TYPE" "$TST_DEV_FS_OPTS" "$TST_DEVICE" "$TST_DEV_EXTRA_OPTS"

> > But in tst_mkfs it has always been the second parameter:
> > local device=${2:-$TST_DEVICE}

> > What am I missing?

> The email that explains why we can't add dev_extra_ops to the tst_mkfs
> the same way we do in C?

Ah, got that, thx :).

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/5] tst_test.sh: tst_mkfs(): Add support for extra opts

[Petr Vorel]

Hi Cyril,

> > > +++ b/testcases/lib/tst_test.sh
> > > @@ -343,9 +343,8 @@ tst_mkfs()
> > >  	[ -z "$fs_type" ] && fs_type="$TST_DEFAULT_FS_TYPE"

> > >  	local device=${2:-$TST_DEVICE}
> > > -	[ $# -ge 1 ] && shift
> > > -	[ $# -ge 1 ] && shift
> > > -	local fs_opts="$@"
> > > +	local fs_opts="$3"
> > > +	local extra_opts="$4"

> > Doesn't this actually break the examples described in the
> > doc/shell-test-api.txt?

> > There we do have:

> > tst_mkfs ext4 /dev/device -T largefile

> > Which obviously parses as "-T" in the fs_opts and "largefile" in the
> > extra_opts.
> Good catch, I forget to check examples in docs.

> -T largefile would have to be quoted:
> tst_mkfs ext4 /dev/device "-T largefile"


> > I do not think that the support for extra opts can be added this way
> > without redesigning the tst_mkfs API. Maybe it would make more sense to
> > just pass the parameters in the same way we end up passing them on a
> > command line, i.e. we would only interpret the first parameter passed to
> > tst_mkfs as a filesystem type and pass the rest as $@.

> But then you'd have to always pass a device ($TST_DEVICE).
> And the problem is that I wanted to specify the options before loading
> tst_test.sh (together with TST_FORMAT_DEVICE=1). At this point TST_DEVICE
> is empty string. Therefore I'd prefer to quote fs-options to be passed to
> tst_mkfs as single option.
OK, now I understand you. It's all about tst_mkfs having $1, shift and rest $@.
And we avoid quoting as you suggested :).

But TST_* variables in stay the same. => I'll implement it in v2.

Thanks!

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/5] tst_test.sh: tst_mkfs(): Add support for extra opts

[Petr Vorel]

> Hi Cyril,

> > > > +++ b/testcases/lib/tst_test.sh
> > > > @@ -343,9 +343,8 @@ tst_mkfs()
> > > >  	[ -z "$fs_type" ] && fs_type="$TST_DEFAULT_FS_TYPE"

> > > >  	local device=${2:-$TST_DEVICE}
> > > > -	[ $# -ge 1 ] && shift
> > > > -	[ $# -ge 1 ] && shift
> > > > -	local fs_opts="$@"
> > > > +	local fs_opts="$3"
> > > > +	local extra_opts="$4"

> > > Doesn't this actually break the examples described in the
> > > doc/shell-test-api.txt?

> > > There we do have:

> > > tst_mkfs ext4 /dev/device -T largefile

> > > Which obviously parses as "-T" in the fs_opts and "largefile" in the
> > > extra_opts.
> > Good catch, I forget to check examples in docs.

> > -T largefile would have to be quoted:
> > tst_mkfs ext4 /dev/device "-T largefile"


> > > I do not think that the support for extra opts can be added this way
> > > without redesigning the tst_mkfs API. Maybe it would make more sense to
> > > just pass the parameters in the same way we end up passing them on a
> > > command line, i.e. we would only interpret the first parameter passed to
> > > tst_mkfs as a filesystem type and pass the rest as $@.

> > But then you'd have to always pass a device ($TST_DEVICE).
> > And the problem is that I wanted to specify the options before loading
> > tst_test.sh (together with TST_FORMAT_DEVICE=1). At this point TST_DEVICE
> > is empty string. Therefore I'd prefer to quote fs-options to be passed to
> > tst_mkfs as single option.
> OK, now I understand you. It's all about tst_mkfs having $1, shift and rest $@.
> And we avoid quoting as you suggested :).

> But TST_* variables in stay the same. => I'll implement it in v2.

Cyril, we forget on think: when passing everything as $@, we just don't know
what is the device.

Thus I'll change
tst_res TINFO "Formatting $device with $fs_type opts='$fs_opts' extra opts='$extra_opts'"
with
tst_res TINFO "Formatting $fs_type with opts='$opts'"

It's just a minor logging problem.
We also loose check for missing device, that's not ideal as it's mandatory, thus
I'll add some check when using the default $TST_DEVICE.

Kind regards,
Petr

> Thanks!

> Kind regards,
> Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/5] tst_test.sh: tst_mkfs(): Add support for extra opts

[Petr Vorel]

Hi,

this commit si going to be replaced by this code:

+++ testcases/lib/tst_test.sh
@@ -342,19 +342,21 @@ tst_umount()
 tst_mkfs()
 {
 	local fs_type=${1:-$TST_FS_TYPE}
-	local device=${2:-$TST_DEVICE}
 	[ $# -ge 1 ] && shift
-	[ $# -ge 1 ] && shift
-	local fs_opts="$@"
 
-	if [ -z "$device" ]; then
-		tst_brk TBROK "No device specified"
+	local opts="$@"
+
+	if [ -z "$opts" ]; then
+		if [ "$TST_NEEDS_DEVICE" != 1 ]; then
+			tst_brk "Using default parameters in tst_mkfs requires TST_NEEDS_DEVICE=1"
+		fi
+		opts="$TST_DEVICE"
 	fi
 
 	tst_require_cmds mkfs.$fs_type
 
-	tst_res TINFO "Formatting $device with $fs_type extra opts='$fs_opts'"
-	ROD_SILENT mkfs.$fs_type $fs_opts $device
+	tst_res TINFO "Formatting $fs_type with opts='$opts'"
+	ROD_SILENT mkfs.$fs_type $opts
 }
 
 # Detect whether running under hypervisor: Microsoft Hyper-V

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp