[RFC PATCH 0/2] Kselftest shell (or even C) API

[RFC PATCH 0/2] Kselftest shell (or even C) API

[pvorel]

Hi,

this is a draft trying to define some API in order to remove some
redundancy from kselftest shell scripts. Existing kselftest.h already
defines some sort of API for C, there is none for shell.

It's just a small example how things could be. Draft, not meant to be
really merged. But instead of defining shell library (with more useful
helpers), I'd rather adopt LTP shell [1] and C [2] API to kselftest.
LTP API [1] is more like a framework, easy to use with a lot of helpers
making tests 1) small, concentrating on the problem itself 2) have
unique output. API is well documented [3] [4], it's creator Cyril Hrubis
made it after years experience of handling (at the time) quite bad
quality LTP code.  Rewriting LTP tests to use this API improved tests a
lot (less buggy, easier to read).

Some examples of advantages of LTP API:
* SAFE_*() macros for C, which handles errors inside a library
* unified messages, unified test status, unified way to exit testing due
missing functionality, at the end of testing there is summary of passed,
failed and skipped tests
* many prepared functionality for both C and shell
* handling threads, parent-child synchronization
* setup and cleanup functions
* "flags" for defining requirements or certain functionality (need root, temporary
directory, ...)
* and many other

kselftest and LTP has a bit different goals and approach. Probably
not all of LTP API is needed atm, but I guess it's at least worth of
thinking to adopt it.

There are of course other options: reinvent a wheel or left kselftest
code in a state it is now (code quality varies, some of the code is
really messy, buggy, not even compile).

[1] https://github.com/linux-test-project/ltp/blob/master/testcases/lib/tst_test.sh
[2] https://github.com/linux-test-project/ltp/tree/master/lib
[3] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#22-writing-a-test-in-c
[4] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#23-writing-a-testcase-in-shell

Petr Vorel (2):
  selftests: Start shell API
  selftest/kexec: Use kselftest shell API

 .../selftests/kexec/kexec_common_lib.sh       | 74 +++++--------------
 .../selftests/kexec/test_kexec_file_load.sh   | 53 ++++++-------
 .../selftests/kexec/test_kexec_load.sh        | 20 ++---
 tools/testing/selftests/kselftest.sh          | 53 +++++++++++++
 4 files changed, 105 insertions(+), 95 deletions(-)
 mode change 100755 => 100644 tools/testing/selftests/kexec/kexec_common_lib.sh
 create mode 100644 tools/testing/selftests/kselftest.sh

-- 
2.20.1

[RFC PATCH 0/2] Kselftest shell (or even C) API

[Petr Vorel]

Hi,

this is a draft trying to define some API in order to remove some
redundancy from kselftest shell scripts. Existing kselftest.h already
defines some sort of API for C, there is none for shell.

It's just a small example how things could be. Draft, not meant to be
really merged. But instead of defining shell library (with more useful
helpers), I'd rather adopt LTP shell [1] and C [2] API to kselftest.
LTP API [1] is more like a framework, easy to use with a lot of helpers
making tests 1) small, concentrating on the problem itself 2) have
unique output. API is well documented [3] [4], it's creator Cyril Hrubis
made it after years experience of handling (at the time) quite bad
quality LTP code.  Rewriting LTP tests to use this API improved tests a
lot (less buggy, easier to read).

Some examples of advantages of LTP API:
* SAFE_*() macros for C, which handles errors inside a library
* unified messages, unified test status, unified way to exit testing due
missing functionality, at the end of testing there is summary of passed,
failed and skipped tests
* many prepared functionality for both C and shell
* handling threads, parent-child synchronization
* setup and cleanup functions
* "flags" for defining requirements or certain functionality (need root, temporary
directory, ...)
* and many other

kselftest and LTP has a bit different goals and approach. Probably
not all of LTP API is needed atm, but I guess it's at least worth of
thinking to adopt it.

There are of course other options: reinvent a wheel or left kselftest
code in a state it is now (code quality varies, some of the code is
really messy, buggy, not even compile).

[1] https://github.com/linux-test-project/ltp/blob/master/testcases/lib/tst_test.sh
[2] https://github.com/linux-test-project/ltp/tree/master/lib
[3] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#22-writing-a-test-in-c
[4] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#23-writing-a-testcase-in-shell

Petr Vorel (2):
  selftests: Start shell API
  selftest/kexec: Use kselftest shell API

 .../selftests/kexec/kexec_common_lib.sh       | 74 +++++--------------
 .../selftests/kexec/test_kexec_file_load.sh   | 53 ++++++-------
 .../selftests/kexec/test_kexec_load.sh        | 20 ++---
 tools/testing/selftests/kselftest.sh          | 53 +++++++++++++
 4 files changed, 105 insertions(+), 95 deletions(-)
 mode change 100755 => 100644 tools/testing/selftests/kexec/kexec_common_lib.sh
 create mode 100644 tools/testing/selftests/kselftest.sh

-- 
2.20.1

[RFC PATCH 1/2] selftests: Start shell API

[pvorel]

kselftest.sh is a beginning of shell API.
ATM it's a stub (target could be as rich as LTP API), containing only:
* exit codes
* filling TEST variable
* logging functions
* requiring root function
* add script directory into PATH

Inspired by kexec functions (with some cleanup)
and LTP.

Signed-off-by: Petr Vorel <pvorel at suse.cz>
---
 tools/testing/selftests/kselftest.sh | 53 ++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 tools/testing/selftests/kselftest.sh

diff --git a/tools/testing/selftests/kselftest.sh b/tools/testing/selftests/kselftest.sh
new file mode 100644
index 000000000000..519ec2707dd8
--- /dev/null
+++ b/tools/testing/selftests/kselftest.sh
@@ -0,0 +1,53 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2019 Petr Vorel <pvorel at suse.cz>
+
+PATH="$(dirname $0):$PATH"
+
+KSFT_PASS=0
+KSFT_FAIL=1
+KSFT_XFAIL=2
+KSFT_XPASS=3
+KSFT_SKIP=4
+
+TEST=$(basename $0)
+
+ksft_info()
+{
+	echo "[INFO] $TEST: $1"
+}
+
+ksft_pass()
+{
+	echo "[PASS] $TEST: $1"
+	exit $KSFT_PASS
+}
+
+ksft_fail()
+{
+	echo "[FAIL] $TEST: $1"
+	exit $KSFT_FAIL
+}
+
+ksft_xfail()
+{
+	echo "[FAIL] $TEST: $1"
+	exit $KSFT_XFAIL
+}
+
+ksft_xpass()
+{
+	echo "[PASS] $TEST: $1"
+	exit $KSFT_XPASS
+}
+
+ksft_skip()
+{
+	echo "[SKIP] $TEST: $1"
+	exit $KSFT_SKIP
+}
+
+ksft_require_root()
+{
+	[ $(id -ru) -eq 0 ] || ksft_skip "requires root privileges"
+}
-- 
2.20.1

[RFC PATCH 1/2] selftests: Start shell API

[Petr Vorel]

kselftest.sh is a beginning of shell API.
ATM it's a stub (target could be as rich as LTP API), containing only:
* exit codes
* filling TEST variable
* logging functions
* requiring root function
* add script directory into PATH

Inspired by kexec functions (with some cleanup)
and LTP.

Signed-off-by: Petr Vorel <pvorel at suse.cz>
---
 tools/testing/selftests/kselftest.sh | 53 ++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 tools/testing/selftests/kselftest.sh

diff --git a/tools/testing/selftests/kselftest.sh b/tools/testing/selftests/kselftest.sh
new file mode 100644
index 000000000000..519ec2707dd8
--- /dev/null
+++ b/tools/testing/selftests/kselftest.sh
@@ -0,0 +1,53 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2019 Petr Vorel <pvorel at suse.cz>
+
+PATH="$(dirname $0):$PATH"
+
+KSFT_PASS=0
+KSFT_FAIL=1
+KSFT_XFAIL=2
+KSFT_XPASS=3
+KSFT_SKIP=4
+
+TEST=$(basename $0)
+
+ksft_info()
+{
+	echo "[INFO] $TEST: $1"
+}
+
+ksft_pass()
+{
+	echo "[PASS] $TEST: $1"
+	exit $KSFT_PASS
+}
+
+ksft_fail()
+{
+	echo "[FAIL] $TEST: $1"
+	exit $KSFT_FAIL
+}
+
+ksft_xfail()
+{
+	echo "[FAIL] $TEST: $1"
+	exit $KSFT_XFAIL
+}
+
+ksft_xpass()
+{
+	echo "[PASS] $TEST: $1"
+	exit $KSFT_XPASS
+}
+
+ksft_skip()
+{
+	echo "[SKIP] $TEST: $1"
+	exit $KSFT_SKIP
+}
+
+ksft_require_root()
+{
+	[ $(id -ru) -eq 0 ] || ksft_skip "requires root privileges"
+}
-- 
2.20.1

