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 <[email protected]> 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/[email protected]?part=35
