Re: [PATCH v5 07/18] tracing/remotes: selftests: Add a test for the dmesg tracefs file

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