[RFC PATCH 2/2] selftest/kexec: Use kselftest shell API

[pvorel]

using kselftest.sh helpers + minor not related changes in kexec
(i.e. remove executable bit from kexec library as not needed
for library).

Signed-off-by: Petr Vorel <pvorel at suse.cz>
---
Why removed VERBOSE: I don't know, if someone really needs tests to be
quiet, he can just redirect to /dev/null.
---
 .../selftests/kexec/kexec_common_lib.sh       | 74 +++++--------------
 .../selftests/kexec/test_kexec_file_load.sh   | 53 ++++++-------
 .../selftests/kexec/test_kexec_load.sh        | 20 ++---
 3 files changed, 52 insertions(+), 95 deletions(-)
 mode change 100755 => 100644 tools/testing/selftests/kexec/kexec_common_lib.sh

diff --git a/tools/testing/selftests/kexec/kexec_common_lib.sh b/tools/testing/selftests/kexec/kexec_common_lib.sh
old mode 100755
new mode 100644
index 43017cfe88f7..08ecfe62c9fc
--- a/tools/testing/selftests/kexec/kexec_common_lib.sh
+++ b/tools/testing/selftests/kexec/kexec_common_lib.sh
@@ -1,39 +1,13 @@
 #!/bin/sh
 # SPDX-License-Identifier: GPL-2.0
-#
-# Kselftest framework defines: ksft_pass=0, ksft_fail=1, ksft_skip=4
+
+. $(dirname $0)/../kselftest.sh
 
 VERBOSE="${VERBOSE:-1}"
 IKCONFIG="/tmp/config-`uname -r`"
 KERNEL_IMAGE="/boot/vmlinuz-`uname -r`"
 SECURITYFS=$(grep "securityfs" /proc/mounts | awk '{print $2}')
 
-log_info()
-{
-	[ $VERBOSE -ne 0 ] && echo "[INFO] $1"
-}
-
-# The ksefltest framework requirement returns 0 for PASS.
-log_pass()
-{
-	[ $VERBOSE -ne 0 ] && echo "$1 [PASS]"
-	exit 0
-}
-
-# The ksefltest framework requirement returns 1 for FAIL.
-log_fail()
-{
-	[ $VERBOSE -ne 0 ] && echo "$1 [FAIL]"
-	exit 1
-}
-
-# The ksefltest framework requirement returns 4 for SKIP.
-log_skip()
-{
-	[ $VERBOSE -ne 0 ] && echo "$1"
-	exit 4
-}
-
 # Check efivar SecureBoot-$(the UUID) and SetupMode-$(the UUID).
 # (Based on kdump-lib.sh)
 get_efivarfs_secureboot_mode()
@@ -46,8 +20,8 @@ get_efivarfs_secureboot_mode()
 
 	# Make sure that efivar_fs is mounted in the normal location
 	if ! grep -q "^\S\+ $efivarfs efivarfs" /proc/mounts; then
-		log_info "efivars is not mounted on $efivarfs"
-		return 0;
+		ksft_info "efivars is not mounted on $efivarfs"
+		return 0
 	fi
 	secure_boot_file=$(find "$efivarfs" -name SecureBoot-* 2>/dev/null)
 	setup_mode_file=$(find "$efivarfs" -name SetupMode-* 2>/dev/null)
@@ -58,11 +32,11 @@ get_efivarfs_secureboot_mode()
 			"$setup_mode_file"|cut -d' ' -f 5)
 
 		if [ $secureboot_mode -eq 1 ] && [ $setup_mode -eq 0 ]; then
-			log_info "secure boot mode enabled (CONFIG_EFIVAR_FS)"
-			return 1;
+			ksft_info "secure boot mode enabled (CONFIG_EFIVAR_FS)"
+			return 1
 		fi
 	fi
-	return 0;
+	return 0
 }
 
 get_efi_var_secureboot_mode()
@@ -73,9 +47,8 @@ get_efi_var_secureboot_mode()
 	local secureboot_mode
 	local setup_mode
 
-	if [ ! -d "$efi_vars" ]; then
-		log_skip "efi_vars is not enabled\n"
-	fi
+	[ -d "$efi_vars" ] || ksft_skip "efi_vars is not enabled"
+
 	secure_boot_file=$(find "$efi_vars" -name SecureBoot-* 2>/dev/null)
 	setup_mode_file=$(find "$efi_vars" -name SetupMode-* 2>/dev/null)
 	if [ -f "$secure_boot_file/data" ] && \
@@ -84,11 +57,11 @@ get_efi_var_secureboot_mode()
 		setup_mode=`od -An -t u1 "$setup_mode_file/data"`
 
 		if [ $secureboot_mode -eq 1 ] && [ $setup_mode -eq 0 ]; then
-			log_info "secure boot mode enabled (CONFIG_EFI_VARS)"
-			return 1;
+			ksft_info "secure boot mode enabled (CONFIG_EFI_VARS)"
+			return 1
 		fi
 	fi
-	return 0;
+	return 0
 }
 
 # Check efivar SecureBoot-$(the UUID) and SetupMode-$(the UUID).
@@ -111,16 +84,9 @@ get_secureboot_mode()
 	fi
 
 	if [ $secureboot_mode -eq 0 ]; then
-		log_info "secure boot mode not enabled"
-	fi
-	return $secureboot_mode;
-}
-
-require_root_privileges()
-{
-	if [ $(id -ru) -ne 0 ]; then
-		log_skip "requires root privileges"
+		ksft_info "secure boot mode not enabled"
 	fi
+	return $secureboot_mode
 }
 
 # Look for config option in Kconfig file.
@@ -132,7 +98,7 @@ kconfig_enabled()
 
 	grep -E -q $config $IKCONFIG
 	if [ $? -eq 0 ]; then
-		log_info "$msg"
+		ksft_info "$msg"
 		return 1
 	fi
 	return 0
@@ -160,17 +126,17 @@ get_kconfig()
 
 	local extract_ikconfig="$module_dir/source/scripts/extract-ikconfig"
 	if [ ! -f $extract_ikconfig ]; then
-		log_skip "extract-ikconfig not found"
+		ksft_skip "extract-ikconfig not found"
 	fi
 
 	$extract_ikconfig $KERNEL_IMAGE > $IKCONFIG 2>/dev/null
 	if [ $? -eq 1 ]; then
 		if [ ! -f $configs_module ]; then
-			log_skip "CONFIG_IKCONFIG not enabled"
+			ksft_skip "CONFIG_IKCONFIG not enabled"
 		fi
 		$extract_ikconfig $configs_module > $IKCONFIG
 		if [ $? -eq 1 ]; then
-			log_skip "CONFIG_IKCONFIG not enabled"
+			ksft_skip "CONFIG_IKCONFIG not enabled"
 		fi
 	fi
 	return 1
@@ -185,7 +151,7 @@ mount_securityfs()
 	fi
 
 	if [ ! -d "$SECURITYFS" ]; then
-		log_fail "$SECURITYFS :securityfs is not mounted"
+		ksft_fail "$SECURITYFS :securityfs is not mounted"
 	fi
 }
 
@@ -204,7 +170,7 @@ check_ima_policy()
 
 	local ima_policy=$SECURITYFS/ima/policy
 	if [ ! -e $ima_policy ]; then
-		log_fail "$ima_policy not found"
+		ksft_fail "$ima_policy not found"
 	fi
 
 	if [ -n $keypair2 ]; then
diff --git a/tools/testing/selftests/kexec/test_kexec_file_load.sh b/tools/testing/selftests/kexec/test_kexec_file_load.sh
index fa7c24e8eefb..7e41dd4a5a63 100755
--- a/tools/testing/selftests/kexec/test_kexec_file_load.sh
+++ b/tools/testing/selftests/kexec/test_kexec_file_load.sh
@@ -10,8 +10,7 @@
 # built with CONFIG_IKCONFIG enabled and either CONFIG_IKCONFIG_PROC
 # enabled or access to the extract-ikconfig script.
 
-TEST="KEXEC_FILE_LOAD"
-. ./kexec_common_lib.sh
+. $(dirname $0)/kexec_common_lib.sh
 
 trap "{ rm -f $IKCONFIG ; }" EXIT
 
