[PATCH v3] discover/grub: Add blscfg command support to parse BootLoaderSpec files

[Samuel Mendoza-Jonas]

On Wed, 2018-03-21 at 02:52 +0100, Javier Martinez Canillas wrote:
> On 03/21/2018 02:45 AM, Brett Grandbois wrote:
> > 
> > 
> > On 21/03/18 11:15, Javier Martinez Canillas wrote:
> > > On 03/21/2018 01:50 AM, Brett Grandbois wrote:
> > > > 
> > > > On 21/03/18 09:59, Javier Martinez Canillas wrote:
> > > > > On 03/20/2018 11:45 PM, Brett Grandbois wrote:
> > > > > > Tested it out and it works.  I just noticed however that there is no
> > > > > > support for default in it so somewhere in bls_finish it would be nice to
> > > > > 
> > > > > Right, I wrongly assumed that this would already work by just setting a
> > > > > default env var in grub.cfg, but now see that was missing support for it.
> > > > > 
> > > > > > have an option->is_default check.  The concept of index doesn't seem to
> > > > > > apply in BLS like it does in a list of menuentries so probably the best
> > > > > > way to go is to do the default env comparison on title or machine_id if
> > > > > > either exist.
> > > > > > 
> > > > > 
> > > > > There's already an option_is_default() in discover/grub2/script.c so I
> > > > > think we should use it for consistency. The logic there checks if there
> > > > > is a default env var and if it's a number, then uses it as an index. If
> > > > > isn't a number, then it compares first with opt->id or as a fallback to
> > > > > opt->name.
> > > > > 
> > > > > So since users expects to set default to the menu entry name as shown
> > > > > in the UI (as is the case for grub2 without BLS) then we should either
> > > > > set opt->id to opt->name or not set opt->id at all.
> > > > 
> > > > But option_is_default is called from within the menuentry parser and
> > > > expects that syntax environment where BLS is a different syntax so I
> > > > don't think consistency is required here if it turns out
> > > > option_is_default is unsuitable.
> > > > 
> > > > Do all grub users expect to set the default to the UI menu display entry
> > > > name?  For systems with more automated startup scripting (like ours)
> > > 
> > > Well, from my point of view BLS should be transparent to the user so if
> > > they already are used to have something like:
> > > 
> > > saved_entry=Fedora (4.15.9-300.fc27.x86_64) 27 (Workstation Edition)
> > > 
> > > in their grubenv and then set default="${saved_entry}" in the grub.cfg, it
> > > should still work after switching to use BLS to populate the menu entries.
> > 
> > Well OK, but wouldn't that still work just by explicitly checking id and 
> > name?  In bls_finish something like:
> > 
> > const char *var = script_env_get(script, "default");
> > opt->is_default = (var && (!strcmp(option->id, var) || 
> > !strcmp(option->name, var)));
> > 
> > With the assumption that id is the filename and name has been determined 
> > by the walk-through of title/version/etc.
> > 
> 
> Yes, I wanted to use the current option_is_default() but you are right that
> it makes no sense to attempt to be consistent on how the default is selected
> since for BLS the index concept doesn't apply anyway.
> 
> I'll wait some time to see if Samuel has any other feedback and then post a
> new version doing what you suggest. Thanks again!

I leaned towards option_is_default() initially as well but it's true that
we're technically parsing a completely different format here so Brett's
suggestion is good.

One thing to confirm - is each BLS fragment only meant to contain one
boot option? In that case using filename as option->id is fine, but
otherwise we'd obviously have conflicts. The spec seems to say only one
option per file though:

> $BOOT/loader/entries/ is the directory containing the drop-in snippets.
> This directory contains one .conf file for each boot menu item.

Regards,
Sam