<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>+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.</p>
<p>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?<br>
</p>
<br>
<div class="moz-cite-prefix">On 08/03/18 19:51, Javier Martinez
Canillas wrote:<br>
</div>
<blockquote type="cite"
cite="mid:8486c666-e221-3e0c-263b-884b8bf41645@redhat.com">
<pre wrap="">On 03/08/2018 06:07 AM, Samuel Mendoza-Jonas wrote:
</pre>
<blockquote type="cite">
<pre wrap="">On Wed, 2018-03-07 at 20:43 +0100, Javier Martinez Canillas wrote:
</pre>
<blockquote type="cite">
<pre wrap="">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]: <a class="moz-txt-link-freetext" href="https://www.freedesktop.org/wiki/Specifications/BootLoaderSpec/">https://www.freedesktop.org/wiki/Specifications/BootLoaderSpec/</a>
Signed-off-by: Javier Martinez Canillas <a class="moz-txt-link-rfc2396E" href="mailto:javierm@redhat.com"><javierm@redhat.com></a>
---
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.
</pre>
</blockquote>
<pre wrap="">
Hi, thanks for thinking ahead! Is there a straightforward way for me to
</pre>
</blockquote>
<pre wrap="">
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.
</pre>
<blockquote type="cite">
<pre wrap="">test this out on Fedora 27 (looks like 28 Beta is later in March)?.
</pre>
</blockquote>
<pre wrap="">
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.
</pre>
<blockquote type="cite">
<pre wrap="">Also is BLS support in upstream GRUB? I had a quick look but it didn't
appear so.
</pre>
</blockquote>
<pre wrap="">
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.
</pre>
<blockquote type="cite">
<pre wrap="">Some quick comments below:
</pre>
</blockquote>
<pre wrap="">
[snip]
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">+
+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) {
</pre>
</blockquote>
<pre wrap="">
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?
</pre>
</blockquote>
<pre wrap="">
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.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">+ 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)))
</pre>
</blockquote>
<pre wrap="">
This says ((unused)) but
</pre>
<blockquote type="cite">
<pre wrap="">+{
+ int offset = strlen(ent->d_name) - strlen(".conf");
</pre>
</blockquote>
<pre wrap="">
We use it?
</pre>
</blockquote>
<pre wrap="">
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]
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">+
+ conf_parse_buf(conf, buf, len);
+
+ talloc_free(buf);
+ talloc_free(state);
+ talloc_free(filename);
+ free(bls_entries[n]);
+
+ if (rc)
+ break;
</pre>
</blockquote>
<pre wrap="">
This check is redundant, we will have break'd after parser_request_file()
</pre>
</blockquote>
<pre wrap="">
Indeed. I reorganized the code a little bit and forgot to remove it. Thanks a
lot for pointing it out.
Best regards,
</pre>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
Petitboot mailing list
<a class="moz-txt-link-abbreviated" href="mailto:Petitboot@lists.ozlabs.org">Petitboot@lists.ozlabs.org</a>
<a class="moz-txt-link-freetext" href="https://lists.ozlabs.org/listinfo/petitboot">https://lists.ozlabs.org/listinfo/petitboot</a>
</pre>
</blockquote>
<br>
</body>
</html>