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

Mon Mar 12 10:23:26 AEDT 2018

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

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?

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,
>
>
> _______________________________________________
> Petitboot mailing list
> Petitboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/petitboot

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/petitboot/attachments/20180312/b807e278/attachment.html>