Re: [PATCH v8 35/43] dyndbg: split multi-query strings with @

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The selftest intended to verify the new '@' query separator mistakenly uses ';' instead, leaving the new feature untested.
- [Medium] Unconditional string splitting on '@' breaks valid queries containing literal '@' characters (e.g., in format strings for device tree addresses or emails).
--

commit 02bd61a3e52ea827cb76db337dc5c8fc31993564
Author: Jim Cromie <jim.cromie@gmail.com>

dyndbg: split multi-query strings with @

This patch adds '@' to the existing ';' and '\n' multi-command
separators, which is more shell-friendly, so you can more fully avoid
quoting and escaping hassles. It also updates the selftests script.

> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 6f700de9738c4..93a5a481c8b8c 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -631,7 +631,7 @@ static int ddebug_exec_queries(char *query, const char *modname)
>  	int i, errs = 0, exitcode = 0, rc, nfound = 0;
>  
>  	for (i = 0; query; query = split) {
> -		split = strpbrk(query, ";\n");
> +		split = strpbrk(query, "@;\n");

[Severity: Medium]
Will this unconditional string splitting on '@' break valid queries containing
literal '@' characters?

For example, format strings containing device tree addresses or emails like
dyndbg="format \"node@1000\" +p". It seems they will be incorrectly split at
the '@' character before quote-aware tokenization occurs, causing syntax
errors and failing to enable the intended debug statements.

>  		if (split)
>  			*split++ = '\0';

[ ... ]

> diff --git a/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
> index f0b18afa73722..0bb3c3e11df75 100755
> --- a/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
> +++ b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh

[ ... ]

> @@ -325,9 +325,19 @@ function FT_basic_queries {
>      ddcmd "module params +l"  'kernel/params.c'
>      ddcmd "module params -m"  'kernel/params.c'
>      ddcmd "module params =_"  'kernel/params.c'
> +}
> +
> +function FT_multi_query {
> +    v_echo "${GREEN}# MULTI_QUERY_TESTS ${NC}"
> +    if [ $LACK_DD_BUILTIN -eq 1 ]; then
> +	echo "SKIP - test requires params, which is a builtin module"
> +	return
> +    fi
> +    ddcmd =_ # zero everything
>  
> -    # multi-query commands split on ; on a single line
> +    # multi-query commands on a single line, split on ;/@ respectively
>      ddcmd "module params +mf ; module params func parse_args +sl"  'kernel/params.c'
> +    ddcmd "module params -f ; module params func parse_args -l"  'kernel/params.c'

[Severity: Medium]
Does this selftest command mistakenly use ';' instead of the intended '@'
separator?

The comment right above in FT_multi_query() indicates it tests splitting on
';/@ respectively', but both commands use ';', which means the new '@'
separator feature remains completely untested here.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260905-dd-cmap-part2-clean-v8-0-a4cc0674f6fd@gmail.com?part=35