@@ -28,7 +27,7 @@ is_ima_sig_required()
 	kconfig_enabled "CONFIG_IMA_APPRAISE_REQUIRE_KEXEC_SIGS=y" \
 		"IMA kernel image signature required"
 	if [ $? -eq 1 ]; then
-		log_info "IMA signature required"
+		ksft_info "IMA signature required"
 		return 1
 	fi
 
@@ -41,7 +40,7 @@ is_ima_sig_required()
 		check_ima_policy "appraise" "func=KEXEC_KERNEL_CHECK" \
 			"appraise_type=imasig"
 		ret=$?
-		[ $ret -eq 1 ] && log_info "IMA signature required";
+		[ $ret -eq 1 ] && ksft_info "IMA signature required"
 	fi
 	return $ret
 }
@@ -50,14 +49,14 @@ is_ima_sig_required()
 # Return 1 for PE signature found and 0 for not found.
 check_for_pesig()
 {
-	which pesign > /dev/null 2>&1 || log_skip "pesign not found"
+	which pesign > /dev/null 2>&1 || ksft_skip "pesign not found"
 
 	pesign -i $KERNEL_IMAGE --show-signature | grep -q "No signatures"
 	local ret=$?
 	if [ $ret -eq 1 ]; then
-		log_info "kexec kernel image PE signed"
+		ksft_info "kexec kernel image PE signed"
 	else
-		log_info "kexec kernel image not PE signed"
+		ksft_info "kexec kernel image not PE signed"
 	fi
 	return $ret
 }
@@ -70,16 +69,16 @@ check_for_imasig()
 
 	which getfattr > /dev/null 2>&1
 	if [ $?	-eq 1 ]; then
-		log_skip "getfattr not found"
+		ksft_skip "getfattr not found"
 	fi
 
 	line=$(getfattr -n security.ima -e hex --absolute-names $KERNEL_IMAGE 2>&1)
 	echo $line | grep -q "security.ima=0x03"
 	if [ $? -eq 0 ]; then
 		ret=1
-		log_info "kexec kernel image IMA signed"
+		ksft_info "kexec kernel image IMA signed"
 	else
-		log_info "kexec kernel image not IMA signed"
+		ksft_info "kexec kernel image not IMA signed"
 	fi
 	return $ret
 }
@@ -99,73 +98,69 @@ kexec_file_load_test()
 		# policy, make sure either an IMA or PE signature exists.
 		if [ $secureboot -eq 1 ] && [ $arch_policy -eq 1 ] && \
 			[ $ima_signed -eq 0 ] && [ $pe_signed -eq 0 ]; then
-			log_fail "$succeed_msg (missing sig)"
+			ksft_fail "$succeed_msg (missing sig)"
 		fi
 
 		if [ $kexec_sig_required -eq 1 -o $pe_sig_required -eq 1 ] \
 		     && [ $pe_signed -eq 0 ]; then
-			log_fail "$succeed_msg (missing PE sig)"
+			ksft_fail "$succeed_msg (missing PE sig)"
 		fi
 
 		if [ $ima_sig_required -eq 1 ] && [ $ima_signed -eq 0 ]; then
-			log_fail "$succeed_msg (missing IMA sig)"
+			ksft_fail "$succeed_msg (missing IMA sig)"
 		fi
 
 		if [ $pe_sig_required -eq 0 ] && [ $ima_appraise -eq 1 ] \
 		    && [ $ima_sig_required -eq 0 ] && [ $ima_signed -eq 0 ] \
 	            && [ $ima_read_policy -eq 0 ]; then
-			log_fail "$succeed_msg (possibly missing IMA sig)"
+			ksft_fail "$succeed_msg (possibly missing IMA sig)"
 		fi
 
 		if [ $pe_sig_required -eq 0 ] && [ $ima_appraise -eq 0 ]; then
-			log_info "No signature verification required"
+			ksft_info "No signature verification required"
 		elif [ $pe_sig_required -eq 0 ] && [ $ima_appraise -eq 1 ] \
 		    && [ $ima_sig_required -eq 0 ] && [ $ima_signed -eq 0 ] \
 	            && [ $ima_read_policy -eq 1 ]; then
-			log_info "No signature verification required"
+			ksft_info "No signature verification required"
 		fi
 
-		log_pass "$succeed_msg"
+		ksft_pass "$succeed_msg"
 	fi
 
 	# Check the reason for the kexec_file_load failure
 	echo $line | grep -q "Required key not available"
 	if [ $? -eq 0 ]; then
 		if [ $platform_keyring -eq 0 ]; then
-			log_pass "$failed_msg (-ENOKEY), $key_msg"
+			ksft_pass "$failed_msg (-ENOKEY), $key_msg"
 		else
-			log_pass "$failed_msg (-ENOKEY)"
+			ksft_pass "$failed_msg (-ENOKEY)"
 		fi
 	fi
 
 	if [ $kexec_sig_required -eq 1 -o $pe_sig_required -eq 1 ] \
 	     && [ $pe_signed -eq 0 ]; then
-		log_pass "$failed_msg (missing PE sig)"
+		ksft_pass "$failed_msg (missing PE sig)"
 	fi
 
 	if [ $ima_sig_required -eq 1 ] && [ $ima_signed -eq 0 ]; then
-		log_pass "$failed_msg (missing IMA sig)"
+		ksft_pass "$failed_msg (missing IMA sig)"
 	fi
 
 	if [ $pe_sig_required -eq 0 ] && [ $ima_appraise -eq 1 ] \
 	    && [ $ima_sig_required -eq 0 ] && [ $ima_read_policy -eq 0 ] \
 	    && [ $ima_signed -eq 0 ]; then
-		log_pass "$failed_msg (possibly missing IMA sig)"
+		ksft_pass "$failed_msg (possibly missing IMA sig)"
 	fi
 
-	log_pass "$failed_msg"
-	return 0
+	ksft_pass "$failed_msg"
 }
 
-# kexec requires root privileges
-require_root_privileges
-
-# get the kernel config
+ksft_require_root
 get_kconfig
 
 kconfig_enabled "CONFIG_KEXEC_FILE=y" "kexec_file_load is enabled"
 if [ $? -eq 0 ]; then
-	log_skip "kexec_file_load is not enabled"
+	ksft_skip "kexec_file_load is not enabled"
 fi
 
 # Determine which kernel config options are enabled
diff --git a/tools/testing/selftests/kexec/test_kexec_load.sh b/tools/testing/selftests/kexec/test_kexec_load.sh
index 49c6aa929137..c5d4eaff74c9 100755
--- a/tools/testing/selftests/kexec/test_kexec_load.sh
+++ b/tools/testing/selftests/kexec/test_kexec_load.sh
@@ -4,18 +4,14 @@
 # Prevent loading a kernel image via the kexec_load syscall when
 # signatures are required.  (Dependent on CONFIG_IMA_ARCH_POLICY.)
 
-TEST="$0"
-. ./kexec_common_lib.sh
+. $(dirname $0)/kexec_common_lib.sh
 
-# kexec requires root privileges
-require_root_privileges
-
-# get the kernel config
+ksft_require_root
 get_kconfig
 
 kconfig_enabled "CONFIG_KEXEC=y" "kexec_load is enabled"
 if [ $? -eq 0 ]; then
-	log_skip "kexec_load is not enabled"
+	ksft_skip "kexec_load is not enabled"
 fi
 
 kconfig_enabled "CONFIG_IMA_APPRAISE=y" "IMA enabled"
@@ -33,15 +29,15 @@ kexec --load $KERNEL_IMAGE > /dev/null 2>&1
 if [ $? -eq 0 ]; then
 	kexec --unload
 	if [ $secureboot -eq 1 ] && [ $arch_policy -eq 1 ]; then
-		log_fail "kexec_load succeeded"
+		ksft_fail "kexec_load succeeded"
 	elif [ $ima_appraise -eq 0 -o $arch_policy -eq 0 ]; then
-		log_info "Either IMA or the IMA arch policy is not enabled"
+		ksft_info "Either IMA or the IMA arch policy is not enabled"
 	fi
-	log_pass "kexec_load succeeded"
+	ksft_pass "kexec_load succeeded"
 else
 	if [ $secureboot -eq 1 ] && [ $arch_policy -eq 1 ] ; then
-		log_pass "kexec_load failed"
+		ksft_pass "kexec_load failed"
 	else
-		log_fail "kexec_load failed"
+		ksft_fail "kexec_load failed"
 	fi
 fi
-- 
2.20.1

[RFC PATCH 2/2] selftest/kexec: Use kselftest shell API

[Petr Vorel]

using kselftest.sh helpers + minor not related changes in kexec
(i.e. remove executable bit from kexec library as not needed
for library).

