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