[DEBUG 0/2] Disable debug conditionals

[DEBUG 0/2] Disable debug conditionals

[Glenn Washburn]

Here are two patches I've found useful when debugging grub issues. The first
patch allows the the $debug variable to contain conditionals prefixed with a
'-' to selectively disable that conditional when all conditionals have been
enabled. Only the first occurance of the conditional in the debug variable is
checked. So a value of "all,btrfs,alloc,-btrfs" will not disable btrfs debug
messages. A better implementation would do a string search for the conditional
from the end of the debug string, but grub doesn't have a string method to
search from the end of the string currently. I think despite its limitations,
the current patch is useful enough to be included, until takes an interest
in upgrading it.

The second patch adds the conditional to the debug message prefix. This is
especially useful in the context of the first patch and a situation where the
user is debugging a boot issue in a live grub and doesn't have the source
readily available (they are in GRUB on a non-booting system!)

Glenn

Glenn Washburn (2):
  misc: Allow selective disabling of debug conditionals
  misc: Add debug log condition to log output

 grub-core/kern/misc.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

-- 
2.27.0

[DEBUG 1/2] misc: Allow selective disabling of debug conditionals

[Glenn Washburn]

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

Note, only the first occurance of the conditional is searched for to
determine if the conditional is enabled. So simply appending ",-conditional"
to the $debug variable may not disable that conditional, if, for example,
the conditional is already present. To illustrate, the command
"set debug=all,btrfs,-scripting,-btrfs" will not disable btrfs.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/kern/misc.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
index 3af336ee2..ecef4839e 100644
--- a/grub-core/kern/misc.c
+++ b/grub-core/kern/misc.c
@@ -162,13 +162,28 @@ __attribute__ ((alias("grub_printf")));
 int
 grub_debug_enabled (const char * condition)
 {
-  const char *debug;
+  const char *debug, *found;
+  char found_end;
 
   debug = grub_env_get ("debug");
   if (!debug)
     return 0;
 
-  if (grub_strword (debug, "all") || grub_strword (debug, condition))
+  if (grub_strword (debug, "all"))
+    {
+      if (debug[3] == '\0')
+	return 1;
+      found = grub_strstr (debug, condition);
+      /* If found conditional is preceeded by a '-' and whole, disable debug */
+      if (found > debug && *(found-1) == '-')
+	{
+	  found_end = *(found + grub_strlen (condition));
+	  if (found_end == '\0' || grub_iswordseparator (found_end))
+	    return 0;
+	}
+      return 1;
+    }
+  else if (grub_strword (debug, condition))
     return 1;
 
   return 0;
-- 
2.27.0

[DEBUG 2/2] misc: Add debug log condition to log output

[Glenn Washburn]

Adding the conditional to debug log messages allows the grub user to
construct the $debug variable without needing to consult the source to
find the conditional (especially useful for situations where the source
is not readily available).

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/kern/misc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
index ecef4839e..17a907b63 100644
--- a/grub-core/kern/misc.c
+++ b/grub-core/kern/misc.c
@@ -197,7 +197,7 @@ grub_real_dprintf (const char *file, const int line, const char *condition,
 
   if (grub_debug_enabled (condition))
     {
-      grub_printf ("%s:%d: ", file, line);
+      grub_printf ("%s:%d:%s: ", file, line, condition);
       va_start (args, fmt);
       grub_vprintf (fmt, args);
       va_end (args);
-- 
2.27.0

Re: [DEBUG 1/2] misc: Allow selective disabling of debug conditionals

[Michael Schierl]

Hello,


Am 19.10.2021 um 08:47 schrieb Glenn Washburn:
>
> Note, only the first occurance of the conditional is searched for to
> determine if the conditional is enabled. So simply appending ",-conditional"
> to the $debug variable may not disable that conditional, if, for example,
> the conditional is already present. To illustrate, the command
> "set debug=all,btrfs,-scripting,-btrfs" will not disable btrfs.

A sligtly bigger problem of the implementation is that


set debug=all,-cryptodisk,-crypt,-disk

will only exclude cryptodisk, as the other two are found as substrings
first. Same for e.g. fs,xfs or ata,pata.

But it is probably the best compromise (both performance- and code-size
wise) when considering grub's built-in string functions, when parsing
this exact command syntax.

As there is no real advantage of both including and excluding
conditionals (with the exception of "all"), a pragmatic approach may be
to change the syntax, so that instead of

set debug=all,-scripting,-btrfs

one has to specify

set debug=-scripting,btrfs

(i.e. if the first character of the variable is a -, the rest is a list
of excluded debug conditionals). So after comparing the first character,
you can skip it and use grub_strword on the rest.

But probably I'm missing something simple. :)


Regards,


Michael

Re: [DEBUG 1/2] misc: Allow selective disabling of debug conditionals

[Glenn Washburn]

On Tue, 19 Oct 2021 21:10:27 +0200
Michael Schierl <schierlm@gmx.de> wrote:

> 
> Hello,
> 
> 
> Am 19.10.2021 um 08:47 schrieb Glenn Washburn:
> >
> > Note, only the first occurance of the conditional is searched for to
> > determine if the conditional is enabled. So simply appending ",-conditional"
> > to the $debug variable may not disable that conditional, if, for example,
> > the conditional is already present. To illustrate, the command
> > "set debug=all,btrfs,-scripting,-btrfs" will not disable btrfs.
> 
> A sligtly bigger problem of the implementation is that
> 
> 
> set debug=all,-cryptodisk,-crypt,-disk
> 
> will only exclude cryptodisk, as the other two are found as substrings
> first. Same for e.g. fs,xfs or ata,pata.

Hmm, good catch.

> 
> But it is probably the best compromise (both performance- and code-size
> wise) when considering grub's built-in string functions, when parsing
> this exact command syntax.

Yes, I was trying to avoid a loop, but looks like it really should have
one. I think this patch needs another version.

> 
> As there is no real advantage of both including and excluding
> conditionals (with the exception of "all"), a pragmatic approach may be
> to change the syntax, so that instead of
> 
> set debug=all,-scripting,-btrfs
> 
> one has to specify
> 
> set debug=-scripting,btrfs
> 
> (i.e. if the first character of the variable is a -, the rest is a list
> of excluded debug conditionals). So after comparing the first character,
> you can skip it and use grub_strword on the rest.

This is not a bad idea, and I like that it makes things simpler. I
don't feel great about the implied "all" though and I think the syntax
may be confusing for the user at first. Performance wise, I'd say its
about the same as me adding a loop to this patch.

I'm not opposed to this, but I'd want Daniel or others
to chime in on their thoughts.

Thanks for taking a look!
Glenn

Re: [DEBUG 2/2] misc: Add debug log condition to log output

[Daniel Kiper]

On Tue, Oct 19, 2021 at 01:47:03AM -0500, Glenn Washburn wrote:
> Adding the conditional to debug log messages allows the grub user to
> construct the $debug variable without needing to consult the source to
> find the conditional (especially useful for situations where the source
> is not readily available).
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [DEBUG 1/2] misc: Allow selective disabling of debug conditionals

[Daniel Kiper]

On Tue, Oct 19, 2021 at 03:45:32PM -0500, Glenn Washburn wrote:
> On Tue, 19 Oct 2021 21:10:27 +0200
> Michael Schierl <schierlm@gmx.de> wrote:
>
> > Hello,
> >
> > Am 19.10.2021 um 08:47 schrieb Glenn Washburn:
> > >
> > > Note, only the first occurance of the conditional is searched for to
> > > determine if the conditional is enabled. So simply appending ",-conditional"
> > > to the $debug variable may not disable that conditional, if, for example,
> > > the conditional is already present. To illustrate, the command
> > > "set debug=all,btrfs,-scripting,-btrfs" will not disable btrfs.
> >
> > A sligtly bigger problem of the implementation is that
> >
> > set debug=all,-cryptodisk,-crypt,-disk
> >
> > will only exclude cryptodisk, as the other two are found as substrings
> > first. Same for e.g. fs,xfs or ata,pata.
>
> Hmm, good catch.
>
> > But it is probably the best compromise (both performance- and code-size
> > wise) when considering grub's built-in string functions, when parsing
> > this exact command syntax.
>
> Yes, I was trying to avoid a loop, but looks like it really should have
> one. I think this patch needs another version.
>
> > As there is no real advantage of both including and excluding
> > conditionals (with the exception of "all"), a pragmatic approach may be
> > to change the syntax, so that instead of
> >
> > set debug=all,-scripting,-btrfs

I prefer this syntax...

> > one has to specify
> >
> > set debug=-scripting,btrfs
> >
> > (i.e. if the first character of the variable is a -, the rest is a list
> > of excluded debug conditionals). So after comparing the first character,
> > you can skip it and use grub_strword on the rest.
>
> This is not a bad idea, and I like that it makes things simpler. I
> don't feel great about the implied "all" though and I think the syntax
> may be confusing for the user at first. Performance wise, I'd say its
> about the same as me adding a loop to this patch.
>
> I'm not opposed to this, but I'd want Daniel or others
> to chime in on their thoughts.

I am OK with the idea until it does not impact performance strongly.
Though it would be nice if last specified conditional is in force,
e.g. "all,-btrfs,btrfs" should mean btrfs logging is enabled.

Additionally, please do not forget to update docs next time.

Daniel