Signed-off-by: Petr Vorel <pvorel at suse.cz>
---
Why removed VERBOSE: I don't know, if someone really needs tests to be
quiet, he can just redirect to /dev/null.
---
 .../selftests/kexec/kexec_common_lib.sh       | 74 +++++--------------
 .../selftests/kexec/test_kexec_file_load.sh   | 53 ++++++-------
 .../selftests/kexec/test_kexec_load.sh        | 20 ++---
 3 files changed, 52 insertions(+), 95 deletions(-)
 mode change 100755 => 100644 tools/testing/selftests/kexec/kexec_common_lib.sh

diff --git a/tools/testing/selftests/kexec/kexec_common_lib.sh b/tools/testing/selftests/kexec/kexec_common_lib.sh
old mode 100755
new mode 100644
index 43017cfe88f7..08ecfe62c9fc
--- a/tools/testing/selftests/kexec/kexec_common_lib.sh
+++ b/tools/testing/selftests/kexec/kexec_common_lib.sh
@@ -1,39 +1,13 @@
 #!/bin/sh
 # SPDX-License-Identifier: GPL-2.0
-#
-# Kselftest framework defines: ksft_pass=0, ksft_fail=1, ksft_skip=4
+
+. $(dirname $0)/../kselftest.sh
 
 VERBOSE="${VERBOSE:-1}"
 IKCONFIG="/tmp/config-`uname -r`"
 KERNEL_IMAGE="/boot/vmlinuz-`uname -r`"
 SECURITYFS=$(grep "securityfs" /proc/mounts | awk '{print $2}')
 
-log_info()
-{
-	[ $VERBOSE -ne 0 ] && echo "[INFO] $1"
-}
-
-# The ksefltest framework requirement returns 0 for PASS.
-log_pass()
-{
-	[ $VERBOSE -ne 0 ] && echo "$1 [PASS]"
-	exit 0
-}
-
-# The ksefltest framework requirement returns 1 for FAIL.
-log_fail()
-{
-	[ $VERBOSE -ne 0 ] && echo "$1 [FAIL]"
-	exit 1
-}
-
-# The ksefltest framework requirement returns 4 for SKIP.
-log_skip()
-{
-	[ $VERBOSE -ne 0 ] && echo "$1"
-	exit 4
-}
-
 # Check efivar SecureBoot-$(the UUID) and SetupMode-$(the UUID).
 # (Based on kdump-lib.sh)
 get_efivarfs_secureboot_mode()
@@ -46,8 +20,8 @@ get_efivarfs_secureboot_mode()
 
 	# Make sure that efivar_fs is mounted in the normal location
 	if ! grep -q "^\S\+ $efivarfs efivarfs" /proc/mounts; then
-		log_info "efivars is not mounted on $efivarfs"
-		return 0;
+		ksft_info "efivars is not mounted on $efivarfs"
+		return 0
 	fi
 	secure_boot_file=$(find "$efivarfs" -name SecureBoot-* 2>/dev/null)
 	setup_mode_file=$(find "$efivarfs" -name SetupMode-* 2>/dev/null)
@@ -58,11 +32,11 @@ get_efivarfs_secureboot_mode()
 			"$setup_mode_file"|cut -d' ' -f 5)
 
 		if [ $secureboot_mode -eq 1 ] && [ $setup_mode -eq 0 ]; then
-			log_info "secure boot mode enabled (CONFIG_EFIVAR_FS)"
-			return 1;
+			ksft_info "secure boot mode enabled (CONFIG_EFIVAR_FS)"
+			return 1
 		fi
 	fi
-	return 0;
+	return 0
 }
 
 get_efi_var_secureboot_mode()
@@ -73,9 +47,8 @@ get_efi_var_secureboot_mode()
 	local secureboot_mode
 	local setup_mode
 
-	if [ ! -d "$efi_vars" ]; then
-		log_skip "efi_vars is not enabled\n"
-	fi
+	[ -d "$efi_vars" ] || ksft_skip "efi_vars is not enabled"
+
 	secure_boot_file=$(find "$efi_vars" -name SecureBoot-* 2>/dev/null)
 	setup_mode_file=$(find "$efi_vars" -name SetupMode-* 2>/dev/null)
 	if [ -f "$secure_boot_file/data" ] && \
@@ -84,11 +57,11 @@ get_efi_var_secureboot_mode()
 		setup_mode=`od -An -t u1 "$setup_mode_file/data"`
 
 		if [ $secureboot_mode -eq 1 ] && [ $setup_mode -eq 0 ]; then
-			log_info "secure boot mode enabled (CONFIG_EFI_VARS)"
-			return 1;
+			ksft_info "secure boot mode enabled (CONFIG_EFI_VARS)"
+			return 1
 		fi
 	fi
-	return 0;
+	return 0
 }
 
 # Check efivar SecureBoot-$(the UUID) and SetupMode-$(the UUID).
@@ -111,16 +84,9 @@ get_secureboot_mode()
 	fi
 
 	if [ $secureboot_mode -eq 0 ]; then
-		log_info "secure boot mode not enabled"
-	fi
-	return $secureboot_mode;
-}
-
-require_root_privileges()
-{
-	if [ $(id -ru) -ne 0 ]; then
-		log_skip "requires root privileges"
+		ksft_info "secure boot mode not enabled"
 	fi
+	return $secureboot_mode
 }
 
 # Look for config option in Kconfig file.
@@ -132,7 +98,7 @@ kconfig_enabled()
 
 	grep -E -q $config $IKCONFIG
 	if [ $? -eq 0 ]; then
-		log_info "$msg"
+		ksft_info "$msg"
 		return 1
 	fi
 	return 0
@@ -160,17 +126,17 @@ get_kconfig()
 
 	local extract_ikconfig="$module_dir/source/scripts/extract-ikconfig"
 	if [ ! -f $extract_ikconfig ]; then
-		log_skip "extract-ikconfig not found"
+		ksft_skip "extract-ikconfig not found"
 	fi
 
 	$extract_ikconfig $KERNEL_IMAGE > $IKCONFIG 2>/dev/null
 	if [ $? -eq 1 ]; then
 		if [ ! -f $configs_module ]; then
-			log_skip "CONFIG_IKCONFIG not enabled"
+			ksft_skip "CONFIG_IKCONFIG not enabled"
 		fi
 		$extract_ikconfig $configs_module > $IKCONFIG
 		if [ $? -eq 1 ]; then
-			log_skip "CONFIG_IKCONFIG not enabled"
+			ksft_skip "CONFIG_IKCONFIG not enabled"
 		fi
 	fi
 	return 1
@@ -185,7 +151,7 @@ mount_securityfs()
 	fi
 
 	if [ ! -d "$SECURITYFS" ]; then
-		log_fail "$SECURITYFS :securityfs is not mounted"
+		ksft_fail "$SECURITYFS :securityfs is not mounted"
 	fi
 }
 
@@ -204,7 +170,7 @@ check_ima_policy()
 
 	local ima_policy=$SECURITYFS/ima/policy
 	if [ ! -e $ima_policy ]; then
-		log_fail "$ima_policy not found"
+		ksft_fail "$ima_policy not found"
 	fi
 
 	if [ -n $keypair2 ]; then
diff --git a/tools/testing/selftests/kexec/test_kexec_file_load.sh b/tools/testing/selftests/kexec/test_kexec_file_load.sh
index fa7c24e8eefb..7e41dd4a5a63 100755
--- a/tools/testing/selftests/kexec/test_kexec_file_load.sh
+++ b/tools/testing/selftests/kexec/test_kexec_file_load.sh
@@ -10,8 +10,7 @@
 # built with CONFIG_IKCONFIG enabled and either CONFIG_IKCONFIG_PROC
 # enabled or access to the extract-ikconfig script.
 
-TEST="KEXEC_FILE_LOAD"
-. ./kexec_common_lib.sh
+. $(dirname $0)/kexec_common_lib.sh
 
 trap "{ rm -f $IKCONFIG ; }" EXIT
 
@@ -28,7 +27,7 @@ is_ima_sig_required()
 	kconfig_enabled "CONFIG_IMA_APPRAISE_REQUIRE_KEXEC_SIGS=y" \
 		"IMA kernel image signature required"
 	if [ $? -eq 1 ]; then
-		log_info "IMA signature required"
+		ksft_info "IMA signature required"
 		return 1
 	fi
 
@@ -41,7 +40,7 @@ is_ima_sig_required()
 		check_ima_policy "appraise" "func=KEXEC_KERNEL_CHECK" \
 			"appraise_type=imasig"
 		ret=$?
