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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Unconditional write to the newly introduced `dmesg` tracefs file in `setup_remote()` breaks existing remote tests on older kernels lacking this feature.
- [Medium] The dmesg test implicitly depends on dmesg timestamps and silently fails if they are disabled.
--

--- Patch [7]: [PATCH v6 07/18] tracing/remotes: selftests: Add a test for the dmesg tracefs file ---
commit f74a3f691a6b4375c6156c109a1d6336670de341
Author: Vincent Donnefort <[email protected]>

tracing/remotes: selftests: Add a test for the dmesg tracefs file

This commit adds a new selftest to exercise the newly introduced dmesg
tracefs file. It checks the ability to turn on and off 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..74f801a5e39ea
> --- /dev/null
> +++ b/tools/testing/selftests/ftrace/test.d/remotes/dmesg.tc

[ ... ]

> +    sleep 1
> +    output=$(mktemp $TMPDIR/remote_test.XXXXXX)
> +    dmesg | awk -v ts=$last_ts '
> +    /^\[ *[0-9]+\.[0-9]+\]/ {
> +        t=$0; sub(/^\[[ \t]*/, "", t); sub(/].*/, "", t);
> +        if (t+0 > ts+0) print $0
> +    }' | grep "selftest id=" | sed 's/^[^]]*] //'> $output

[Severity: Medium]
Does this implicitly depend on dmesg timestamps being enabled?

If the kernel is configured with CONFIG_PRINTK_TIME=n, or if the user-space
dmesg utility suppresses timestamps by default, the output won't have the
[ X.Y] prefix.

The awk script strictly expects this format and drops lines that do not
match the prefix. This would result in an empty temporary file. Because
the script executes with set -e, an empty file might cause the subsequent
check_trace validation to abort the test.

[ ... ]

> 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]
Will this unconditional write break existing remote tests when run on older
kernels?

Since this test suite frequently runs with set -e enabled in the caller
scripts, if the dmesg tracefs file does not exist on an older kernel, this
echo command will fail with a "No such file or directory" error and abort
the script.

Could we check if the dmesg file exists before attempting to write to it?

>  	clear_trace
>  	echo 7 > buffer_size_kb
>  	echo 0 > events/enable

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7