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