-		[ $ret -eq 1 ] && log_info "IMA signature required";
+		[ $ret -eq 1 ] && ksft_info "IMA signature required"
 	fi
 	return $ret
 }
@@ -50,14 +49,14 @@ is_ima_sig_required()
 # Return 1 for PE signature found and 0 for not found.
 check_for_pesig()
 {
-	which pesign > /dev/null 2>&1 || log_skip "pesign not found"
+	which pesign > /dev/null 2>&1 || ksft_skip "pesign not found"
 
 	pesign -i $KERNEL_IMAGE --show-signature | grep -q "No signatures"
 	local ret=$?
 	if [ $ret -eq 1 ]; then
-		log_info "kexec kernel image PE signed"
+		ksft_info "kexec kernel image PE signed"
 	else
-		log_info "kexec kernel image not PE signed"
+		ksft_info "kexec kernel image not PE signed"
 	fi
 	return $ret
 }
@@ -70,16 +69,16 @@ check_for_imasig()
 
 	which getfattr > /dev/null 2>&1
 	if [ $?	-eq 1 ]; then
-		log_skip "getfattr not found"
+		ksft_skip "getfattr not found"
 	fi
 
 	line=$(getfattr -n security.ima -e hex --absolute-names $KERNEL_IMAGE 2>&1)
 	echo $line | grep -q "security.ima=0x03"
 	if [ $? -eq 0 ]; then
 		ret=1
-		log_info "kexec kernel image IMA signed"
+		ksft_info "kexec kernel image IMA signed"
 	else
-		log_info "kexec kernel image not IMA signed"
+		ksft_info "kexec kernel image not IMA signed"
 	fi
 	return $ret
 }
@@ -99,73 +98,69 @@ kexec_file_load_test()
 		# policy, make sure either an IMA or PE signature exists.
 		if [ $secureboot -eq 1 ] && [ $arch_policy -eq 1 ] && \
 			[ $ima_signed -eq 0 ] && [ $pe_signed -eq 0 ]; then
-			log_fail "$succeed_msg (missing sig)"
+			ksft_fail "$succeed_msg (missing sig)"
 		fi
 
 		if [ $kexec_sig_required -eq 1 -o $pe_sig_required -eq 1 ] \
 		     && [ $pe_signed -eq 0 ]; then
-			log_fail "$succeed_msg (missing PE sig)"
+			ksft_fail "$succeed_msg (missing PE sig)"
 		fi
 
 		if [ $ima_sig_required -eq 1 ] && [ $ima_signed -eq 0 ]; then
-			log_fail "$succeed_msg (missing IMA sig)"
+			ksft_fail "$succeed_msg (missing IMA sig)"
 		fi
 
 		if [ $pe_sig_required -eq 0 ] && [ $ima_appraise -eq 1 ] \
 		    && [ $ima_sig_required -eq 0 ] && [ $ima_signed -eq 0 ] \
 	            && [ $ima_read_policy -eq 0 ]; then
-			log_fail "$succeed_msg (possibly missing IMA sig)"
+			ksft_fail "$succeed_msg (possibly missing IMA sig)"
 		fi
 
 		if [ $pe_sig_required -eq 0 ] && [ $ima_appraise -eq 0 ]; then
-			log_info "No signature verification required"
+			ksft_info "No signature verification required"
 		elif [ $pe_sig_required -eq 0 ] && [ $ima_appraise -eq 1 ] \
 		    && [ $ima_sig_required -eq 0 ] && [ $ima_signed -eq 0 ] \
 	            && [ $ima_read_policy -eq 1 ]; then
-			log_info "No signature verification required"
+			ksft_info "No signature verification required"
 		fi
 
-		log_pass "$succeed_msg"
+		ksft_pass "$succeed_msg"
 	fi
 
 	# Check the reason for the kexec_file_load failure
 	echo $line | grep -q "Required key not available"
 	if [ $? -eq 0 ]; then
 		if [ $platform_keyring -eq 0 ]; then
-			log_pass "$failed_msg (-ENOKEY), $key_msg"
+			ksft_pass "$failed_msg (-ENOKEY), $key_msg"
 		else
-			log_pass "$failed_msg (-ENOKEY)"
+			ksft_pass "$failed_msg (-ENOKEY)"
 		fi
 	fi
 
 	if [ $kexec_sig_required -eq 1 -o $pe_sig_required -eq 1 ] \
 	     && [ $pe_signed -eq 0 ]; then
-		log_pass "$failed_msg (missing PE sig)"
+		ksft_pass "$failed_msg (missing PE sig)"
 	fi
 
 	if [ $ima_sig_required -eq 1 ] && [ $ima_signed -eq 0 ]; then
-		log_pass "$failed_msg (missing IMA sig)"
+		ksft_pass "$failed_msg (missing IMA sig)"
 	fi
 
 	if [ $pe_sig_required -eq 0 ] && [ $ima_appraise -eq 1 ] \
 	    && [ $ima_sig_required -eq 0 ] && [ $ima_read_policy -eq 0 ] \
 	    && [ $ima_signed -eq 0 ]; then
-		log_pass "$failed_msg (possibly missing IMA sig)"
+		ksft_pass "$failed_msg (possibly missing IMA sig)"
 	fi
 
-	log_pass "$failed_msg"
-	return 0
+	ksft_pass "$failed_msg"
 }
 
-# kexec requires root privileges
-require_root_privileges
-
-# get the kernel config
+ksft_require_root
 get_kconfig
 
 kconfig_enabled "CONFIG_KEXEC_FILE=y" "kexec_file_load is enabled"
 if [ $? -eq 0 ]; then
-	log_skip "kexec_file_load is not enabled"
+	ksft_skip "kexec_file_load is not enabled"
 fi
 
 # Determine which kernel config options are enabled
diff --git a/tools/testing/selftests/kexec/test_kexec_load.sh b/tools/testing/selftests/kexec/test_kexec_load.sh
index 49c6aa929137..c5d4eaff74c9 100755
--- a/tools/testing/selftests/kexec/test_kexec_load.sh
+++ b/tools/testing/selftests/kexec/test_kexec_load.sh
@@ -4,18 +4,14 @@
 # Prevent loading a kernel image via the kexec_load syscall when
 # signatures are required.  (Dependent on CONFIG_IMA_ARCH_POLICY.)
 
-TEST="$0"
-. ./kexec_common_lib.sh
+. $(dirname $0)/kexec_common_lib.sh
 
-# kexec requires root privileges
-require_root_privileges
-
-# get the kernel config
+ksft_require_root
 get_kconfig
 
 kconfig_enabled "CONFIG_KEXEC=y" "kexec_load is enabled"
 if [ $? -eq 0 ]; then
-	log_skip "kexec_load is not enabled"
+	ksft_skip "kexec_load is not enabled"
 fi
 
 kconfig_enabled "CONFIG_IMA_APPRAISE=y" "IMA enabled"
@@ -33,15 +29,15 @@ kexec --load $KERNEL_IMAGE > /dev/null 2>&1
 if [ $? -eq 0 ]; then
 	kexec --unload
 	if [ $secureboot -eq 1 ] && [ $arch_policy -eq 1 ]; then
-		log_fail "kexec_load succeeded"
+		ksft_fail "kexec_load succeeded"
 	elif [ $ima_appraise -eq 0 -o $arch_policy -eq 0 ]; then
-		log_info "Either IMA or the IMA arch policy is not enabled"
+		ksft_info "Either IMA or the IMA arch policy is not enabled"
 	fi
-	log_pass "kexec_load succeeded"
+	ksft_pass "kexec_load succeeded"
 else
 	if [ $secureboot -eq 1 ] && [ $arch_policy -eq 1 ] ; then
-		log_pass "kexec_load failed"
+		ksft_pass "kexec_load failed"
 	else
-		log_fail "kexec_load failed"
+		ksft_fail "kexec_load failed"
 	fi
 fi
-- 
2.20.1

[RFC PATCH 1/2] selftests: Start shell API

[zohar]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1885 bytes --]

Hi Petr,

On Sat, 2019-04-06 at 23:49 +0200, Petr Vorel wrote:
> kselftest.sh is a beginning of shell API.
> ATM it's a stub (target could be as rich as LTP API), containing only:
> * exit codes
> * filling TEST variable
> * logging functions
> * requiring root function
> * add script directory into PATH
> 
> Inspired by kexec functions (with some cleanup)
> and LTP.
> 
> Signed-off-by: Petr Vorel <pvorel at suse.cz>
> ---
>  tools/testing/selftests/kselftest.sh | 53 ++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 tools/testing/selftests/kselftest.sh
> 
> diff --git a/tools/testing/selftests/kselftest.sh b/tools/testing/selftests/kselftest.sh
> new file mode 100644
> index 000000000000..519ec2707dd8
> --- /dev/null
> +++ b/tools/testing/selftests/kselftest.sh
> @@ -0,0 +1,53 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2019 Petr Vorel <pvorel at suse.cz>
> +
> +PATH="$(dirname $0):$PATH"
> +
> +KSFT_PASS=0
> +KSFT_FAIL=1
> +KSFT_XFAIL=2
> +KSFT_XPASS=3
> +KSFT_SKIP=4

