Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] The test contains an unconditional early return, leaving dead code intended to manually trigger a kernel panic, which fails to validate the feature and creates a dangerous booby trap. - [Medium] Unconditional write to `dump_on_panic` in the shared `setup_remote()` function will break all ftrace remote selftests on older kernels. - [Medium] The new test fails instead of gracefully skipping on kernels that do not support the dump_on_panic feature. --
--- Patch [14]: [PATCH v5 14/18] tracing/remotes: selftests: Add a test for the dump_on_panic tracefs file --- commit 77956bd02c91415a58b7ae353d502fbc7538fb12 Author: Vincent Donnefort <[email protected]> tracing/remotes: selftests: Add a test for the dump_on_panic tracefs file This patch adds a selftest for the dump_on_panic tracefs file. It creates a new test script and updates the remote setup functions to configure the parameter. > diff --git a/tools/testing/selftests/ftrace/test.d/remotes/dump_on_panic.tc > b/tools/testing/selftests/ftrace/test.d/remotes/dump_on_panic.tc > new file mode 100644 > index 0000000000000..cb9b4c6dc86c5 > --- /dev/null > +++ b/tools/testing/selftests/ftrace/test.d/remotes/dump_on_panic.tc > @@ -0,0 +1,51 @@ > +#!/bin/sh > +# SPDX-License-Identifier: GPL-2.0 > +# description: Test trace remote dump_on_panic > +# requires: remotes/test > + > +. $TEST_DIR/remotes/functions > + > +test_dump_on_panic() > +{ > + # > + # Toggle when the buffer is loaded > + # > + assert_loaded [Severity: Medium] Since this test specifically targets the dump_on_panic file, should there be an existence check here (such as [ -f dump_on_panic ] || exit_unsupported) to prevent false negatives? Without this check, the test fails instead of gracefully skipping on kernels that do not support the dump_on_panic feature. > + echo 1 > dump_on_panic > + echo 0 > dump_on_panic [ ... ] > + echo 1 > dump_on_panic > + echo 1 > tracing_on > + echo 0 > tracing_on > + > + # REMOVE ME FOR A PROPER OOPS TEST > + return > + > + echo 1 > tracing_on > + > + for i in $(seq 1 32); do > + echo $i > write_event > + done > + > + echo c > /proc/sysrq-trigger > +} [Severity: Medium] Is this early return intentional? This skips the actual panic-triggering code, leaving the dump_on_panic feature untested. Furthermore, leaving the sysrq-trigger command behind an early return could be dangerous if someone removes the return without realizing it will crash the host machine during a CI run. > + > +if [ -z "$SOURCE_REMOTE_TEST" ]; then > + set -e > + setup_remote_test > + test_dump_on_panic > +fi > diff --git a/tools/testing/selftests/ftrace/test.d/remotes/functions > b/tools/testing/selftests/ftrace/test.d/remotes/functions > index 4a14aa72fdf09..bdd28b5b85966 100644 > --- a/tools/testing/selftests/ftrace/test.d/remotes/functions > +++ b/tools/testing/selftests/ftrace/test.d/remotes/functions > @@ -9,6 +9,7 @@ setup_remote() > cd remotes/$name/ > echo 0 > tracing_on > echo 0 > dmesg > + echo 0 > dump_on_panic [Severity: Medium] Could this unconditional write cause failures on older kernels? Because setup_remote() is a shared setup function, and test scripts execute with set -e (errexit), if this is run against an older kernel lacking the dump_on_panic file, the shell will immediately abort. This would break all ftrace remote selftests on those kernels. > clear_trace > echo 7 > buffer_size_kb > echo 0 > events/enable -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=14
