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

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

Best regards,
Javier

 discover/grub2/Makefile.am                   |   1 +
 discover/grub2/blscfg.c                      | 188 +++++++++++++++++++++++++++
 discover/grub2/builtins.c                    |   9 +-
 discover/parser.c                            |  16 +++
 discover/parser.h                            |   8 ++
 test/parser/Makefile.am                      |   3 +
 test/parser/test-grub2-blscfg-multiple-bls.c |  44 +++++++
 test/parser/test-grub2-blscfg-opts-config.c  |  29 +++++
 test/parser/test-grub2-blscfg-opts-grubenv.c |  34 +++++
 test/parser/utils.c                          |  59 +++++++++
 10 files changed, 390 insertions(+), 1 deletion(-)
 create mode 100644 discover/grub2/blscfg.c
 create mode 100644 test/parser/test-grub2-blscfg-multiple-bls.c
 create mode 100644 test/parser/test-grub2-blscfg-opts-config.c
 create mode 100644 test/parser/test-grub2-blscfg-opts-grubenv.c

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@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
test this out on Fedora 27 (looks like 28 Beta is later in March)?.

Also is BLS support in upstream GRUB? I had a quick look but it didn't
appear so.

Some quick comments below:

> 
> Best regards,
> Javier
> 
>  discover/grub2/Makefile.am                   |   1 +
>  discover/grub2/blscfg.c                      | 188 +++++++++++++++++++++++++++
>  discover/grub2/builtins.c                    |   9 +-
>  discover/parser.c                            |  16 +++
>  discover/parser.h                            |   8 ++
>  test/parser/Makefile.am                      |   3 +
>  test/parser/test-grub2-blscfg-multiple-bls.c |  44 +++++++
>  test/parser/test-grub2-blscfg-opts-config.c  |  29 +++++
>  test/parser/test-grub2-blscfg-opts-grubenv.c |  34 +++++
>  test/parser/utils.c                          |  59 +++++++++
>  10 files changed, 390 insertions(+), 1 deletion(-)
>  create mode 100644 discover/grub2/blscfg.c
>  create mode 100644 test/parser/test-grub2-blscfg-multiple-bls.c
>  create mode 100644 test/parser/test-grub2-blscfg-opts-config.c
>  create mode 100644 test/parser/test-grub2-blscfg-opts-grubenv.c
> 
> diff --git a/discover/grub2/Makefile.am b/discover/grub2/Makefile.am
> index 130ede88e18c..b240106d7a54 100644
> --- a/discover/grub2/Makefile.am
> +++ b/discover/grub2/Makefile.am
> @@ -15,6 +15,7 @@
>  noinst_PROGRAMS += discover/grub2/grub2-parser.ro
>  
>  discover_grub2_grub2_parser_ro_SOURCES = \
> +	discover/grub2/blscfg.c \
>  	discover/grub2/builtins.c \
>  	discover/grub2/env.c \
>  	discover/grub2/grub2.h \
> diff --git a/discover/grub2/blscfg.c b/discover/grub2/blscfg.c
> new file mode 100644
> index 000000000000..a74eac30161d
> --- /dev/null
> +++ b/discover/grub2/blscfg.c
> @@ -0,0 +1,188 @@
> +
> +#define _GNU_SOURCE
> +
> +#include <assert.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <dirent.h>
> +
> +#include <log/log.h>
> +#include <file/file.h>
> +#include <talloc/talloc.h>
> +#include <i18n/i18n.h>
> +
> +#include "grub2.h"
> +#include "discover/parser-conf.h"
> +#include "discover/parser.h"
> +
> +#define BLS_DIR "/loader/entries"
> +
> +struct bls_state {
> +	struct discover_boot_option *opt;
> +	struct grub2_script *script;
> +	const char *filename;
> +	const char *initrd;
> +	const char *image;
> +};
> +
> +static void bls_process_pair(struct conf_context *conf, const char *name,
> +			     char *value)
> +{
> +	struct discover_device *dev = conf->dc->device;
> +	struct bls_state *state = conf->parser_info;
> +	struct discover_boot_option *opt = state->opt;
> +	struct boot_option *option = opt->option;
> +	const char *boot_args;
> +
> +	if (streq(name, "title")) {
> +		option->id = talloc_asprintf(option, "%s#%s", dev->device->id,
> +					     value);
> +		option->name = talloc_strdup(option, value);
> +		return;
> +	}
> +
> +	if (streq(name, "linux")) {
> +		state->image = talloc_strdup(state, value);
> +		return;
> +	}
> +
> +	if (streq(name, "initrd")) {
> +		state->initrd = talloc_strdup(state, value);
> +		return;
> +	}
> +
> +	if (streq(name, "options")) {
> +		if (value[0] == '$') {
> +			boot_args = script_env_get(state->script, value + 1);
> +			if (!boot_args)
> +				return;
> +
> +			option->boot_args = talloc_strdup(opt, boot_args);
> +		} else {
> +			option->boot_args = talloc_strdup(opt, value);
> +		}
> +		return;
> +	}
> +}
> +
> +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?

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

