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

[Javier Martinez Canillas]

Hello Brett,

On 03/15/2018 12:11 AM, Brett Grandbois wrote:
> Hi Javier,
> 
>  From a functional perspective it looks fine to me.  We use compiled-in 
> initramfs and cmdline args so removing the strict checks there had it 
> work fine once I got our build system spitting out BLS config files.
>

Great, thanks a lot for testing and your feedback!
 
> The only suggestions I have are again related to specification 
> adherence.  As far as I can tell from 
> https://www.freedesktop.org/wiki/Specifications/BootLoaderSpec/ the only 
> mandatory key is 'linux', everything else is optional. You've accounted 
> for that in v2 by only requiring the image to be populated which is 
> filled by the linux key, but you're still relying on the 'title' key to 
> be present to fill out the id even though title is stated to be 
> optional.  So my suggestions would be:
> 
>   * in bls_process_pair() move the population of option->id to the
>     'linux' key path, that way you're guaranteed to have an id assuming
>     the conf file is conformant
>   * add new fields in bls_state for title, version, and machine-id and
>     strdup them if they are found in the conf file
>   * in bls_finish() have a bit of logic to fill out the option->name
>     based on what optional fields are available. For example if
>     state->title is populated then have that directly fill option->name
>     but if state-title is empty because the key wasn't there then work
>     your way down to some name based on some mashup of the available
>     version and/or machine id.  The fallback case could be id I guess.
>   * We don't use devicetrees so it doesn't affect us, but if you're
>     going this far you might as well add in support for the 'devicetree'
>     key for everyone who does need it.
> 

Agreed with all your suggestions. I'll take them into account and post a v3.

> Otherwise it looks good to me and thanks for submitting the implementation.
> 
> Brett
> 
> 
Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat