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

[PATCH v2] 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>
---
 docs/grub.texi        | 13 ++++++++++---
 grub-core/kern/misc.c | 40 ++++++++++++++++++++++++++++++++++++----
 2 files changed, 46 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..9f676b73e 100644
--- a/grub-core/kern/misc.c
+++ b/grub-core/kern/misc.c
@@ -162,16 +162,48 @@ __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"))
+    {
+      ret = 1;
+      if (debug[3] == '\0')
+	return 1;
+    }
 
-  return 0;
+  clen = grub_strlen (condition);
+  found = debug;
+  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 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.
+       */
+      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 v2] misc: Allow selective disabling of debug facility names

[Michael Schierl]

Hello Glenn,

Comments below, note that I did not test the patch so maybe I am missing
something.

Am 06.12.2021 um 18:03 schrieb Glenn Washburn:

>   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"))
> +    {
> +      ret = 1;
> +      if (debug[3] == '\0')
> +	return 1;

maybe move the conditional before the assignment of ret?

> +    }
>
> -  return 0;
> +  clen = grub_strlen (condition);
> +  found = debug;
> +  while(1)
> +    {
> +      found = grub_strstr (found+1, condition);

Off by one error: in case the condition is the first one in debug, it
won't be found. And after fixing this...

> +
> +      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 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.
> +       */
> +      if (*(found-1) == '-' && ((found == debug + 1) || (*(found-2) == ','
> +			       || grub_isspace (*(found-2)))))

you will read beyond the start of the string here.

> +	ret = 0;
> +      else if (*(found-1) == ',' || grub_isspace (*(found-1)))

What about the other separators in grub_iswordseparator besides comma?



Regards,


Michael

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

[Glenn Washburn]

Hi Michael,

Thanks for taking a look at this.

On Mon, 6 Dec 2021 22:01:22 +0100
Michael Schierl <schierlm@gmx.de> wrote:

> 
> Hello Glenn,
> 
> Comments below, note that I did not test the patch so maybe I am missing
> something.
> 
> Am 06.12.2021 um 18:03 schrieb Glenn Washburn:
> 
> >   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"))
> > +    {
> > +      ret = 1;
> > +      if (debug[3] == '\0')
> > +	return 1;
> 
> maybe move the conditional before the assignment of ret?

I'm understanding you to be suggesting to move the assignment of ret to
after the if statement that follows it. The only point I see is saving
an assignment in the case that debug=all. Is there more to it?

> > +    }
> >
> > -  return 0;
> > +  clen = grub_strlen (condition);
> > +  found = debug;
> > +  while(1)
> > +    {
> > +      found = grub_strstr (found+1, condition);
> 
> Off by one error: in case the condition is the first one in debug, it
> won't be found. And after fixing this...

Yep, this is an issue. It was intentional when I had the code only
running in the case "all" was present, in which case skipping the
condition if its the at the start of $debug is fine because the
condition is already enabled. After changing to run also without "all"
being present, this becomes an issue.

> > +
> > +      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 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.
> > +       */
> > +      if (*(found-1) == '-' && ((found == debug + 1) || (*(found-2) == ','
> > +			       || grub_isspace (*(found-2)))))
> 
> you will read beyond the start of the string here.

Yep, that's why I had the +1 above to account for this.

> > +	ret = 0;
> > +      else if (*(found-1) == ',' || grub_isspace (*(found-1)))
> 
> What about the other separators in grub_iswordseparator besides comma?

I was using grub_iswordseparator in the original patch, but decided
against it this round because the documentation states that separators
are whitespace or comma.

Glenn

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

[Michael Schierl]

Hello Glenn,


Am 07.12.2021 um 10:17 schrieb Glenn Washburn:

>> maybe move the conditional before the assignment of ret?
>
> I'm understanding you to be suggesting to move the assignment of ret to
> after the if statement that follows it.

Correct.

> The only point I see is saving
> an assignment in the case that debug=all. Is there more to it?

No. It just bugged me to do a definitely never used assignment directly
before returning. Feel free to ignore it.


> Yep, this is an issue. It was intentional when I had the code only
> running in the case "all" was present, in which case skipping the
> condition if its the at the start of $debug is fine because the
> condition is already enabled. After changing to run also without "all"
> being present, this becomes an issue.

If "all" still was present I would probably have complained about
initializing it to +1 instead of +3 as the first three characters cannot
be a match if they are "all". :)

> I was using grub_iswordseparator in the original patch, but decided
> against it this round because the documentation states that separators
> are whitespace or comma.

Fair point. I don't think there are any common cases where this
backward-incompatible change really matters.


Regards,


Michael

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

[Glenn Washburn]

On Tue, 7 Dec 2021 22:01:14 +0100
Michael Schierl <schierlm@gmx.de> wrote:

> Hello Glenn,
> 
> 
> Am 07.12.2021 um 10:17 schrieb Glenn Washburn:
> 
> >> maybe move the conditional before the assignment of ret?
> >
> > I'm understanding you to be suggesting to move the assignment of ret to
> > after the if statement that follows it.
> 
> Correct.
> 
> > The only point I see is saving
> > an assignment in the case that debug=all. Is there more to it?
> 
> No. It just bugged me to do a definitely never used assignment directly
> before returning. Feel free to ignore it.

I'll think about this. Originally, I didn't think it looked as nice, but
on second look it seems fine.

> > Yep, this is an issue. It was intentional when I had the code only
> > running in the case "all" was present, in which case skipping the
> > condition if its the at the start of $debug is fine because the
> > condition is already enabled. After changing to run also without "all"
> > being present, this becomes an issue.
> 
> If "all" still was present I would probably have complained about
> initializing it to +1 instead of +3 as the first three characters cannot
> be a match if they are "all". :)

