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... [2] https://github.com/linux-test-project/ltp/tree/master/lib [3] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#22-wr... [4] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#23-wr...
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
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@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@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" +}
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@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@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"
+}
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@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
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.
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
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@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
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@linux.ibm.com
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.
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... [2] https://github.com/linux-test-project/ltp/tree/master/lib [3] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#22-wr... [4] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#23-wr...
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
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
linux-kselftest-mirror@lists.linaro.org
-
Cyril Hrubis -
Mimi Zohar -
Petr Vorel