The kexec tests only defined functions for PASS, FAIL, and SKIP.  What
is the difference between KSFT_FAIL and KSFT_XFAIL, and similarly
between KSFT_PASS and KSFT_XPASS?  Either here or above the functions
should be a comment.

> +
> +TEST=$(basename $0)
> +
> +ksft_info()
> +{
> +	echo "[INFO] $TEST: $1"
> +}


The "ksft_" prefix is good.

Mimi

> +ksft_pass()
> +{
> +	echo "[PASS] $TEST: $1"
> +	exit $KSFT_PASS
> +}
> +
> +ksft_fail()
> +{
> +	echo "[FAIL] $TEST: $1"
> +	exit $KSFT_FAIL
> +}
> +
> +ksft_xfail()
> +{
> +	echo "[FAIL] $TEST: $1"
> +	exit $KSFT_XFAIL
> +}

> +ksft_xpass()
> +{
> +	echo "[PASS] $TEST: $1"
> +	exit $KSFT_XPASS
> +}
> +
> +ksft_skip()
> +{
> +	echo "[SKIP] $TEST: $1"
> +	exit $KSFT_SKIP
> +}
> +
> +ksft_require_root()
> +{
> +	[ $(id -ru) -eq 0 ] || ksft_skip "requires root privileges"
> +}

[RFC PATCH 1/2] selftests: Start shell API

[Mimi Zohar]

Hi Petr,

On Sat, 2019-04-06@23:49 +0200, Petr Vorel wrote:
> kselftest.sh is a beginning of shell API.
> ATM it's a stub (target could be as rich as LTP API), containing only:
> * exit codes
> * filling TEST variable
> * logging functions
> * requiring root function
> * add script directory into PATH
> 
> Inspired by kexec functions (with some cleanup)
> and LTP.
> 
> Signed-off-by: Petr Vorel <pvorel at suse.cz>
> ---
>  tools/testing/selftests/kselftest.sh | 53 ++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 tools/testing/selftests/kselftest.sh
> 
> diff --git a/tools/testing/selftests/kselftest.sh b/tools/testing/selftests/kselftest.sh
> new file mode 100644
> index 000000000000..519ec2707dd8
> --- /dev/null
> +++ b/tools/testing/selftests/kselftest.sh
> @@ -0,0 +1,53 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2019 Petr Vorel <pvorel at suse.cz>
> +
> +PATH="$(dirname $0):$PATH"
> +
> +KSFT_PASS=0
> +KSFT_FAIL=1
> +KSFT_XFAIL=2
> +KSFT_XPASS=3
> +KSFT_SKIP=4

The kexec tests only defined functions for PASS, FAIL, and SKIP.  What
is the difference between KSFT_FAIL and KSFT_XFAIL, and similarly
between KSFT_PASS and KSFT_XPASS?  Either here or above the functions
should be a comment.

> +
> +TEST=$(basename $0)
> +
> +ksft_info()
> +{
> +	echo "[INFO] $TEST: $1"
> +}


The "ksft_" prefix is good.

Mimi

> +ksft_pass()
> +{
> +	echo "[PASS] $TEST: $1"
> +	exit $KSFT_PASS
> +}
> +
> +ksft_fail()
> +{
> +	echo "[FAIL] $TEST: $1"
> +	exit $KSFT_FAIL
> +}
> +
> +ksft_xfail()
> +{
> +	echo "[FAIL] $TEST: $1"
> +	exit $KSFT_XFAIL
> +}

> +ksft_xpass()
> +{
> +	echo "[PASS] $TEST: $1"
> +	exit $KSFT_XPASS
> +}
> +
> +ksft_skip()
> +{
> +	echo "[SKIP] $TEST: $1"
> +	exit $KSFT_SKIP
> +}
> +
> +ksft_require_root()
> +{
> +	[ $(id -ru) -eq 0 ] || ksft_skip "requires root privileges"
> +}

[RFC PATCH 2/2] selftest/kexec: Use kselftest shell API

[zohar]

Hi Petr,

On Sat, 2019-04-06 at 23:49 +0200, Petr Vorel wrote:
> using kselftest.sh helpers + minor not related changes in kexec
> (i.e. remove executable bit from kexec library as not needed
> for library).
 
Acked-by:  Mimi Zohar <zohar at linux.ibm.com>

[RFC PATCH 2/2] selftest/kexec: Use kselftest shell API

[Mimi Zohar]

Hi Petr,

On Sat, 2019-04-06@23:49 +0200, Petr Vorel wrote:
> using kselftest.sh helpers + minor not related changes in kexec
> (i.e. remove executable bit from kexec library as not needed
> for library).
 
Acked-by:  Mimi Zohar <zohar at linux.ibm.com>

[RFC PATCH 1/2] selftests: Start shell API

[chrubis]

Hi!
> +ksft_pass()
> +{
> +	echo "[PASS] $TEST: $1"
> +	exit $KSFT_PASS
> +}
> +
> +ksft_fail()
> +{
> +	echo "[FAIL] $TEST: $1"
> +	exit $KSFT_FAIL
> +}

I think that the main disadvantage here is that these functions call
exit instead of storing the results which leads to a common pattern of
passing the result up the function call chain which is prone to errors.

What I have learned the hard way over the years is that the result
reporting should be separated from the functions that exit the tests and
that the test code should not be trusted with passing the overall test
result at the end. I've seen too many cases where the actuall failure
was ignored becaues the failure was lost on it's way to the main
function.

Another lesson is that tests shouldn't implement the main() function,
that is something that the test library should do, which allows for
resources to be listed in a declarative way instead of calling init
funcitons at the start of the tests. Which means that in LTP you can say
"mount at least 512MB device formatted with ext4 to this mount point"
and all this handled in the test library before the actual test starts.

As the last point this completely misses a cleanup callback support,
i.e. function that is called to clean up if you need to exit in the
middle of a test in a case of an error.

-- 
Cyril Hrubis
chrubis at suse.cz

[RFC PATCH 1/2] selftests: Start shell API

[Cyril Hrubis]

Hi!
> +ksft_pass()
> +{
> +	echo "[PASS] $TEST: $1"
> +	exit $KSFT_PASS
> +}
> +
> +ksft_fail()
> +{
> +	echo "[FAIL] $TEST: $1"
> +	exit $KSFT_FAIL
> +}

I think that the main disadvantage here is that these functions call
exit instead of storing the results which leads to a common pattern of
passing the result up the function call chain which is prone to errors.

What I have learned the hard way over the years is that the result
reporting should be separated from the functions that exit the tests and
that the test code should not be trusted with passing the overall test
result at the end. I've seen too many cases where the actuall failure
was ignored becaues the failure was lost on it's way to the main
function.

Another lesson is that tests shouldn't implement the main() function,
that is something that the test library should do, which allows for
resources to be listed in a declarative way instead of calling init
funcitons at the start of the tests. Which means that in LTP you can say
"mount at least 512MB device formatted with ext4 to this mount point"
and all this handled in the test library before the actual test starts.

As the last point this completely misses a cleanup callback support,
i.e. function that is called to clean up if you need to exit in the
middle of a test in a case of an error.

-- 
Cyril Hrubis
chrubis at suse.cz

[RFC PATCH 0/2] Kselftest shell (or even C) API

[chrubis]

Hi!
> It's just a small example how things could be. Draft, not meant to be
> really merged. But instead of defining shell library (with more useful
> helpers), I'd rather adopt LTP shell [1] and C [2] API to kselftest.
> LTP API [1] is more like a framework, easy to use with a lot of helpers
> making tests 1) small, concentrating on the problem itself 2) have
> unique output. API is well documented [3] [4], it's creator Cyril Hrubis
> made it after years experience of handling (at the time) quite bad
> quality LTP code.  Rewriting LTP tests to use this API improved tests a
> lot (less buggy, easier to read).
> 
> Some examples of advantages of LTP API:
> * SAFE_*() macros for C, which handles errors inside a library
> * unified messages, unified test status, unified way to exit testing due
> missing functionality, at the end of testing there is summary of passed,
> failed and skipped tests
> * many prepared functionality for both C and shell
> * handling threads, parent-child synchronization
> * setup and cleanup functions
> * "flags" for defining requirements or certain functionality (need root, temporary
> directory, ...)
> * and many other

