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

[Brett Grandbois]

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.

>
>> using a unique --id (or equivalent) field makes automatic boot
>> determination in the grub.cfg simpler.  In the current menuentry parser
>> the option->id is first populated to the --id field if present and only
>> falls back to name if id isn't present.
>>
> Right, there's no equivalent of --id in BLS though. We do have an id field
> in our generated bls.conf but I didn't add it since is a slightly deviation
> from the spec:
>
> https://src.fedoraproject.org/rpms/kernel/blob/master/f/generate_bls_conf.sh
>
> But if you think is useful, I can add it as another optional field and set
> opt->name to it as another fallback.
>
>> For BLS as I mentioned in my follow-up email, I actually now think the
>> id should be the filename minus the .conf extension as the spec states:
>>
>>> The file name of the file is used for identification of the boot item
>> Which sounds to me like the perfect candidate for the option->id field.
>> Then any combination of title/machine-id/version can be displayed to the
>> UI without any fear of an id name collision.
>>
> Yes, sorry for missing that follow-up email. I don't mind to use the file
> name to set option->id, but as mentioned I really think we should use the
> option->name to check the default option to avoid changing grub2 behavior.
>
> Best regards,