Re: [PATCH v3 23/54] dyndbg: treat comma as a token separator

[Louis Chauvet <louis.chauvet@bootlin.com>]

Le 02/04/2025 à 19:41, Jim Cromie a écrit :
> Treat comma as a token terminator, just like a space.  This allows a
> user to avoid quoting hassles when spaces are otherwise needed:
> 
>   :#> modprobe drm dyndbg=class,DRM_UT_CORE,+p\;class,DRM_UT_KMS,+p
> 
> or as a boot arg:
> 
>   drm.dyndbg=class,DRM_UT_CORE,+p  # todo: support multi-query here
> 
> Given the many ways a boot-line +args can be assembled and then passed
> in/down/around shell based tools, this may allow side-stepping all
> sorts of quoting hassles thru those layers.
> 
> existing query format:
> 
>   modprobe test_dynamic_debug dyndbg="class D2_CORE +p"
> 
> new format:
> 
>   modprobe test_dynamic_debug dyndbg=class,D2_CORE,+p
> 
> ALSO
> 
> selftests-dyndbg: add comma_terminator_tests
> 
> New fn validates parsing and effect of queries using combinations of
> commas and spaces to delimit the tokens.
> 
> It manipulates pr-debugs in builtin module/params, so might have deps
> I havent foreseen on odd configurations.
> 
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> Co-developed-by: Łukasz Bartosik <ukaszb@chromium.org>
> Signed-off-by: Łukasz Bartosik <ukaszb@chromium.org>
> ---
> - skip comma tests if no builtins
> -v3 squash in tests and doc
> ---
>   .../admin-guide/dynamic-debug-howto.rst       |  9 +++++---
>   lib/dynamic_debug.c                           | 17 +++++++++++----
>   .../dynamic_debug/dyndbg_selftest.sh          | 21 ++++++++++++++++++-
>   3 files changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
> index 63a511f2337b..e2dbb5d9b314 100644
> --- a/Documentation/admin-guide/dynamic-debug-howto.rst
> +++ b/Documentation/admin-guide/dynamic-debug-howto.rst
> @@ -78,11 +78,12 @@ Command Language Reference
>   ==========================
>   
>   At the basic lexical level, a command is a sequence of words separated
> -by spaces or tabs.  So these are all equivalent::
> +by spaces, tabs, or commas.  So these are all equivalent::
>   
>     :#> ddcmd file svcsock.c line 1603 +p
>     :#> ddcmd "file svcsock.c line 1603 +p"
>     :#> ddcmd '  file   svcsock.c     line  1603 +p  '
> +  :#> ddcmd file,svcsock.c,line,1603,+p
>   
>   Command submissions are bounded by a write() system call.
>   Multiple commands can be written together, separated by ``;`` or ``\n``::
> @@ -167,9 +168,11 @@ module
>       The given string is compared against the module name
>       of each callsite.  The module name is the string as
>       seen in ``lsmod``, i.e. without the directory or the ``.ko``
> -    suffix and with ``-`` changed to ``_``.  Examples::
> +    suffix and with ``-`` changed to ``_``.
>   
> -	module sunrpc
> +    Examples::
> +
> +	module,sunrpc	# with ',' as token separator
>   	module nfsd
>   	module drm*	# both drm, drm_kms_helper
>   
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 0d603caadef8..5737f1b4eba8 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -299,6 +299,14 @@ static int ddebug_change(const struct ddebug_query *query, struct flag_settings
>   	return nfound;
>   }
>   
> +static char *skip_spaces_and_commas(const char *str)
> +{
> +	str = skip_spaces(str);
> +	while (*str == ',')
> +		str = skip_spaces(++str);
> +	return (char *)str;
> +}
> +
>   /*
>    * Split the buffer `buf' into space-separated words.
>    * Handles simple " and ' quoting, i.e. without nested,
> @@ -312,8 +320,8 @@ static int ddebug_tokenize(char *buf, char *words[], int maxwords)
>   	while (*buf) {
>   		char *end;
>   
> -		/* Skip leading whitespace */
> -		buf = skip_spaces(buf);
> +		/* Skip leading whitespace and comma */
> +		buf = skip_spaces_and_commas(buf);
>   		if (!*buf)
>   			break;	/* oh, it was trailing whitespace */
>   		if (*buf == '#')
> @@ -329,7 +337,7 @@ static int ddebug_tokenize(char *buf, char *words[], int maxwords)
>   				return -EINVAL;	/* unclosed quote */
>   			}
>   		} else {
> -			for (end = buf; *end && !isspace(*end); end++)
> +			for (end = buf; *end && !isspace(*end) && *end != ','; end++)
>   				;

Why don't you use the skip_spaces_and_commas here?

>   			if (end == buf) {
>   				pr_err("parse err after word:%d=%s\n", nwords,
> @@ -601,7 +609,8 @@ static int ddebug_exec_queries(char *query, const char *modname)
>   		if (split)
>   			*split++ = '\0';
>   
> -		query = skip_spaces(query);
> +		query = skip_spaces_and_commas(query);
> +
>   		if (!query || !*query || *query == '#')
>   			continue;
>   
> diff --git a/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
> index 465fad3f392c..c7bf521f36ee 100755
> --- a/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
> +++ b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
> @@ -216,7 +216,7 @@ function check_err_msg() {
>   function basic_tests {
>       echo -e "${GREEN}# BASIC_TESTS ${NC}"
>       if [ $LACK_DD_BUILTIN -eq 1 ]; then
> -	echo "SKIP"
> +	echo "SKIP - test requires params, which is a builtin module"
>   	return
>       fi
>       ddcmd =_ # zero everything
> @@ -238,8 +238,27 @@ EOF
>       ddcmd =_
>   }
>   
> +function comma_terminator_tests {
> +    echo -e "${GREEN}# COMMA_TERMINATOR_TESTS ${NC}"
> +    if [ $LACK_DD_BUILTIN -eq 1 ]; then
> +	echo "SKIP - test requires params, which is a builtin module"
> +	return
> +    fi
> +    # try combos of spaces & commas
> +    check_match_ct '\[params\]' 4 -r
> +    ddcmd module,params,=_		# commas as spaces
> +    ddcmd module,params,+mpf		# turn on module's pr-debugs
> +    check_match_ct =pmf 4
> +    ddcmd ,module ,, ,  params, -p
> +    check_match_ct =mf 4
> +    ddcmd " , module ,,, ,  params, -m"	#
> +    check_match_ct =f 4
> +    ddcmd =_
> +}
> +
>   tests_list=(
>       basic_tests
> +    comma_terminator_tests
>   )
>   
>   # Run tests

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com