On Tue, Oct 19, 2021 at 03:45:32PM -0500, Glenn Washburn wrote: > On Tue, 19 Oct 2021 21:10:27 +0200 > Michael Schierl <schie...@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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
