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

[Javier Martinez Canillas]

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!

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat