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

Wed Mar 14 08:29:14 AEDT 2018


On 14/03/18 06:25, Javier Martinez Canillas wrote:
> 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.
Yep, I do intend to apply and try it out.  It's just going to take me a 
little time to pivot our existing system to spit out BLS conf files first.
>> 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,