> +
> +	if (offset < 0)
> +		return 0;
> +
> +	return strncmp(ent->d_name + offset, ".conf", strlen(".conf")) == 0;
> +}
> +
> +static int bls_sort(const struct dirent **ent_a, const struct dirent **ent_b)
> +{
> +	return strverscmp((*ent_b)->d_name, (*ent_a)->d_name);
> +}
> +
> +int builtin_blscfg(struct grub2_script *script,
> +		void *data __attribute__((unused)),
> +		int argc __attribute__((unused)),
> +		char *argv[] __attribute__((unused)));
> +
> +int builtin_blscfg(struct grub2_script *script,
> +		void *data __attribute__((unused)),
> +		int argc __attribute__((unused)),
> +		char *argv[] __attribute__((unused)))
> +{
> +	struct discover_context *dc = script->ctx;
> +	struct dirent **bls_entries;
> +	struct conf_context *conf;
> +	struct bls_state *state;
> +	char *buf, *filename;
> +	int n, len, rc = -1;
> +
> +	conf = talloc_zero(dc, struct conf_context);
> +	if (!conf)
> +		return rc;
> +
> +	conf->dc = dc;
> +	conf->get_pair = conf_get_pair_space;
> +	conf->process_pair = bls_process_pair;
> +	conf->finish = bls_finish;
> +
> +	n = parser_scandir(dc, BLS_DIR, &bls_entries, bls_filter, bls_sort);
> +	if (n <= 0)
> +		goto err;
> +
> +	while (n--) {
> +		filename = talloc_asprintf(dc, BLS_DIR"/%s",
> +					   bls_entries[n]->d_name);
> +		if (!filename)
> +			break;
> +
> +		state = talloc_zero(conf, struct bls_state);
> +		if (!state)
> +			break;
> +
> +		state->opt = discover_boot_option_create(dc, dc->device);
> +		if (!state->opt)
> +			break;
> +
> +		state->script = script;
> +		state->filename = filename;
> +		conf->parser_info = state;
> +
> +		rc = parser_request_file(dc, dc->device, filename, &buf, &len);
> +		if (rc)
> +			break;
> +
> +		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()

> +	}
> +
> +	if (n > 0) {
> +		device_handler_status_dev_info(dc->handler, dc->device,
> +					       _("Scanning %s failed"),
> +					       BLS_DIR);
> +		do {
> +			free(bls_entries[n]);
> +		} while (n-- > 0);
> +	}
> +
> +	free(bls_entries);
> +err:
> +	talloc_free(conf);
> +	return rc;
> +}
> diff --git a/discover/grub2/builtins.c b/discover/grub2/builtins.c
> index c16b6390225a..e42821a64a9a 100644
> --- a/discover/grub2/builtins.c
> +++ b/discover/grub2/builtins.c
> @@ -330,7 +330,10 @@ extern int builtin_load_env(struct grub2_script *script,
>  int builtin_save_env(struct grub2_script *script,
>  		void *data __attribute__((unused)),
>  		int argc, char *argv[]);
> -
> +int builtin_blscfg(struct grub2_script *script,
> +		void *data __attribute__((unused)),
> +		int argc __attribute__((unused)),
> +		char *argv[] __attribute__((unused)));
>  
>  static struct {
>  	const char *name;
> @@ -380,6 +383,10 @@ static struct {
>  		.name = "save_env",
>  		.fn = builtin_save_env,
>  	},
> +	{
> +		.name = "blscfg",
> +		.fn = builtin_blscfg,
> +	}
>  };
>  
>  static const char *nops[] = {
> diff --git a/discover/parser.c b/discover/parser.c
> index 5598f963e236..9fe1925d94c4 100644
> --- a/discover/parser.c
> +++ b/discover/parser.c
> @@ -128,6 +128,22 @@ out:
>  	return -1;
>  }
>  
> +int parser_scandir(struct discover_context *ctx, const char *dirname,
> +		   struct dirent ***files, int (*filter)(const struct dirent *),
> +		   int (*comp)(const struct dirent **, const struct dirent **))
> +{
> +	char *path;
> +	int n;
> +
> +	path = talloc_asprintf(ctx, "%s%s", ctx->device->mount_path, dirname);
> +	if (!path)
> +		return -1;
> +
> +	n = scandir(path, files, filter, comp);
> +	talloc_free(path);
> +	return n;
> +}
> +
>  void iterate_parsers(struct discover_context *ctx)
>  {
>  	struct p_item* i;
> diff --git a/discover/parser.h b/discover/parser.h
> index fc165c5aeda4..bff52e30d09f 100644
> --- a/discover/parser.h
> +++ b/discover/parser.h
> @@ -5,6 +5,7 @@
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <unistd.h>
> +#include <dirent.h>
>  
>  #include "device-handler.h"
>  
> @@ -76,5 +77,12 @@ int parser_request_url(struct discover_context *ctx, struct pb_url *url,
>  int parser_stat_path(struct discover_context *ctx,
>  		struct discover_device *dev, const char *path,
>  		struct stat *statbuf);
> +/* Function used to list the files on a directory. The dirname should
> + * be relative to the discover context device mount path. It returns
> + * the number of files returned in files or a negative value on error.
> + */
> +int parser_scandir(struct discover_context *ctx, const char *dirname,
> +		   struct dirent ***files, int (*filter)(const struct dirent *),
> +		   int (*comp)(const struct dirent **, const struct dirent **));
>  
>  #endif /* _PARSER_H */
> diff --git a/test/parser/Makefile.am b/test/parser/Makefile.am
> index a0795dbcf899..b943408c4942 100644
> --- a/test/parser/Makefile.am
> +++ b/test/parser/Makefile.am
> @@ -40,6 +40,9 @@ parser_TESTS = \
>  	test/parser/test-grub2-parser-error \
>  	test/parser/test-grub2-test-file-ops \
>  	test/parser/test-grub2-single-yocto \
> +	test/parser/test-grub2-blscfg-multiple-bls \
> +	test/parser/test-grub2-blscfg-opts-config \
> +	test/parser/test-grub2-blscfg-opts-grubenv \
>  	test/parser/test-kboot-single \
>  	test/parser/test-yaboot-empty \
>  	test/parser/test-yaboot-single \
> diff --git a/test/parser/test-grub2-blscfg-multiple-bls.c b/test/parser/test-grub2-blscfg-multiple-bls.c
> new file mode 100644
> index 000000000000..8fd218c371e8
> --- /dev/null
> +++ b/test/parser/test-grub2-blscfg-multiple-bls.c
> @@ -0,0 +1,44 @@
> +#include "parser-test.h"
> +
> +#if 0 /* PARSER_EMBEDDED_CONFIG */
> +blscfg
> +#endif
> +
> +void run_test(struct parser_test *test)
> +{
> +	struct discover_boot_option *opt;
> +	struct discover_context *ctx;
> +
> +	test_add_file_string(test, test->ctx->device,
> +			     "/loader/entries/6c063c8e48904f2684abde8eea303f41-4.15.2-302.fc28.x86_64.conf",
> +			     "title Fedora (4.15.2-302.fc28.x86_64) 28 (Twenty Eight)\n"
> +			     "linux /vmlinuz-4.15.2-302.fc28.x86_64\n"
> +			     "initrd /initramfs-4.15.2-302.fc28.x86_64.img\n"
> +			     "options root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/root\n\n");
> +
> +	test_add_file_string(test, test->ctx->device,
> +			     "/loader/entries/6c063c8e48904f2684abde8eea303f41-4.14.18-300.fc28.x86_64.conf",
> +			     "title Fedora (4.14.18-300.fc28.x86_64) 28 (Twenty Eight)\n"
> +			     "linux /vmlinuz-4.14.18-300.fc28.x86_64\n"
> +			     "initrd /initramfs-4.14.18-300.fc28.x86_64.img\n"
> +			     "options root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/root\n");
> +
> +	test_read_conf_embedded(test, "/boot/grub2/grub.cfg");
> +
> +	test_run_parser(test, "grub2");
> +
> +	ctx = test->ctx;
> +
> +	check_boot_option_count(ctx, 2);
> +	opt = get_boot_option(ctx, 0);
> +
> +	check_name(opt, "Fedora (4.15.2-302.fc28.x86_64) 28 (Twenty Eight)");
> +	check_resolved_local_resource(opt->boot_image, ctx->device,
> +			"/vmlinuz-4.15.2-302.fc28.x86_64");
> +
> +	opt = get_boot_option(ctx, 1);
> +
> +	check_name(opt, "Fedora (4.14.18-300.fc28.x86_64) 28 (Twenty Eight)");
> +	check_resolved_local_resource(opt->initrd, ctx->device,
> +			"/initramfs-4.14.18-300.fc28.x86_64.img");
> +}
> diff --git a/test/parser/test-grub2-blscfg-opts-config.c b/test/parser/test-grub2-blscfg-opts-config.c
> new file mode 100644
> index 000000000000..856aae2adf5f
> --- /dev/null
> +++ b/test/parser/test-grub2-blscfg-opts-config.c
> @@ -0,0 +1,29 @@
> +#include "parser-test.h"
> +
> +#if 0 /* PARSER_EMBEDDED_CONFIG */
> +set kernelopts=root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/root
> +blscfg
> +#endif
> +
> +void run_test(struct parser_test *test)
> +{
> +	struct discover_boot_option *opt;
> +	struct discover_context *ctx;
> +
> +	test_add_file_string(test, test->ctx->device,
> +			     "/loader/entries/6c063c8e48904f2684abde8eea303f41-4.15.2-302.fc28.x86_64.conf",
> +			     "title Fedora (4.15.2-302.fc28.x86_64) 28 (Twenty Eight)\n"
> +			     "linux /vmlinuz-4.15.2-302.fc28.x86_64\n"
> +			     "initrd /initramfs-4.15.2-302.fc28.x86_64.img\n"
> +			     "options $kernelopts\n");
> +
> +	test_read_conf_embedded(test, "/boot/grub2/grub.cfg");
> +
> +	test_run_parser(test, "grub2");
> +
> +	ctx = test->ctx;
> +
> +	opt = get_boot_option(ctx, 0);
> +
> +	check_args(opt, "root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/root");
> +}
> diff --git a/test/parser/test-grub2-blscfg-opts-grubenv.c b/test/parser/test-grub2-blscfg-opts-grubenv.c
> new file mode 100644
> index 000000000000..c77c589b7707
> --- /dev/null
> +++ b/test/parser/test-grub2-blscfg-opts-grubenv.c
> @@ -0,0 +1,34 @@
> +#include "parser-test.h"
> +
> +#if 0 /* PARSER_EMBEDDED_CONFIG */
> +load_env
> +blscfg
> +#endif
> +
> +void run_test(struct parser_test *test)
> +{
> +	struct discover_boot_option *opt;
> +	struct discover_context *ctx;
> +
> +	test_add_file_string(test, test->ctx->device,
> +			     "/boot/grub2/grubenv",
> +			     "# GRUB Environment Block\n"
> +			     "kernelopts=root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/root\n");
> +
> +	test_add_file_string(test, test->ctx->device,
> +			     "/loader/entries/6c063c8e48904f2684abde8eea303f41-4.15.2-302.fc28.x86_64.conf",
> +			     "title Fedora (4.15.2-302.fc28.x86_64) 28 (Twenty Eight)\n"
> +			     "linux /vmlinuz-4.15.2-302.fc28.x86_64\n"
> +			     "initrd /initramfs-4.15.2-302.fc28.x86_64.img\n"
> +			     "options $kernelopts\n");
> +
> +	test_read_conf_embedded(test, "/boot/grub2/grub.cfg");
> +
> +	test_run_parser(test, "grub2");
> +
> +	ctx = test->ctx;
> +
> +	opt = get_boot_option(ctx, 0);
> +
> +	check_args(opt, "root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/root");
> +}
> diff --git a/test/parser/utils.c b/test/parser/utils.c
> index 8900bd72bebd..683bba7d0379 100644
> --- a/test/parser/utils.c
> +++ b/test/parser/utils.c
> @@ -309,6 +309,65 @@ int parser_replace_file(struct discover_context *ctx,
>  	return 0;
>  }
>  
> +int parser_scandir(struct discover_context *ctx, const char *dirname,
> +		   struct dirent ***files, int (*filter)(const struct dirent *)
> +		   __attribute__((unused)),
> +		   int (*comp)(const struct dirent **, const struct dirent **)
> +		   __attribute__((unused)))
> +{
> +	struct parser_test *test = ctx->test_data;
> +	struct test_file *f;
> +	char *filename;
> +	struct dirent **dirents = NULL, **new_dirents;
> +	int n = 0, namelen;
> +
> +	list_for_each_entry(&test->files, f, list) {
> +		if (f->dev != ctx->device)
> +			continue;
> +
> +		filename = strrchr(f->name, '/');
> +		if (!filename)
> +			continue;
> +
> +		namelen = strlen(filename);
> +
> +		if (strncmp(f->name, dirname, strlen(f->name) - namelen))
> +			continue;
> +
> +		if (!dirents) {
> +			dirents = malloc(sizeof(struct dirent *));
> +		} else {
> +			new_dirents = realloc(dirents, sizeof(struct dirent *)
> +					      * (n + 1));
> +			if (!new_dirents)
> +				goto err_cleanup;
> +
> +			dirents = new_dirents;
> +		}
> +
> +		dirents[n] = malloc(sizeof(struct dirent) + namelen + 1);
> +
> +		if (!dirents[n])
> +			goto err_cleanup;
> +
> +		strcpy(dirents[n]->d_name, filename + 1);
> +		n++;
> +	}
> +
> +	*files = dirents;
> +
> +	return n;
> +
> +err_cleanup:
> +	do {
> +		free(dirents[n]);
> +	} while (n-- > 0);
> +
> +	free(dirents);
> +
> +	return -1;
> +}
> +
>  struct load_url_result *load_url_async(void *ctx, struct pb_url *url,
>  		load_url_complete async_cb, void *async_data,
>  		waiter_cb stdout_cb, void *stdout_data)

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@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,

+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@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@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/petitboot
<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">&lt;javierm@redhat.com&gt;</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-&gt;parser_info;
+	struct discover_context *dc = conf-&gt;dc;
+	struct discover_boot_option *opt = state-&gt;opt;
+	struct boot_option *option = opt-&gt;option;
+	const char *root;
+
+	if (!option-&gt;id || !option-&gt;name || !option-&gt;boot_args ||
+	    !state-&gt;image || !state-&gt;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 &amp; 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-&gt;handler, dc-&gt;device,
+					       _("BLS file %s is incorrect"),
+					       state-&gt;filename);
+		return;
+	}
+
+	root = script_env_get(state-&gt;script, "root");
+
+	opt-&gt;boot_image = create_grub2_resource(opt, conf-&gt;dc-&gt;device,
+						root, state-&gt;image);
+	opt-&gt;initrd = create_grub2_resource(opt, conf-&gt;dc-&gt;device,
+					    root, state-&gt;initrd);
+	discover_context_add_boot_option(dc, opt);
+
+	device_handler_status_dev_info(dc-&gt;handler, dc-&gt;device,
+				       _("Created menu entry from BLS file %s"),
+				       state-&gt;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-&gt;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>

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,

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,

On Wed, Mar 14, 2018 at 07:29:14AM +1000, Brett Grandbois wrote:
> 
> 
> 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.

FWIW, rawhide kernels include them in
/lib/modules/%{version}-{release}/bls.conf currently.