Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [Medium] The test script leaks a temporary file in `/tmp/` if the test aborts due to a failure (e.g., when a command fails under `set -e`). - [Medium] Unconditional write to the new `dmesg` tracefs file breaks the entire remote trace test suite on older kernels. - [Medium] The new `dmesg.tc` script lacks the necessary `# requires:` header for the `dmesg` tracefs file, causing it to fail instead of being skipped on older kernels. - [Low] The awk regular expression for matching dmesg timestamps contains an unescaped bracket, unintentionally creating a character class. --
--- Patch [7]: [PATCH v5 07/18] tracing/remotes: selftests: Add a test for the dmesg tracefs file --- commit bd86167985384496571cc5a8f03ca97dd77d76fd Author: Vincent Donnefort <[email protected]> tracing/remotes: selftests: Add a test for the dmesg tracefs file Exercise the newly introduced dmesg tracefs file that turns on and off the dmesg redirection. > diff --git a/tools/testing/selftests/ftrace/test.d/remotes/dmesg.tc > b/tools/testing/selftests/ftrace/test.d/remotes/dmesg.tc > new file mode 100644 > index 0000000000000..9d389f9d0fb78 > --- /dev/null > +++ b/tools/testing/selftests/ftrace/test.d/remotes/dmesg.tc > @@ -0,0 +1,81 @@ > +#!/bin/sh > +# SPDX-License-Identifier: GPL-2.0 > +# description: Test trace remote dmesg redirection > +# requires: remotes/test [Severity: Medium] Does this script also need a "# requires: remotes/test/dmesg" declaration? Without this explicit dependency, running the test on an older kernel that lacks the dmesg tracefs file will cause it to fail abruptly due to set -e, rather than gracefully skipping it as unsupported. [ ... ] > + nr_events=128 > + for i in $(seq 1 $nr_events); do > + echo $i > write_event > + done > + > + sleep 1 > + output=$(mktemp /tmp/remote_test.XXXXXX) [Severity: Medium] Will this leak a temporary file in /tmp/ if the test aborts prematurely? The script is running with set -e, so if a validation step like check_trace fails, it will immediately exit before reaching the rm $output command. Should this use $TMPDIR instead, since the ftracetest framework cleans it up automatically? > + dmesg | awk -v ts=$last_ts ' > + /^[ *[0-9]+\.[0-9]+\]/ { [Severity: Low] Does this regular expression match the bracket as intended? By failing to escape the first bracket (^[), this creates a character class that matches spaces, asterisks, brackets, or digits instead of a literal opening bracket. Could this be properly escaped as /^\[ *[0-9]+\.[0-9]+\]/ ? > + t=$0; sub(/^[[ \t]*/, "", t); sub(/].*/, "", t); > + if (t+0 > ts+0) print $0 > + }' | grep "selftest id=" | sed 's/^[^]]*] //'> $output [ ... ] > diff --git a/tools/testing/selftests/ftrace/test.d/remotes/functions > b/tools/testing/selftests/ftrace/test.d/remotes/functions > index 05224fac3653d..4a14aa72fdf09 100644 > --- a/tools/testing/selftests/ftrace/test.d/remotes/functions > +++ b/tools/testing/selftests/ftrace/test.d/remotes/functions > @@ -8,6 +8,7 @@ setup_remote() > > cd remotes/$name/ > echo 0 > tracing_on > + echo 0 > dmesg [Severity: Medium] Does this unconditional write break the test suite on older kernels? This setup function is used by all remote selftests. If the dmesg file does not exist, this echo will fail. Since the test scripts operate with set -e, this failure will cause the entire suite to crash on unsupported targets. Could we check for the file's existence before writing to it? > clear_trace > echo 7 > buffer_size_kb > echo 0 > events/enable -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
