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

[Javier Martinez Canillas]

Hi Samuel,

On 03/22/2018 01:20 AM, Samuel Mendoza-Jonas wrote:
> 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.
>

Awesome. My question was more general though and I wanted to know if you had
other things to point out, so I could address those in the next version. But
I'll assume that you are OK with the rest of the patch.
 
> 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.
>

That's correct, each BLS fragment only contains a single boot option.

> Regards,
> Sam
> 
> 

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