Re: [PATCH v5 07/18] tracing/remotes: selftests: Add a test for the dmesg tracefs file
From: Vincent Donnefort <hidden>
Date: 2026-08-13 14:06:22
Also in:
sashiko-reviews
On Wed, Aug 12, 2026 at 04:40:39PM +0000, sashiko-bot@kernel.org wrote:
quoted hunk ↗ jump to hunk
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 [off-list ref] 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.quoted
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. [ ... ]quoted
+ 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?
sigh... I have s/TMPDIR/tmp/ temporarily (pun unintended) to debug the script... I'll fix that!
quoted
+ 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]+\]/ ?
Sure, I can escape "[" -- Vincent
quoted
+ t=$0; sub(/^[[ \t]*/, "", t); sub(/].*/, "", t); + if (t+0 > ts+0) print $0 + }' | grep "selftest id=" | sed 's/^[^]]*] //'> $output[ ... ]quoted
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?quoted
clear_trace echo 7 > buffer_size_kb echo 0 > events/enable-- Sashiko AI review · https://sashiko.dev/#/patchset/20260812161340.2417322-1-vdonnefort@google.com?part=7