I guess that I can help to create a library with a subset of LTP C API
that could be used to implement C tests if that is something that has a
good chance to get adopted.

-- 
Cyril Hrubis
chrubis at suse.cz

[RFC PATCH 0/2] Kselftest shell (or even C) API

[Cyril Hrubis]

Hi!
> It's just a small example how things could be. Draft, not meant to be
> really merged. But instead of defining shell library (with more useful
> helpers), I'd rather adopt LTP shell [1] and C [2] API to kselftest.
> LTP API [1] is more like a framework, easy to use with a lot of helpers
> making tests 1) small, concentrating on the problem itself 2) have
> unique output. API is well documented [3] [4], it's creator Cyril Hrubis
> made it after years experience of handling (at the time) quite bad
> quality LTP code.  Rewriting LTP tests to use this API improved tests a
> lot (less buggy, easier to read).
> 
> Some examples of advantages of LTP API:
> * SAFE_*() macros for C, which handles errors inside a library
> * unified messages, unified test status, unified way to exit testing due
> missing functionality, at the end of testing there is summary of passed,
> failed and skipped tests
> * many prepared functionality for both C and shell
> * handling threads, parent-child synchronization
> * setup and cleanup functions
> * "flags" for defining requirements or certain functionality (need root, temporary
> directory, ...)
> * and many other

I guess that I can help to create a library with a subset of LTP C API
that could be used to implement C tests if that is something that has a
good chance to get adopted.

-- 
Cyril Hrubis
chrubis at suse.cz

[RFC PATCH 0/2] Kselftest shell (or even C) API

[zohar]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 3351 bytes --]

Hi Petr, Shuah,

On Sat, 2019-04-06 at 23:49 +0200, Petr Vorel wrote:
> Hi,
> 
> this is a draft trying to define some API in order to remove some
> redundancy from kselftest shell scripts. Existing kselftest.h already
> defines some sort of API for C, there is none for shell.

Shuah, when the tests were in the selftests/ima directory I was
planning on including them in my pull request; and then they moved to
selftests/kexec.  As they were still IMA related, I was still
shepherding them and planned on including them in my pull request. (Is
this Okay?  Your Review/Ack would be much appreciated.)  This patch
set, however, introduces a set of "common" set of kselftest functions.

Originally, you suggested deferring defining a set of "common"
kselftests functions to prevent delaying upstreaming the tests.  With
these patches, that time is here.  How do you want to handle this?

Thanks,

Mimi

> 
> It's just a small example how things could be. Draft, not meant to be
> really merged. But instead of defining shell library (with more useful
> helpers), I'd rather adopt LTP shell [1] and C [2] API to kselftest.
> LTP API [1] is more like a framework, easy to use with a lot of helpers
> making tests 1) small, concentrating on the problem itself 2) have
> unique output. API is well documented [3] [4], it's creator Cyril Hrubis
> made it after years experience of handling (at the time) quite bad
> quality LTP code.  Rewriting LTP tests to use this API improved tests a
> lot (less buggy, easier to read).
> 
> Some examples of advantages of LTP API:
> * SAFE_*() macros for C, which handles errors inside a library
> * unified messages, unified test status, unified way to exit testing due
> missing functionality, at the end of testing there is summary of passed,
> failed and skipped tests
> * many prepared functionality for both C and shell
> * handling threads, parent-child synchronization
> * setup and cleanup functions
> * "flags" for defining requirements or certain functionality (need root, temporary
> directory, ...)
> * and many other
> 
> kselftest and LTP has a bit different goals and approach. Probably
> not all of LTP API is needed atm, but I guess it's at least worth of
> thinking to adopt it.
> 
> There are of course other options: reinvent a wheel or left kselftest
> code in a state it is now (code quality varies, some of the code is
> really messy, buggy, not even compile).
> 
> [1] https://github.com/linux-test-project/ltp/blob/master/testcases/lib/tst_test.sh
> [2] https://github.com/linux-test-project/ltp/tree/master/lib
> [3] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#22-writing-a-test-in-c
> [4] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#23-writing-a-testcase-in-shell
> 
> Petr Vorel (2):
>   selftests: Start shell API
>   selftest/kexec: Use kselftest shell API
> 
>  .../selftests/kexec/kexec_common_lib.sh       | 74 +++++--------------
>  .../selftests/kexec/test_kexec_file_load.sh   | 53 ++++++-------
>  .../selftests/kexec/test_kexec_load.sh        | 20 ++---
>  tools/testing/selftests/kselftest.sh          | 53 +++++++++++++
>  4 files changed, 105 insertions(+), 95 deletions(-)
>  mode change 100755 => 100644 tools/testing/selftests/kexec/kexec_common_lib.sh
>  create mode 100644 tools/testing/selftests/kselftest.sh
> 

[RFC PATCH 0/2] Kselftest shell (or even C) API

[Mimi Zohar]

Hi Petr, Shuah,

On Sat, 2019-04-06@23:49 +0200, Petr Vorel wrote:
> Hi,
> 
> this is a draft trying to define some API in order to remove some
> redundancy from kselftest shell scripts. Existing kselftest.h already
> defines some sort of API for C, there is none for shell.

Shuah, when the tests were in the selftests/ima directory I was
planning on including them in my pull request; and then they moved to
selftests/kexec.  As they were still IMA related, I was still
shepherding them and planned on including them in my pull request. (Is
this Okay?  Your Review/Ack would be much appreciated.)  This patch
set, however, introduces a set of "common" set of kselftest functions.

Originally, you suggested deferring defining a set of "common"
kselftests functions to prevent delaying upstreaming the tests.  With
these patches, that time is here.  How do you want to handle this?

Thanks,

Mimi

> 
> It's just a small example how things could be. Draft, not meant to be
> really merged. But instead of defining shell library (with more useful
> helpers), I'd rather adopt LTP shell [1] and C [2] API to kselftest.
> LTP API [1] is more like a framework, easy to use with a lot of helpers
> making tests 1) small, concentrating on the problem itself 2) have
> unique output. API is well documented [3] [4], it's creator Cyril Hrubis
> made it after years experience of handling (at the time) quite bad
> quality LTP code.  Rewriting LTP tests to use this API improved tests a
> lot (less buggy, easier to read).
> 
> Some examples of advantages of LTP API:
> * SAFE_*() macros for C, which handles errors inside a library
> * unified messages, unified test status, unified way to exit testing due
> missing functionality, at the end of testing there is summary of passed,
> failed and skipped tests
> * many prepared functionality for both C and shell
> * handling threads, parent-child synchronization
> * setup and cleanup functions
> * "flags" for defining requirements or certain functionality (need root, temporary
> directory, ...)
> * and many other
> 
> kselftest and LTP has a bit different goals and approach. Probably
> not all of LTP API is needed atm, but I guess it's at least worth of
> thinking to adopt it.
> 
> There are of course other options: reinvent a wheel or left kselftest
> code in a state it is now (code quality varies, some of the code is
> really messy, buggy, not even compile).
> 
> [1] https://github.com/linux-test-project/ltp/blob/master/testcases/lib/tst_test.sh
> [2] https://github.com/linux-test-project/ltp/tree/master/lib
> [3] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#22-writing-a-test-in-c
> [4] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#23-writing-a-testcase-in-shell
> 
> Petr Vorel (2):
>   selftests: Start shell API
>   selftest/kexec: Use kselftest shell API
> 
>  .../selftests/kexec/kexec_common_lib.sh       | 74 +++++--------------
>  .../selftests/kexec/test_kexec_file_load.sh   | 53 ++++++-------
>  .../selftests/kexec/test_kexec_load.sh        | 20 ++---
>  tools/testing/selftests/kselftest.sh          | 53 +++++++++++++
>  4 files changed, 105 insertions(+), 95 deletions(-)
>  mode change 100755 => 100644 tools/testing/selftests/kexec/kexec_common_lib.sh
>  create mode 100644 tools/testing/selftests/kselftest.sh
> 

[RFC PATCH 1/2] selftests: Start shell API

[pvorel]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1228 bytes --]

Hi Mimi,

> > +++ b/tools/testing/selftests/kselftest.sh
> > @@ -0,0 +1,53 @@
> > +#!/bin/sh
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2019 Petr Vorel <pvorel at suse.cz>
> > +
> > +PATH="$(dirname $0):$PATH"
> > +
> > +KSFT_PASS=0
> > +KSFT_FAIL=1
> > +KSFT_XFAIL=2
> > +KSFT_XPASS=3
> > +KSFT_SKIP=4

