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

[Brett Grandbois]

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) 
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.

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.



>
> So I think it should be
>
> if title
> else if machine_id && version
> else if version
> else if dev->device->id && state->image
>
>> I know I originally suggested it, but looking at the implementation I
>> can see that having option->name be a mash up of machine_id and version
>> isn't the way to go.  It looks much cleaner to just have:
>>
>> if title
>>
>> else if machine_id
>>
> But the different BLS fragments will have the same machine_id, so this
> can't be used alone.
Ah sorry, you're right there.  I had misinterpreted that key.
>> else if version
>>
>> else
>>
>>
>> which then makes the default check easy has you can do it based on an
>> option->name comparison afterwards.
>>
>> Brett
>>
> Best regards,

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/petitboot/attachments/20180321/5e09672e/attachment-0001.html>