Yes, but I didn't want to assume that "all" is the first item in the
list. I decided that just the presence of an "all" item any where in
the list of conditionals enables all conditionals at the beginning of
processing. So debug="-btrfs,all" is the same as debug="all,-btrfs" and
means all of the debug conditionals are turned on except btrfs.

I decided that it doesn't make as much sense to have the position of
"all" matter, and effectively be a macro for all conditionals, because
at that point why append ",all,...", just set debug to "all,..."
erasing the previous contents.

I'm open to suggestions on what the preferred way to handle this might
be. We could say that "all" must appear at the beginning, but that
seems unnecessary and overly restrictive. What's less clear to me is
what situation I might want to put have "all" not at the beginning of
the string. I'm thinking there might be one, which is why I'm leaving
that as an option, but maybe I'm wrong.

> 
> > I was using grub_iswordseparator in the original patch, but decided
> > against it this round because the documentation states that separators
> > are whitespace or comma.
> 
> Fair point. I don't think there are any common cases where this
> backward-incompatible change really matters.

Yeah, and its undocumented that other seperators would work.

Glenn

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

[Michael Schierl]

Hello Glenn,


Am 07.12.2021 um 22:59 schrieb Glenn Washburn:
> Yes, but I didn't want to assume that "all" is the first item in the
> list.

More important, "all" does not have to be present at all.

DEBUG=fat,btrfs,-fat

should be perfectly legal (and it is).

> I decided that it doesn't make as much sense to have the position of
> "all" matter, and effectively be a macro for all conditionals, because
> at that point why append ",all,...", just set debug to "all,..."
> erasing the previous contents.

That is a fair point. Same applies to "-all" (just erase everything in
the string before it).

Instead of

DEBUG=btrfs,all,fat,-all,ext2

probably the user just wanted to enter

DEBUG=ext2

> I'm open to suggestions on what the preferred way to handle this might
> be. We could say that "all" must appear at the beginning, but that
> seems unnecessary and overly restrictive. What's less clear to me is
> what situation I might want to put have "all" not at the beginning of
> the string. I'm thinking there might be one, which is why I'm leaving
> that as an option, but maybe I'm wrong.

I cannot think of any sensible one that would not confuse the user, like
your example with

DEBUG=-btrfs,all,-fat

But I don't think we have to put too much effort in it, especially since
the code should also work in case "all" is not present at all.


Regards,


Michael

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