> The kexec tests only defined functions for PASS, FAIL, and SKIP.  What
> is the difference between KSFT_FAIL and KSFT_XFAIL, and similarly
> between KSFT_PASS and KSFT_XPASS?  Either here or above the functions
> should be a comment.
I guess xfail and xpass are taken from pytest [1].
I took them from kselftest.h, in order to be somehow compatible with existing C
API. But grepping code xpass is never used (not even in list of kselftest results [2]),
xfail is used in about 4 tests (binderfs, ftrace, pidfd, seccomp).

But I'm not a big fan of this pytest terminology "something is resulting the
opposite than expected", IMHO simple pass and fail are enough.
On the other hand I miss "test failed in preparation phase" (TBROK in LTP),
skip has different meaning.

Kind regards,
Petr

[1] https://docs.pytest.org/en/latest/skipping.html
[2] https://www.spinics.net/lists/linux-kselftest/msg06651.html

[RFC PATCH 1/2] selftests: Start shell API

[Petr Vorel]

Hi Mimi,

> > +++ b/tools/testing/selftests/kselftest.sh
> > @@ -0,0 +1,53 @@
> > +#!/bin/sh
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2019 Petr Vorel <pvorel at suse.cz>
> > +
> > +PATH="$(dirname $0):$PATH"
> > +
> > +KSFT_PASS=0
> > +KSFT_FAIL=1
> > +KSFT_XFAIL=2
> > +KSFT_XPASS=3
> > +KSFT_SKIP=4

> The kexec tests only defined functions for PASS, FAIL, and SKIP.  What
> is the difference between KSFT_FAIL and KSFT_XFAIL, and similarly
> between KSFT_PASS and KSFT_XPASS?  Either here or above the functions
> should be a comment.
I guess xfail and xpass are taken from pytest [1].
I took them from kselftest.h, in order to be somehow compatible with existing C
API. But grepping code xpass is never used (not even in list of kselftest results [2]),
xfail is used in about 4 tests (binderfs, ftrace, pidfd, seccomp).

But I'm not a big fan of this pytest terminology "something is resulting the
opposite than expected", IMHO simple pass and fail are enough.
On the other hand I miss "test failed in preparation phase" (TBROK in LTP),
skip has different meaning.

Kind regards,
Petr

[1] https://docs.pytest.org/en/latest/skipping.html
[2] https://www.spinics.net/lists/linux-kselftest/msg06651.html

[RFC PATCH 0/2] Kselftest shell (or even C) API

[pvorel]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1277 bytes --]

Hi Mimi, Shuah,
> Hi Petr, Shuah,

> On Sat, 2019-04-06 at 23:49 +0200, Petr Vorel wrote:
> > Hi,

> > this is a draft trying to define some API in order to remove some
> > redundancy from kselftest shell scripts. Existing kselftest.h already
> > defines some sort of API for C, there is none for shell.

> Shuah, when the tests were in the selftests/ima directory I was
> planning on including them in my pull request; and then they moved to
> selftests/kexec.  As they were still IMA related, I was still
> shepherding them and planned on including them in my pull request. (Is
> this Okay?  Your Review/Ack would be much appreciated.)  This patch
> set, however, introduces a set of "common" set of kselftest functions.

> Originally, you suggested deferring defining a set of "common"
> kselftests functions to prevent delaying upstreaming the tests.  With
> these patches, that time is here.  How do you want to handle this?

I agree with separation of common kselftests functions / proper API effort.
kexec tests are ready and IMHO should not be delayed with this effort.
"common functions" proposed by this patchset are more for to start a discussion
about it, what I brought doesn't help much. Proper design takes some time.

> Thanks,

> Mimi

Kind regards,
Petr

[RFC PATCH 0/2] Kselftest shell (or even C) API

[Petr Vorel]

Hi Mimi, Shuah,
> Hi Petr, Shuah,

> On Sat, 2019-04-06@23:49 +0200, Petr Vorel wrote:
> > Hi,

> > this is a draft trying to define some API in order to remove some
> > redundancy from kselftest shell scripts. Existing kselftest.h already
> > defines some sort of API for C, there is none for shell.

> Shuah, when the tests were in the selftests/ima directory I was
> planning on including them in my pull request; and then they moved to
> selftests/kexec.  As they were still IMA related, I was still
> shepherding them and planned on including them in my pull request. (Is
> this Okay?  Your Review/Ack would be much appreciated.)  This patch
> set, however, introduces a set of "common" set of kselftest functions.

> Originally, you suggested deferring defining a set of "common"
> kselftests functions to prevent delaying upstreaming the tests.  With
> these patches, that time is here.  How do you want to handle this?

I agree with separation of common kselftests functions / proper API effort.
kexec tests are ready and IMHO should not be delayed with this effort.
"common functions" proposed by this patchset are more for to start a discussion
about it, what I brought doesn't help much. Proper design takes some time.

> Thanks,

> Mimi

Kind regards,
Petr

[RFC PATCH 1/2] selftests: Start shell API

[pvorel]

Hi,

> > +ksft_pass()
> > +{
> > +	echo "[PASS] $TEST: $1"
> > +	exit $KSFT_PASS
> > +}
> > +
> > +ksft_fail()
> > +{
> > +	echo "[FAIL] $TEST: $1"
> > +	exit $KSFT_FAIL
> > +}

> I think that the main disadvantage here is that these functions call
> exit instead of storing the results which leads to a common pattern of
> passing the result up the function call chain which is prone to errors.

> What I have learned the hard way over the years is that the result
> reporting should be separated from the functions that exit the tests and
> that the test code should not be trusted with passing the overall test
> result at the end. I've seen too many cases where the actuall failure
> was ignored becaues the failure was lost on it's way to the main
> function.

> Another lesson is that tests shouldn't implement the main() function,
> that is something that the test library should do, which allows for
> resources to be listed in a declarative way instead of calling init
> funcitons at the start of the tests. Which means that in LTP you can say
> "mount at least 512MB device formatted with ext4 to this mount point"
> and all this handled in the test library before the actual test starts.

> As the last point this completely misses a cleanup callback support,
> i.e. function that is called to clean up if you need to exit in the
> middle of a test in a case of an error.

Agree with all mentioned. My patchset was mainly to bring the discussion.
Although library defining some general functions and constants to reduce
duplicity is itself a small improvement, kselftest deserves a proper API.
For both C and shell.

Kind regards,
Petr

[RFC PATCH 1/2] selftests: Start shell API

[Petr Vorel]

Hi,

> > +ksft_pass()
> > +{
> > +	echo "[PASS] $TEST: $1"
> > +	exit $KSFT_PASS
> > +}
> > +
> > +ksft_fail()
> > +{
> > +	echo "[FAIL] $TEST: $1"
> > +	exit $KSFT_FAIL
> > +}

> I think that the main disadvantage here is that these functions call
> exit instead of storing the results which leads to a common pattern of
> passing the result up the function call chain which is prone to errors.

> What I have learned the hard way over the years is that the result
> reporting should be separated from the functions that exit the tests and
> that the test code should not be trusted with passing the overall test
> result at the end. I've seen too many cases where the actuall failure
> was ignored becaues the failure was lost on it's way to the main
> function.

> Another lesson is that tests shouldn't implement the main() function,
> that is something that the test library should do, which allows for
> resources to be listed in a declarative way instead of calling init
> funcitons at the start of the tests. Which means that in LTP you can say
> "mount at least 512MB device formatted with ext4 to this mount point"
> and all this handled in the test library before the actual test starts.

> As the last point this completely misses a cleanup callback support,
> i.e. function that is called to clean up if you need to exit in the
> middle of a test in a case of an error.

Agree with all mentioned. My patchset was mainly to bring the discussion.
Although library defining some general functions and constants to reduce
duplicity is itself a small improvement, kselftest deserves a proper API.
For both C and shell.

Kind regards,
Petr

[RFC PATCH 0/2] Kselftest shell (or even C) API

[pvorel]

Hi,

> I guess that I can help to create a library with a subset of LTP C API
> that could be used to implement C tests if that is something that has a
> good chance to get adopted.
Great! I'd take shell part. So, it's up to Shuah I guess.

Kind regards,
Petr

[RFC PATCH 0/2] Kselftest shell (or even C) API

[Petr Vorel]

Hi,

> I guess that I can help to create a library with a subset of LTP C API
> that could be used to implement C tests if that is something that has a
> good chance to get adopted.
Great! I'd take shell part. So, it's up to Shuah I guess.

Kind regards,
Petr