Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] Test failure conditions incorrectly exit with a success status (0), silently hiding test failures from the kselftest framework. - [Medium] The script incorrectly assumes `.config` is available at runtime, which will cause false test failures when selftests are installed and run on a target device. --
commit 5991b9725cc8742537ed8b6a73204e4d44da0fc2 Author: Jim Cromie <[email protected]> selftests-dyndbg: add a dynamic_debug run_tests target This commit adds a dynamic-debug selftest script that modifies pr_debug() flags and checks their effects. It also integrates dynamic_debug into the kselftest framework's TARGETS list. > diff --git a/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh > b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh > new file mode 100755 > index 0000000000000..01c035fe8c9a2 > --- /dev/null > +++ b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh > @@ -0,0 +1,257 @@ > +#!/bin/bash > +# SPDX-License-Identifier: GPL-2.0-only [ ... ] > +[ -f "$KCONFIG_CONFIG" ] || KCONFIG_CONFIG=".config" > +if [ -f "$KCONFIG_CONFIG" ]; then [Severity: Medium] Will this work reliably when selftests are installed and run on a target device? When tests are run via `make install` on a target, the source tree's `.config` won't be available. If the `.config` is missing, the script defaults to `LACK_DD_BUILTIN=0`, which may cause false test failures if the target kernel doesn't actually have the config enabled. Could runtime indicators be checked instead of build-time files? > + echo "# consulting KCONFIG_CONFIG: $KCONFIG_CONFIG" > + grep -q "CONFIG_DYNAMIC_DEBUG=y" $KCONFIG_CONFIG ; LACK_DD_BUILTIN=$? > + grep -q "CONFIG_TEST_DYNAMIC_DEBUG=m" $KCONFIG_CONFIG ; LACK_TMOD=$? [ ... ] > +function handle_exit_code() { > + local exp_exit_code=0 > + [ $# == 4 ] && exp_exit_code=$4 > + if [ $3 -ne $exp_exit_code ]; then > + echo -e "${RED}: $BASH_SOURCE:$1 $2() expected to exit with code > $exp_exit_code" > + [ $3 == 1 ] && echo "Error: '$error_msg'" > + exit $exp_exit_code [Severity: Medium] Does this mask test failures? If the actual exit code (`$3`) does not match `exp_exit_code` (which defaults to 0), the script calls `exit $exp_exit_code`. Since `exp_exit_code` is typically 0 when expecting a command to succeed, the script will exit with 0 upon encountering an error, silently hiding the failure from the kselftest framework. > + fi > +} [ ... ] > +function check_match_ct { > + pattern="\s$1\s" > + exp_cnt=0 > + > + [ "$3" == "-r" ] && pattern="$1" > + let cnt=$(ddgrep "$pattern" | wc -l) > + if [ $V -eq 1 ] || [ "$3" == "-v" ] || [ "$4" == "-v" ]; then > + echo -ne "${BLUE}" && ddgrep $pattern && echo -ne "${NC}" > + fi > + [ $# -gt 1 ] && exp_cnt=$2 > + if [ $cnt -ne $exp_cnt ]; then > + echo -e "${RED}: $BASH_SOURCE:$BASH_LINENO check failed expected > $exp_cnt on $1, got $cnt" > + exit [Severity: Medium] Does this also mask test failures? Calling `exit` without arguments returns the status of the last executed command. Since the `echo` command immediately preceding it will succeed with a 0 exit code, the script exits with 0 and the kselftest framework will incorrectly mark the failed test as passed. This same pattern appears to be used in several other functions in the script. > + else > + echo ": $cnt matches on $1" > + fi > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=22
