| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJfuBxxR9GZwRmVCuu=at2RUXT_pUWHrG4V61G+WjQSKJnh2Fg@mail.gmail.com>
Date: Tue, 15 Apr 2025 14:17:07 -0600
From: jim.cromie@...il.com
To: Louis Chauvet <louis.chauvet@...tlin.com>
Cc: jbaron@...mai.com, gregkh@...uxfoundation.org, ukaszb@...omium.org,
linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
amd-gfx@...ts.freedesktop.org, intel-gvt-dev@...ts.freedesktop.org,
intel-gfx@...ts.freedesktop.org, daniel.vetter@...ll.ch,
tvrtko.ursulin@...ux.intel.com, jani.nikula@...el.com,
ville.syrjala@...ux.intel.com
Subject: Re: [PATCH v3 23/54] dyndbg: treat comma as a token separator
On Tue, Apr 15, 2025 at 4:05 AM Louis Chauvet <louis.chauvet@...tlin.com> 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@...il.com>
> > Co-developed-by: Łukasz Bartosik <ukaszb@...omium.org>
> > Signed-off-by: Łukasz Bartosik <ukaszb@...omium.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?
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
>
>
Powered by blists - more mailing lists