On Wed, Aug 12, 2026 at 04:40:39PM +0000, [email protected] wrote:
> 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?
sigh... I have s/TMPDIR/tmp/ temporarily (pun unintended) to debug the
script...
I'll fix that!
>
> > + 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
>
> > + 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
