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

Wed Mar 14 07:25:32 AEDT 2018

Hello Brett,

> +1 to accept this sooner rather than wait for upstream GRUB. This is 
> almost exactly what we're after and I had thought I was going to have to 
> write my own equivalent parser.
>

I'm glad that you could find it useful. Any testing or review would be
highly appreciated.

> About the strict checks in bls_finish, the spec states that various keys 
> like options, initrd, etc, are optional so that would imply to me they 
> shouldn't be strictly checked?  Yes that allows users to modify the 
> options but they can do that anyway once the configurations are written 
> to disk?  If users want to be strictly enforce entries I would assume 
> they'd use the signed-boot system?

Fair enough, I'll relax this to only make the linux field to be required.

On 08/03/18 19:51, Javier Martinez Canillas wrote:
> On 03/08/2018 06:07 AM, Samuel Mendoza-Jonas wrote:
>> On Wed, 2018-03-07 at 20:43 +0100, Javier Martinez Canillas wrote:
>>> The BootLoaderSpec (BLS) defines a file format for boot configurations,
>>> so bootloaders can parse these files and create their boot menu entries
>>> by using the information provided by them [0].
>>>
>>> This allow to configure the boot items as drop-in files in a directory
>>> instead of having to parse and modify a bootloader configuration file.
>>>
>>> The GRUB 2 bootloader provides a blscfg command that parses these files
>>> and creates menu entries using this information. Add support for it.
>>>
>>> [0]: https://www.freedesktop.org/wiki/Specifications/BootLoaderSpec/
>>>
>>> Signed-off-by: Javier Martinez Canillas <javierm at redhat.com>
>>>
>>> ---
>>>
>>> Hello,
>>>
>>>  From Fedora 28 there will be an option to use BootLoaderSpec snippets to
>>> update GRUB's boot menu entries. So I'm posting this patch to allow this
>>> to also work on ppc64 machines using petitboot, instead of grub-ieee1275.
>> Hi, thanks for thinking ahead! Is there a straightforward way for me to
> Thanks for the quick review! I don't know if I made clear above, but this won't
> be the default for Fedora 28. We will provide it as an optional feature and let
> users enable it if they want to test, by setting a GRUB_ENABLE_BLSCFG="true" in
> the /etc/default/grub config file.
>
> We plan it to be the default though on later releases once we have the support
> for all the architectures in place. For now only platforms with GRUB will work
> out-of-the-box.
>
>> test this out on Fedora 27 (looks like 28 Beta is later in March)?.
>>
> I think the easiest way for you to test is to create a /boot/loader/entries dir
> and copy there a (or a set of) BLS config. For example, something like following
> /boot/loader/entries/6c063c8e48904f2684abde8eea303f41-4.15.6-300.fc27.x86_64.conf:
>
> title Fedora (4.15.6-300.fc27.x86_64) 27 (Twenty Seven)
> linux /vmlinuz-4.15.6-300.fc27.x86_64
> initrd /initramfs-4.15.6-300.fc27.x86_64.img
> options root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/root rd.luks.uuid=luks-0b078909-4a1c-4a57-91b8-b9f724e86a1a rd.lvm.lv=fedora/swap rhgb quiet LANG=en_US.UTF-8
> id fedora-20180214051518-4.15.6-300.fc27.x86_64.x86_64
> grub_users $grub_users
> grub_arg --unrestricted
> grub_class kernel
>
> And then you need a grub.cfg. It could be one that contains just the following:
>
> blscfg
>
> I've also attached the grub.cfg that's generated in F28 when GRUB_ENABLE_BLSCFG
> is enabled, in case you want to test a more complete grub.cfg (just remember to
> update the root UUID to match your block device or petitboot won't be able to
> resolve the grub resource).
>
> With those two you should be able to test this. The BLS snippets are included in
> the kernel package and copied to /boot/loader/entries on kernel installation but
> that's all platform independent and not interesting for you to test.
>
> I've also included tests, and made sure that they pass with make check. So you
> may also want to take a look at those for other use cases. For example, there is
> also support to have the kernel command line as a GRUB variable and have defined
> in grubenv. That's how we will ship, so users could change it in a single place.
>
>> Also is BLS support in upstream GRUB? I had a quick look but it didn't
>> appear so.
>>
> Not yet, but we plan to upstream it. The problem is that GRUB upstream isn't
> really great at reviewing and taking patches in a timely manner, so a change
> like this may take months to land. Will accepting it be blocked by upstream
> first taking it? I'm asking to know how much I should prioritize engaging
> with upstream GRUB if that's the case.
>
>> Some quick comments below:
>>
> [snip]
>
>>> +
>>> +static void bls_finish(struct conf_context *conf)
>>> +{
>>> +	struct bls_state *state = conf->parser_info;
>>> +	struct discover_context *dc = conf->dc;
>>> +	struct discover_boot_option *opt = state->opt;
>>> +	struct boot_option *option = opt->option;
>>> +	const char *root;
>>> +
>>> +	if (!option->id || !option->name || !option->boot_args ||
>>> +	    !state->image || !state->initrd) {
>> Do we want to be this strict, or allow boot options to have for example
>> just a kernel image, or image & initrd but not boot_args?
>>
> That's a very good question. I believe we want to be very strict on this and not
> allow users to boot an entry that's incorrect. It's mostly defensive programming
> really since as mentioned the BLS are generated at build time and shipped in the
> kernel package, so they should be correct.
>
> But users can still modify it and that's why I preferred to ignore the ones that
> don't have the expected fields. I've added a dev_info below so users can know if
> a BLS snippet was ignored due being wrong, by looking at the System status log.
>
>>> +		device_handler_status_dev_info(dc->handler, dc->device,
>>> +					       _("BLS file %s is incorrect"),
>>> +					       state->filename);
>>> +		return;
>>> +	}
>>> +
>>> +	root = script_env_get(state->script, "root");
>>> +
>>> +	opt->boot_image = create_grub2_resource(opt, conf->dc->device,
>>> +						root, state->image);
>>> +	opt->initrd = create_grub2_resource(opt, conf->dc->device,
>>> +					    root, state->initrd);
>>> +	discover_context_add_boot_option(dc, opt);
>>> +
>>> +	device_handler_status_dev_info(dc->handler, dc->device,
>>> +				       _("Created menu entry from BLS file %s"),
>>> +				       state->filename);
>>> +}
>>> +
>>> +static int bls_filter(const struct dirent *ent __attribute__((unused)))
>> This says ((unused)) but
>>
>>> +{
>>> +	int offset = strlen(ent->d_name) - strlen(".conf");
>> We use it?
>>
> Right, sorry about that. During development I just wrote an empty function and
> then I forgot to remove the unused attribute when I added the filter logic.
>
> [snip]
>
>>> +
>>> +		conf_parse_buf(conf, buf, len);
>>> +
>>> +		talloc_free(buf);
>>> +		talloc_free(state);
>>> +		talloc_free(filename);
>>> +		free(bls_entries[n]);
>>> +
>>> +		if (rc)
>>> +			break;
>> This check is redundant, we will have break'd after parser_request_file()
>>
> Indeed. I reorganized the code a little bit and forgot to remove it. Thanks a
> lot for pointing it out.
>
> Best regards,