[PATCH v3] misc: Allow selective disabling of debug facility names

[PATCH v3] misc: Allow selective disabling of debug facility names

[Glenn Washburn]

Sometimes you know only know which debug logging facility names you want to
turn off, not necessarily all the ones you want enabled. This patch allows
the debug string to contain facility names in the $debug variable which are
prefixed with a "-" to disable debug log messages for that conditional. Say
you want all debug logging on except for btrfs and scripting, then do:
 "set debug=all,-btrfs,-scripting"

Note, that only the last occurence of the facility name with or without a
leading "-" is considered. So simply appending ",-facilityname" to the
$debug variable will disable that conditional. To illustrate, the command
"set debug=all,-btrfs,-scripting,btrfs" will enable btrfs.

Also, add documentation explaining this new behavior.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
Changes since v2:
* Fix issue where a facility at the start of the string wouldn't be matched

---
Range-diff against v2:
1:  ef2662d0b ! 1:  31439a7fb misc: Allow selective disabling of debug facility names
    @@ grub-core/kern/misc.c: __attribute__ ((alias("grub_printf")));
     -    return 1;
     +  if (grub_strword (debug, "all"))
     +    {
    -+      ret = 1;
     +      if (debug[3] == '\0')
     +	return 1;
    ++      ret = 1;
     +    }
      
     -  return 0;
     +  clen = grub_strlen (condition);
    -+  found = debug;
    ++  found = debug-1;
     +  while(1)
     +    {
     +      found = grub_strstr (found+1, condition);
    @@ grub-core/kern/misc.c: __attribute__ ((alias("grub_printf")));
     +	continue;
     +
     +      /*
    -+       * If found condition is prefixed with '-' and the start is on a word
    ++       * If found condition is at the start of debug, then enable debug. Else
    ++       * if found condition is prefixed with '-' and the start is on a word
     +       * boundary, then disable debug. Otherwise, if the start is on a word
    -+       * boundary, enable debug. If neither, ignore.
    ++       * boundary, enable debug. If none of these cases, ignore.
     +       */
    -+      if (*(found-1) == '-' && ((found == debug + 1) || (*(found-2) == ','
    ++      if (found == debug)
    ++	ret = 1;
    ++      else if (*(found-1) == '-' && ((found == debug + 1) || (*(found-2) == ','
     +			       || grub_isspace (*(found-2)))))
     +	ret = 0;
     +      else if (*(found-1) == ',' || grub_isspace (*(found-1)))

 docs/grub.texi        | 13 ++++++++++---
 grub-core/kern/misc.c | 43 +++++++++++++++++++++++++++++++++++++++----
 2 files changed, 49 insertions(+), 7 deletions(-)

diff --git a/docs/grub.texi b/docs/grub.texi
index 99d0a0149..d13aa6600 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -3388,9 +3388,16 @@ processed by commands @command{configfile} (@pxref{configfile}) or @command{norm
 @subsection debug
 
 This variable may be set to enable debugging output from various components
-of GRUB.  The value is a list of debug facility names separated by
-whitespace or @samp{,}, or @samp{all} to enable all available debugging
-output. The facility names are the first argument to grub_dprintf. Consult
+of GRUB. The value is an ordered list of debug facility names separated by
+whitespace or @samp{,}. If the special facility names @samp{all} is present
+then debugging output of all facility names is enabled at the start of
+processing the value of this variable. A facility's debug output can then be
+disabled by prefixing its name with a @samp{-}. The last occurence facility
+name with or without a leading @samp{-} takes precendent over any previous
+occurence. This allows the easy enabling or disabling of facilities by
+appending a @samp{,} and then the facility name with or without the leading
+@samp{-}, which will preserve the state of the rest of the facilities.
+The facility names are the first argument to grub_dprintf. Consult the
 source for more details.
 
 
diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
index 11b8592c8..fda69fe00 100644
--- a/grub-core/kern/misc.c
+++ b/grub-core/kern/misc.c
@@ -162,16 +162,51 @@ __attribute__ ((alias("grub_printf")));
 int
 grub_debug_enabled (const char * condition)
 {
-  const char *debug;
+  const char *debug, *found;
+  grub_size_t clen;
+  int ret = 0;
 
   debug = grub_env_get ("debug");
   if (!debug)
     return 0;
 
-  if (grub_strword (debug, "all") || grub_strword (debug, condition))
-    return 1;
+  if (grub_strword (debug, "all"))
+    {
+      if (debug[3] == '\0')
+	return 1;
+      ret = 1;
+    }
 
-  return 0;
+  clen = grub_strlen (condition);
+  found = debug-1;
+  while(1)
+    {
+      found = grub_strstr (found+1, condition);
+
+      if (found == NULL)
+	break;
+
+      /* Found condition is not a whole word, so ignore it */
+      if (*(found + clen) != '\0' && *(found + clen) != ','
+	 && !grub_isspace (*(found + clen)))
+	continue;
+
+      /*
+       * If found condition is at the start of debug, then enable debug. Else
+       * if found condition is prefixed with '-' and the start is on a word
+       * boundary, then disable debug. Otherwise, if the start is on a word
+       * boundary, enable debug. If none of these cases, ignore.
+       */
+      if (found == debug)
+	ret = 1;
+      else if (*(found-1) == '-' && ((found == debug + 1) || (*(found-2) == ','
+			       || grub_isspace (*(found-2)))))
+	ret = 0;
+      else if (*(found-1) == ',' || grub_isspace (*(found-1)))
+	ret = 1;
+    }
+
+  return ret;
 }
 
 void
-- 
2.27.0

Re: [PATCH v3] misc: Allow selective disabling of debug facility names

[Michael Schierl]

Hello Glenn,


Am 10.12.2021 um 09:13 schrieb Glenn Washburn:
> Sometimes you know only know which debug logging facility names you want to
> turn off, not necessarily all the ones you want enabled. This patch allows
> the debug string to contain facility names in the $debug variable which are
> prefixed with a "-" to disable debug log messages for that conditional. Say
> you want all debug logging on except for btrfs and scripting, then do:
>   "set debug=all,-btrfs,-scripting"
>
> Note, that only the last occurence of the facility name with or without a
> leading "-" is considered. So simply appending ",-facilityname" to the
> $debug variable will disable that conditional. To illustrate, the command
> "set debug=all,-btrfs,-scripting,btrfs" will enable btrfs.
>
> Also, add documentation explaining this new behavior.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>


LGTM.


Regards,

Michael

Re: [PATCH v3] misc: Allow selective disabling of debug facility names

[Daniel Kiper]

On Fri, Dec 10, 2021 at 02:13:52AM -0600, Glenn Washburn wrote:
> Sometimes you know only know which debug logging facility names you want to

s/only know/only/

> turn off, not necessarily all the ones you want enabled. This patch allows
> the debug string to contain facility names in the $debug variable which are
> prefixed with a "-" to disable debug log messages for that conditional. Say
> you want all debug logging on except for btrfs and scripting, then do:
>  "set debug=all,-btrfs,-scripting"
>
> Note, that only the last occurence of the facility name with or without a
> leading "-" is considered. So simply appending ",-facilityname" to the
> $debug variable will disable that conditional. To illustrate, the command
> "set debug=all,-btrfs,-scripting,btrfs" will enable btrfs.
>
> Also, add documentation explaining this new behavior.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
> Changes since v2:
> * Fix issue where a facility at the start of the string wouldn't be matched
>
> ---
> Range-diff against v2:
> 1:  ef2662d0b ! 1:  31439a7fb misc: Allow selective disabling of debug facility names
>     @@ grub-core/kern/misc.c: __attribute__ ((alias("grub_printf")));
>      -    return 1;
>      +  if (grub_strword (debug, "all"))
>      +    {
>     -+      ret = 1;
>      +      if (debug[3] == '\0')
>      +	return 1;
>     ++      ret = 1;
>      +    }
>
>      -  return 0;
>      +  clen = grub_strlen (condition);
>     -+  found = debug;
>     ++  found = debug-1;
>      +  while(1)
>      +    {
>      +      found = grub_strstr (found+1, condition);
>     @@ grub-core/kern/misc.c: __attribute__ ((alias("grub_printf")));
>      +	continue;
>      +
>      +      /*
>     -+       * If found condition is prefixed with '-' and the start is on a word
>     ++       * If found condition is at the start of debug, then enable debug. Else
>     ++       * if found condition is prefixed with '-' and the start is on a word
>      +       * boundary, then disable debug. Otherwise, if the start is on a word
>     -+       * boundary, enable debug. If neither, ignore.
>     ++       * boundary, enable debug. If none of these cases, ignore.
>      +       */
>     -+      if (*(found-1) == '-' && ((found == debug + 1) || (*(found-2) == ','
>     ++      if (found == debug)
>     ++	ret = 1;
>     ++      else if (*(found-1) == '-' && ((found == debug + 1) || (*(found-2) == ','
>      +			       || grub_isspace (*(found-2)))))
>      +	ret = 0;
>      +      else if (*(found-1) == ',' || grub_isspace (*(found-1)))
>
>  docs/grub.texi        | 13 ++++++++++---
>  grub-core/kern/misc.c | 43 +++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 49 insertions(+), 7 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 99d0a0149..d13aa6600 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -3388,9 +3388,16 @@ processed by commands @command{configfile} (@pxref{configfile}) or @command{norm
>  @subsection debug
>
>  This variable may be set to enable debugging output from various components
> -of GRUB.  The value is a list of debug facility names separated by
> -whitespace or @samp{,}, or @samp{all} to enable all available debugging
> -output. The facility names are the first argument to grub_dprintf. Consult
> +of GRUB. The value is an ordered list of debug facility names separated by
> +whitespace or @samp{,}. If the special facility names @samp{all} is present

s/names/name/

> +then debugging output of all facility names is enabled at the start of
> +processing the value of this variable. A facility's debug output can then be
> +disabled by prefixing its name with a @samp{-}. The last occurence facility
> +name with or without a leading @samp{-} takes precendent over any previous
> +occurence. This allows the easy enabling or disabling of facilities by
> +appending a @samp{,} and then the facility name with or without the leading
> +@samp{-}, which will preserve the state of the rest of the facilities.
> +The facility names are the first argument to grub_dprintf. Consult the
>  source for more details.
>
>
> diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
> index 11b8592c8..fda69fe00 100644
> --- a/grub-core/kern/misc.c
> +++ b/grub-core/kern/misc.c
> @@ -162,16 +162,51 @@ __attribute__ ((alias("grub_printf")));
>  int
>  grub_debug_enabled (const char * condition)
>  {
> -  const char *debug;
> +  const char *debug, *found;
> +  grub_size_t clen;
> +  int ret = 0;
>
>    debug = grub_env_get ("debug");
>    if (!debug)
>      return 0;
>
> -  if (grub_strword (debug, "all") || grub_strword (debug, condition))
> -    return 1;
> +  if (grub_strword (debug, "all"))
> +    {
> +      if (debug[3] == '\0')
> +	return 1;
> +      ret = 1;
> +    }
>
> -  return 0;
> +  clen = grub_strlen (condition);
> +  found = debug-1;
> +  while(1)
> +    {
> +      found = grub_strstr (found+1, condition);
> +
> +      if (found == NULL)
> +	break;
> +
> +      /* Found condition is not a whole word, so ignore it */
> +      if (*(found + clen) != '\0' && *(found + clen) != ','
> +	 && !grub_isspace (*(found + clen)))
> +	continue;
> +
> +      /*
> +       * If found condition is at the start of debug, then enable debug. Else
> +       * if found condition is prefixed with '-' and the start is on a word
> +       * boundary, then disable debug. Otherwise, if the start is on a word
> +       * boundary, enable debug. If none of these cases, ignore.
> +       */
> +      if (found == debug)
> +	ret = 1;
> +      else if (*(found-1) == '-' && ((found == debug + 1) || (*(found-2) == ','
> +			       || grub_isspace (*(found-2)))))
> +	ret = 0;
> +      else if (*(found-1) == ',' || grub_isspace (*(found-1)))

I think this could be merged with first if.

Additionally, please add missing spaces around "+" and "-" operators.

> +	ret = 1;
> +    }
> +
> +  return ret;
>  }

Daniel