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

[jim . cromie]

On Tue, Apr 15, 2025 at 4:05 AM Louis Chauvet <louis.chauvet@xxxxxxxxxxx> wrote:
>
>
>
> 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@xxxxxxxxx>
> > Co-developed-by: Łukasz Bartosik <ukaszb@xxxxxxxxxxxx>
> > Signed-off-by: Łukasz Bartosik <ukaszb@xxxxxxxxxxxx>
> > ---
> > - 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?

yes, thx. I will.

>
> >                       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
>
>