[Glenn Washburn]

On Tue, 7 Dec 2021 23:07:32 +0100
Michael Schierl <schierlm@gmx.de> wrote:

> 
> Hello Glenn,
> 
> 
> Am 07.12.2021 um 22:59 schrieb Glenn Washburn:
> > Yes, but I didn't want to assume that "all" is the first item in the
> > list.
> 
> More important, "all" does not have to be present at all.
> 
> DEBUG=fat,btrfs,-fat
> 
> should be perfectly legal (and it is).

Agreed.

> > I decided that it doesn't make as much sense to have the position of
> > "all" matter, and effectively be a macro for all conditionals, because
> > at that point why append ",all,...", just set debug to "all,..."
> > erasing the previous contents.
> 
> That is a fair point. Same applies to "-all" (just erase everything in
> the string before it).
> 
> Instead of
> 
> DEBUG=btrfs,all,fat,-all,ext2
> 
> probably the user just wanted to enter
> 
> DEBUG=ext2

Yep.

> > I'm open to suggestions on what the preferred way to handle this might
> > be. We could say that "all" must appear at the beginning, but that
> > seems unnecessary and overly restrictive. What's less clear to me is
> > what situation I might want to put have "all" not at the beginning of
> > the string. I'm thinking there might be one, which is why I'm leaving
> > that as an option, but maybe I'm wrong.
> 
> I cannot think of any sensible one that would not confuse the user, like
> your example with
> 
> DEBUG=-btrfs,all,-fat
> 
> But I don't think we have to put too much effort in it, especially since
> the code should also work in case "all" is not present at all.

The other thing is that the original code allows "all" to be any item
in the string. So not allowing that would break backward-compatibility
(maybe not a big deal though). And I also agree that it can be
confusing. Maybe Daniel has an opinion.

Glenn

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

[Daniel Kiper]

On Tue, Dec 07, 2021 at 06:27:26PM -0600, Glenn Washburn wrote:
> On Tue, 7 Dec 2021 23:07:32 +0100
> Michael Schierl <schierlm@gmx.de> wrote:
>
> > Hello Glenn,
> >
> >
> > Am 07.12.2021 um 22:59 schrieb Glenn Washburn:
> > > Yes, but I didn't want to assume that "all" is the first item in the
> > > list.
> >
> > More important, "all" does not have to be present at all.
> >
> > DEBUG=fat,btrfs,-fat
> >
> > should be perfectly legal (and it is).
>
> Agreed.
>
> > > I decided that it doesn't make as much sense to have the position of
> > > "all" matter, and effectively be a macro for all conditionals, because
> > > at that point why append ",all,...", just set debug to "all,..."
> > > erasing the previous contents.
> >
> > That is a fair point. Same applies to "-all" (just erase everything in
> > the string before it).
> >
> > Instead of
> >
> > DEBUG=btrfs,all,fat,-all,ext2
> >
> > probably the user just wanted to enter
> >
> > DEBUG=ext2
>
> Yep.
>
> > > I'm open to suggestions on what the preferred way to handle this might
> > > be. We could say that "all" must appear at the beginning, but that
> > > seems unnecessary and overly restrictive. What's less clear to me is
> > > what situation I might want to put have "all" not at the beginning of
> > > the string. I'm thinking there might be one, which is why I'm leaving
> > > that as an option, but maybe I'm wrong.
> >
> > I cannot think of any sensible one that would not confuse the user, like
> > your example with
> >
> > DEBUG=-btrfs,all,-fat
> >
> > But I don't think we have to put too much effort in it, especially since
> > the code should also work in case "all" is not present at all.
>
> The other thing is that the original code allows "all" to be any item
> in the string. So not allowing that would break backward-compatibility
> (maybe not a big deal though). And I also agree that it can be
> confusing. Maybe Daniel has an opinion.

It would be nice to have backward compatibility but I do not think it is
a must here. If we have to drop backward compatibility to provide
better functionality I am OK with it. For me the implementation of this
feature has to be efficient, i.e. do not delay execution strongly,
the configuration has to be easy to understand and well documented.

Daniel