| Message ID | 20180319092723.1047-1-javierm@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] discover/grub: Add blscfg command support to parse BootLoaderSpec files | expand |
Tested it out and it works. I just noticed however that there is no support for default in it so somewhere in bls_finish it would be nice to have an option->is_default check. The concept of index doesn't seem to apply in BLS like it does in a list of menuentries so probably the best way to go is to do the default env comparison on title or machine_id if either exist. I know I originally suggested it, but looking at the implementation I can see that having option->name be a mash up of machine_id and version isn't the way to go. It looks much cleaner to just have: if title else if machine_id else if version else which then makes the default check easy has you can do it based on an option->name comparison afterwards. Brett On 19/03/18 19:27, 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. > > This can be tested by creating a BLS config under /boot/loader/entries, > for example following /boot/loader/entries/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 a grub.cfg that calls the blscfg command, it could just be the following: > > blscfg > > Best regards, > Javier > > Changes in v3: > - Populate boot option id using the linux field instead of title field. > - Don't fill boot option name using the optional title field, instead > fill this from optional fields that are present in the BLS fragment. > - Add support for the devicetree field. > > Changes in v2: > - Allow optional fields, only require the linux field to be present. > - Remove unused identifier from bls_filter() struct dirent * param. > - Remove redundant check in builtin_blscfg() while loop. > > discover/grub2/Makefile.am | 1 + > discover/grub2/blscfg.c | 221 +++++++++++++++++++++++++++ > 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, 423 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..5677aa081531 > --- /dev/null > +++ b/discover/grub2/blscfg.c > @@ -0,0 +1,221 @@ > + > +#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 *title; > + const char *version; > + const char *machine_id; > + const char *image; > + const char *initrd; > + const char *dtb; > +}; > + > +static void bls_process_pair(struct conf_context *conf, const char *name, > + char *value) > +{ > + 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")) { > + state->title = talloc_strdup(state, value); > + return; > + } > + > + if (streq(name, "version")) { > + state->version = talloc_strdup(state, value); > + return; > + } > + > + if (streq(name, "machine-id")) { > + state->machine_id = talloc_strdup(state, 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, "devicetree")) { > + state->dtb = 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 discover_device *dev = conf->dc->device; > + 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 (!state->image) { > + device_handler_status_dev_info(dc->handler, dc->device, > + _("linux field not found in %s"), > + state->filename); > + return; > + } > + > + option->id = talloc_asprintf(option, "%s#%s", dev->device->id, > + state->image); > + > + if (state->title) > + option->name = talloc_strdup(option, state->title); > + else if (state->machine_id && state->version) > + option->name = talloc_asprintf(option, "%s %s", > + state->machine_id, > + state->version); > + else if (state->version) > + option->name = talloc_strdup(option, state->version); > + else > + option->name = talloc_strdup(option, option->id); > + > + root = script_env_get(state->script, "root"); > + > + opt->boot_image = create_grub2_resource(opt, conf->dc->device, > + root, state->image); > + > + if (state->initrd) > + opt->initrd = create_grub2_resource(opt, conf->dc->device, > + root, state->initrd); > + > + if (state->dtb) > + opt->dtb = create_grub2_resource(opt, conf->dc->device, > + root, state->dtb); > + 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) > +{ > + int offset = strlen(ent->d_name) - strlen(".conf"); > + > + 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 (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)
It also just occurred to me that the BLS filename itself minus the '.conf' extension should also be used as a comparison for the is_default check from this line in the spec: > The file name of the file is used for identification of the boot item, > but shall never be presented to the user in the UI. And possibly even used as the basis for option->id rather than state->image? On 21/03/18 08:45, Brett Grandbois wrote: > [This sender failed our fraud detection checks and may not be who they > appear to be. Learn about spoofing at http://aka.ms/LearnAboutSpoofing] > > Tested it out and it works. I just noticed however that there is no > support for default in it so somewhere in bls_finish it would be nice to > have an option->is_default check. The concept of index doesn't seem to > apply in BLS like it does in a list of menuentries so probably the best > way to go is to do the default env comparison on title or machine_id if > either exist. > > I know I originally suggested it, but looking at the implementation I > can see that having option->name be a mash up of machine_id and version > isn't the way to go. It looks much cleaner to just have: > > if title > > else if machine_id > > else if version > > else > > > which then makes the default check easy has you can do it based on an > option->name comparison afterwards. > > Brett > > > On 19/03/18 19:27, 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. >> >> This can be tested by creating a BLS config under /boot/loader/entries, >> for example following /boot/loader/entries/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 a grub.cfg that calls the blscfg command, it could just be the >> following: >> >> blscfg >> >> Best regards, >> Javier >> >> Changes in v3: >> - Populate boot option id using the linux field instead of title field. >> - Don't fill boot option name using the optional title field, instead >> fill this from optional fields that are present in the BLS fragment. >> - Add support for the devicetree field. >> >> Changes in v2: >> - Allow optional fields, only require the linux field to be present. >> - Remove unused identifier from bls_filter() struct dirent * param. >> - Remove redundant check in builtin_blscfg() while loop. >> >> discover/grub2/Makefile.am | 1 + >> discover/grub2/blscfg.c | 221 >> +++++++++++++++++++++++++++ >> 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, 423 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..5677aa081531 >> --- /dev/null >> +++ b/discover/grub2/blscfg.c >> @@ -0,0 +1,221 @@ >> + >> +#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 *title; >> + const char *version; >> + const char *machine_id; >> + const char *image; >> + const char *initrd; >> + const char *dtb; >> +}; >> + >> +static void bls_process_pair(struct conf_context *conf, const char >> *name, >> + char *value) >> +{ >> + 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")) { >> + state->title = talloc_strdup(state, value); >> + return; >> + } >> + >> + if (streq(name, "version")) { >> + state->version = talloc_strdup(state, value); >> + return; >> + } >> + >> + if (streq(name, "machine-id")) { >> + state->machine_id = talloc_strdup(state, 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, "devicetree")) { >> + state->dtb = 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 discover_device *dev = conf->dc->device; >> + 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 (!state->image) { >> + device_handler_status_dev_info(dc->handler, dc->device, >> + _("linux field not found >> in %s"), >> + state->filename); >> + return; >> + } >> + >> + option->id = talloc_asprintf(option, "%s#%s", dev->device->id, >> + state->image); >> + >> + if (state->title) >> + option->name = talloc_strdup(option, state->title); >> + else if (state->machine_id && state->version) >> + option->name = talloc_asprintf(option, "%s %s", >> + state->machine_id, >> + state->version); >> + else if (state->version) >> + option->name = talloc_strdup(option, state->version); >> + else >> + option->name = talloc_strdup(option, option->id); >> + >> + root = script_env_get(state->script, "root"); >> + >> + opt->boot_image = create_grub2_resource(opt, conf->dc->device, >> + root, state->image); >> + >> + if (state->initrd) >> + opt->initrd = create_grub2_resource(opt, conf->dc->device, >> + root, state->initrd); >> + >> + if (state->dtb) >> + opt->dtb = create_grub2_resource(opt, conf->dc->device, >> + root, state->dtb); >> + 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) >> +{ >> + int offset = strlen(ent->d_name) - strlen(".conf"); >> + >> + 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 (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) > > _______________________________________________ > 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>It also just occurred to me that the BLS filename itself minus the '.conf' extension should also be used as a comparison for the is_default check from this line in the spec:</p> <p> <blockquote type="cite"><span style="color: rgb(0, 0, 0); font-family: "Times New Roman"; font-size: 16.16px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); text-decoration-style: initial; text-decoration-color: initial; display: inline !important; float: none;">The file name of the file is used for identification of the boot item, but shall never be presented to the user in the UI.</span></blockquote> And possibly even used as the basis for option->id rather than state->image?<br> </p> <br> <div class="moz-cite-prefix">On 21/03/18 08:45, Brett Grandbois wrote:<br> </div> <blockquote type="cite" cite="mid:3e40d783-f9e7-e798-819f-859f0c4ab123@opengear.com">[This sender failed our fraud detection checks and may not be who they appear to be. Learn about spoofing at <a class="moz-txt-link-freetext" href="http://aka.ms/LearnAboutSpoofing">http://aka.ms/LearnAboutSpoofing</a>] <br> <br> Tested it out and it works. I just noticed however that there is no <br> support for default in it so somewhere in bls_finish it would be nice to <br> have an option->is_default check. The concept of index doesn't seem to <br> apply in BLS like it does in a list of menuentries so probably the best <br> way to go is to do the default env comparison on title or machine_id if <br> either exist. <br> <br> I know I originally suggested it, but looking at the implementation I <br> can see that having option->name be a mash up of machine_id and version <br> isn't the way to go. It looks much cleaner to just have: <br> <br> if title <br> <br> else if machine_id <br> <br> else if version <br> <br> else <br> <br> <br> which then makes the default check easy has you can do it based on an <br> option->name comparison afterwards. <br> <br> Brett <br> <br> <br> On 19/03/18 19:27, Javier Martinez Canillas wrote: <br> <blockquote type="cite">The BootLoaderSpec (BLS) defines a file format for boot configurations, <br> so bootloaders can parse these files and create their boot menu entries <br> by using the information provided by them [0]. <br> <br> This allow to configure the boot items as drop-in files in a directory <br> instead of having to parse and modify a bootloader configuration file. <br> <br> The GRUB 2 bootloader provides a blscfg command that parses these files <br> and creates menu entries using this information. Add support for it. <br> <br> [0]: <a class="moz-txt-link-freetext" href="https://www.freedesktop.org/wiki/Specifications/BootLoaderSpec/">https://www.freedesktop.org/wiki/Specifications/BootLoaderSpec/</a> <br> <br> Signed-off-by: Javier Martinez Canillas <a class="moz-txt-link-rfc2396E" href="mailto:javierm@redhat.com"><javierm@redhat.com></a> <br> <br> --- <br> <br> Hello, <br> <br> From Fedora 28 there will be an option to use BootLoaderSpec snippets to <br> update GRUB's boot menu entries. So I'm posting this patch to allow this <br> to also work on ppc64 machines using petitboot, instead of grub-ieee1275. <br> <br> This can be tested by creating a BLS config under /boot/loader/entries, <br> for example following /boot/loader/entries/4.15.6-300.fc27.x86_64.conf: <br> <br> title Fedora (4.15.6-300.fc27.x86_64) 27 (Twenty Seven) <br> linux /vmlinuz-4.15.6-300.fc27.x86_64 <br> initrd /initramfs-4.15.6-300.fc27.x86_64.img <br> 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 <br> id fedora-20180214051518-4.15.6-300.fc27.x86_64.x86_64 <br> grub_users $grub_users <br> grub_arg --unrestricted <br> grub_class kernel <br> <br> And a grub.cfg that calls the blscfg command, it could just be the following: <br> <br> blscfg <br> <br> Best regards, <br> Javier <br> <br> Changes in v3: <br> - Populate boot option id using the linux field instead of title field. <br> - Don't fill boot option name using the optional title field, instead <br> fill this from optional fields that are present in the BLS fragment. <br> - Add support for the devicetree field. <br> <br> Changes in v2: <br> - Allow optional fields, only require the linux field to be present. <br> - Remove unused identifier from bls_filter() struct dirent * param. <br> - Remove redundant check in builtin_blscfg() while loop. <br> <br> discover/grub2/Makefile.am | 1 + <br> discover/grub2/blscfg.c | 221 +++++++++++++++++++++++++++ <br> discover/grub2/builtins.c | 9 +- <br> discover/parser.c | 16 ++ <br> discover/parser.h | 8 + <br> test/parser/Makefile.am | 3 + <br> test/parser/test-grub2-blscfg-multiple-bls.c | 44 ++++++ <br> test/parser/test-grub2-blscfg-opts-config.c | 29 ++++ <br> test/parser/test-grub2-blscfg-opts-grubenv.c | 34 +++++ <br> test/parser/utils.c | 59 +++++++ <br> 10 files changed, 423 insertions(+), 1 deletion(-) <br> create mode 100644 discover/grub2/blscfg.c <br> create mode 100644 test/parser/test-grub2-blscfg-multiple-bls.c <br> create mode 100644 test/parser/test-grub2-blscfg-opts-config.c <br> create mode 100644 test/parser/test-grub2-blscfg-opts-grubenv.c <br> <br> diff --git a/discover/grub2/Makefile.am b/discover/grub2/Makefile.am <br> index 130ede88e18c..b240106d7a54 100644 <br> --- a/discover/grub2/Makefile.am <br> +++ b/discover/grub2/Makefile.am <br> @@ -15,6 +15,7 @@ <br> noinst_PROGRAMS += discover/grub2/grub2-parser.ro <br> <br> discover_grub2_grub2_parser_ro_SOURCES = \ <br> + discover/grub2/blscfg.c \ <br> discover/grub2/builtins.c \ <br> discover/grub2/env.c \ <br> discover/grub2/grub2.h \ <br> diff --git a/discover/grub2/blscfg.c b/discover/grub2/blscfg.c <br> new file mode 100644 <br> index 000000000000..5677aa081531 <br> --- /dev/null <br> +++ b/discover/grub2/blscfg.c <br> @@ -0,0 +1,221 @@ <br> + <br> +#define _GNU_SOURCE <br> + <br> +#include <assert.h> <br> +#include <stdlib.h> <br> +#include <string.h> <br> +#include <dirent.h> <br> + <br> +#include <log/log.h> <br> +#include <file/file.h> <br> +#include <talloc/talloc.h> <br> +#include <i18n/i18n.h> <br> + <br> +#include "grub2.h" <br> +#include "discover/parser-conf.h" <br> +#include "discover/parser.h" <br> + <br> +#define BLS_DIR "/loader/entries" <br> + <br> +struct bls_state { <br> + struct discover_boot_option *opt; <br> + struct grub2_script *script; <br> + const char *filename; <br> + const char *title; <br> + const char *version; <br> + const char *machine_id; <br> + const char *image; <br> + const char *initrd; <br> + const char *dtb; <br> +}; <br> + <br> +static void bls_process_pair(struct conf_context *conf, const char *name, <br> + char *value) <br> +{ <br> + struct bls_state *state = conf->parser_info; <br> + struct discover_boot_option *opt = state->opt; <br> + struct boot_option *option = opt->option; <br> + const char *boot_args; <br> + <br> + if (streq(name, "title")) { <br> + state->title = talloc_strdup(state, value); <br> + return; <br> + } <br> + <br> + if (streq(name, "version")) { <br> + state->version = talloc_strdup(state, value); <br> + return; <br> + } <br> + <br> + if (streq(name, "machine-id")) { <br> + state->machine_id = talloc_strdup(state, value); <br> + return; <br> + } <br> + <br> + if (streq(name, "linux")) { <br> + state->image = talloc_strdup(state, value); <br> + return; <br> + } <br> + <br> + if (streq(name, "initrd")) { <br> + state->initrd = talloc_strdup(state, value); <br> + return; <br> + } <br> + <br> + if (streq(name, "devicetree")) { <br> + state->dtb = talloc_strdup(state, value); <br> + return; <br> + } <br> + <br> + if (streq(name, "options")) { <br> + if (value[0] == '$') { <br> + boot_args = script_env_get(state->script, value + 1); <br> + if (!boot_args) <br> + return; <br> + <br> + option->boot_args = talloc_strdup(opt, boot_args); <br> + } else { <br> + option->boot_args = talloc_strdup(opt, value); <br> + } <br> + return; <br> + } <br> +} <br> + <br> +static void bls_finish(struct conf_context *conf) <br> +{ <br> + struct discover_device *dev = conf->dc->device; <br> + struct bls_state *state = conf->parser_info; <br> + struct discover_context *dc = conf->dc; <br> + struct discover_boot_option *opt = state->opt; <br> + struct boot_option *option = opt->option; <br> + const char *root; <br> + <br> + if (!state->image) { <br> + device_handler_status_dev_info(dc->handler, dc->device, <br> + _("linux field not found in %s"), <br> + state->filename); <br> + return; <br> + } <br> + <br> + option->id = talloc_asprintf(option, "%s#%s", dev->device->id, <br> + state->image); <br> + <br> + if (state->title) <br> + option->name = talloc_strdup(option, state->title); <br> + else if (state->machine_id && state->version) <br> + option->name = talloc_asprintf(option, "%s %s", <br> + state->machine_id, <br> + state->version); <br> + else if (state->version) <br> + option->name = talloc_strdup(option, state->version); <br> + else <br> + option->name = talloc_strdup(option, option->id); <br> + <br> + root = script_env_get(state->script, "root"); <br> + <br> + opt->boot_image = create_grub2_resource(opt, conf->dc->device, <br> + root, state->image); <br> + <br> + if (state->initrd) <br> + opt->initrd = create_grub2_resource(opt, conf->dc->device, <br> + root, state->initrd); <br> + <br> + if (state->dtb) <br> + opt->dtb = create_grub2_resource(opt, conf->dc->device, <br> + root, state->dtb); <br> + discover_context_add_boot_option(dc, opt); <br> + <br> + device_handler_status_dev_info(dc->handler, dc->device, <br> + _("Created menu entry from BLS file %s"), <br> + state->filename); <br> +} <br> + <br> +static int bls_filter(const struct dirent *ent) <br> +{ <br> + int offset = strlen(ent->d_name) - strlen(".conf"); <br> + <br> + if (offset < 0) <br> + return 0; <br> + <br> + return strncmp(ent->d_name + offset, ".conf", strlen(".conf")) == 0; <br> +} <br> + <br> +static int bls_sort(const struct dirent **ent_a, const struct dirent **ent_b) <br> +{ <br> + return strverscmp((*ent_b)->d_name, (*ent_a)->d_name); <br> +} <br> + <br> +int builtin_blscfg(struct grub2_script *script, <br> + void *data __attribute__((unused)), <br> + int argc __attribute__((unused)), <br> + char *argv[] __attribute__((unused))); <br> + <br> +int builtin_blscfg(struct grub2_script *script, <br> + void *data __attribute__((unused)), <br> + int argc __attribute__((unused)), <br> + char *argv[] __attribute__((unused))) <br> +{ <br> + struct discover_context *dc = script->ctx; <br> + struct dirent **bls_entries; <br> + struct conf_context *conf; <br> + struct bls_state *state; <br> + char *buf, *filename; <br> + int n, len, rc = -1; <br> + <br> + conf = talloc_zero(dc, struct conf_context); <br> + if (!conf) <br> + return rc; <br> + <br> + conf->dc = dc; <br> + conf->get_pair = conf_get_pair_space; <br> + conf->process_pair = bls_process_pair; <br> + conf->finish = bls_finish; <br> + <br> + n = parser_scandir(dc, BLS_DIR, &bls_entries, bls_filter, bls_sort); <br> + if (n <= 0) <br> + goto err; <br> + <br> + while (n--) { <br> + filename = talloc_asprintf(dc, BLS_DIR"/%s", <br> + bls_entries[n]->d_name); <br> + if (!filename) <br> + break; <br> + <br> + state = talloc_zero(conf, struct bls_state); <br> + if (!state) <br> + break; <br> + <br> + state->opt = discover_boot_option_create(dc, dc->device); <br> + if (!state->opt) <br> + break; <br> + <br> + state->script = script; <br> + state->filename = filename; <br> + conf->parser_info = state; <br> + <br> + rc = parser_request_file(dc, dc->device, filename, &buf, &len); <br> + if (rc) <br> + break; <br> + <br> + conf_parse_buf(conf, buf, len); <br> + <br> + talloc_free(buf); <br> + talloc_free(state); <br> + talloc_free(filename); <br> + free(bls_entries[n]); <br> + } <br> + <br> + if (n > 0) { <br> + device_handler_status_dev_info(dc->handler, dc->device, <br> + _("Scanning %s failed"), <br> + BLS_DIR); <br> + do { <br> + free(bls_entries[n]); <br> + } while (n-- > 0); <br> + } <br> + <br> + free(bls_entries); <br> +err: <br> + talloc_free(conf); <br> + return rc; <br> +} <br> diff --git a/discover/grub2/builtins.c b/discover/grub2/builtins.c <br> index c16b6390225a..e42821a64a9a 100644 <br> --- a/discover/grub2/builtins.c <br> +++ b/discover/grub2/builtins.c <br> @@ -330,7 +330,10 @@ extern int builtin_load_env(struct grub2_script *script, <br> int builtin_save_env(struct grub2_script *script, <br> void *data __attribute__((unused)), <br> int argc, char *argv[]); <br> - <br> +int builtin_blscfg(struct grub2_script *script, <br> + void *data __attribute__((unused)), <br> + int argc __attribute__((unused)), <br> + char *argv[] __attribute__((unused))); <br> <br> static struct { <br> const char *name; <br> @@ -380,6 +383,10 @@ static struct { <br> .name = "save_env", <br> .fn = builtin_save_env, <br> }, <br> + { <br> + .name = "blscfg", <br> + .fn = builtin_blscfg, <br> + } <br> }; <br> <br> static const char *nops[] = { <br> diff --git a/discover/parser.c b/discover/parser.c <br> index 5598f963e236..9fe1925d94c4 100644 <br> --- a/discover/parser.c <br> +++ b/discover/parser.c <br> @@ -128,6 +128,22 @@ out: <br> return -1; <br> } <br> <br> +int parser_scandir(struct discover_context *ctx, const char *dirname, <br> + struct dirent ***files, int (*filter)(const struct dirent *), <br> + int (*comp)(const struct dirent **, const struct dirent **)) <br> +{ <br> + char *path; <br> + int n; <br> + <br> + path = talloc_asprintf(ctx, "%s%s", ctx->device->mount_path, dirname); <br> + if (!path) <br> + return -1; <br> + <br> + n = scandir(path, files, filter, comp); <br> + talloc_free(path); <br> + return n; <br> +} <br> + <br> void iterate_parsers(struct discover_context *ctx) <br> { <br> struct p_item* i; <br> diff --git a/discover/parser.h b/discover/parser.h <br> index fc165c5aeda4..bff52e30d09f 100644 <br> --- a/discover/parser.h <br> +++ b/discover/parser.h <br> @@ -5,6 +5,7 @@ <br> #include <sys/types.h> <br> #include <sys/stat.h> <br> #include <unistd.h> <br> +#include <dirent.h> <br> <br> #include "device-handler.h" <br> <br> @@ -76,5 +77,12 @@ int parser_request_url(struct discover_context *ctx, struct pb_url *url, <br> int parser_stat_path(struct discover_context *ctx, <br> struct discover_device *dev, const char *path, <br> struct stat *statbuf); <br> +/* Function used to list the files on a directory. The dirname should <br> + * be relative to the discover context device mount path. It returns <br> + * the number of files returned in files or a negative value on error. <br> + */ <br> +int parser_scandir(struct discover_context *ctx, const char *dirname, <br> + struct dirent ***files, int (*filter)(const struct dirent *), <br> + int (*comp)(const struct dirent **, const struct dirent **)); <br> <br> #endif /* _PARSER_H */ <br> diff --git a/test/parser/Makefile.am b/test/parser/Makefile.am <br> index a0795dbcf899..b943408c4942 100644 <br> --- a/test/parser/Makefile.am <br> +++ b/test/parser/Makefile.am <br> @@ -40,6 +40,9 @@ parser_TESTS = \ <br> test/parser/test-grub2-parser-error \ <br> test/parser/test-grub2-test-file-ops \ <br> test/parser/test-grub2-single-yocto \ <br> + test/parser/test-grub2-blscfg-multiple-bls \ <br> + test/parser/test-grub2-blscfg-opts-config \ <br> + test/parser/test-grub2-blscfg-opts-grubenv \ <br> test/parser/test-kboot-single \ <br> test/parser/test-yaboot-empty \ <br> test/parser/test-yaboot-single \ <br> diff --git a/test/parser/test-grub2-blscfg-multiple-bls.c b/test/parser/test-grub2-blscfg-multiple-bls.c <br> new file mode 100644 <br> index 000000000000..8fd218c371e8 <br> --- /dev/null <br> +++ b/test/parser/test-grub2-blscfg-multiple-bls.c <br> @@ -0,0 +1,44 @@ <br> +#include "parser-test.h" <br> + <br> +#if 0 /* PARSER_EMBEDDED_CONFIG */ <br> +blscfg <br> +#endif <br> + <br> +void run_test(struct parser_test *test) <br> +{ <br> + struct discover_boot_option *opt; <br> + struct discover_context *ctx; <br> + <br> + test_add_file_string(test, test->ctx->device, <br> + "/loader/entries/6c063c8e48904f2684abde8eea303f41-4.15.2-302.fc28.x86_64.conf",<br> + "title Fedora (4.15.2-302.fc28.x86_64) 28 (Twenty Eight)\n" <br> + "linux /vmlinuz-4.15.2-302.fc28.x86_64\n" <br> + "initrd /initramfs-4.15.2-302.fc28.x86_64.img\n" <br> + "options root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/root\n\n"); <br> + <br> + test_add_file_string(test, test->ctx->device, <br> + "/loader/entries/6c063c8e48904f2684abde8eea303f41-4.14.18-300.fc28.x86_64.conf",<br> + "title Fedora (4.14.18-300.fc28.x86_64) 28 (Twenty Eight)\n" <br> + "linux /vmlinuz-4.14.18-300.fc28.x86_64\n" <br> + "initrd /initramfs-4.14.18-300.fc28.x86_64.img\n" <br> + "options root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/root\n"); <br> + <br> + test_read_conf_embedded(test, "/boot/grub2/grub.cfg"); <br> + <br> + test_run_parser(test, "grub2"); <br> + <br> + ctx = test->ctx; <br> + <br> + check_boot_option_count(ctx, 2); <br> + opt = get_boot_option(ctx, 0); <br> + <br> + check_name(opt, "Fedora (4.15.2-302.fc28.x86_64) 28 (Twenty Eight)"); <br> + check_resolved_local_resource(opt->boot_image, ctx->device, <br> + "/vmlinuz-4.15.2-302.fc28.x86_64"); <br> + <br> + opt = get_boot_option(ctx, 1); <br> + <br> + check_name(opt, "Fedora (4.14.18-300.fc28.x86_64) 28 (Twenty Eight)"); <br> + check_resolved_local_resource(opt->initrd, ctx->device, <br> + "/initramfs-4.14.18-300.fc28.x86_64.img"); <br> +} <br> diff --git a/test/parser/test-grub2-blscfg-opts-config.c b/test/parser/test-grub2-blscfg-opts-config.c <br> new file mode 100644 <br> index 000000000000..856aae2adf5f <br> --- /dev/null <br> +++ b/test/parser/test-grub2-blscfg-opts-config.c <br> @@ -0,0 +1,29 @@ <br> +#include "parser-test.h" <br> + <br> +#if 0 /* PARSER_EMBEDDED_CONFIG */ <br> +set kernelopts=root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/root <br> +blscfg <br> +#endif <br> + <br> +void run_test(struct parser_test *test) <br> +{ <br> + struct discover_boot_option *opt; <br> + struct discover_context *ctx; <br> + <br> + test_add_file_string(test, test->ctx->device, <br> + "/loader/entries/6c063c8e48904f2684abde8eea303f41-4.15.2-302.fc28.x86_64.conf",<br> + "title Fedora (4.15.2-302.fc28.x86_64) 28 (Twenty Eight)\n" <br> + "linux /vmlinuz-4.15.2-302.fc28.x86_64\n" <br> + "initrd /initramfs-4.15.2-302.fc28.x86_64.img\n" <br> + "options $kernelopts\n"); <br> + <br> + test_read_conf_embedded(test, "/boot/grub2/grub.cfg"); <br> + <br> + test_run_parser(test, "grub2"); <br> + <br> + ctx = test->ctx; <br> + <br> + opt = get_boot_option(ctx, 0); <br> + <br> + check_args(opt, "root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/root"); <br> +} <br> diff --git a/test/parser/test-grub2-blscfg-opts-grubenv.c b/test/parser/test-grub2-blscfg-opts-grubenv.c <br> new file mode 100644 <br> index 000000000000..c77c589b7707 <br> --- /dev/null <br> +++ b/test/parser/test-grub2-blscfg-opts-grubenv.c <br> @@ -0,0 +1,34 @@ <br> +#include "parser-test.h" <br> + <br> +#if 0 /* PARSER_EMBEDDED_CONFIG */ <br> +load_env <br> +blscfg <br> +#endif <br> + <br> +void run_test(struct parser_test *test) <br> +{ <br> + struct discover_boot_option *opt; <br> + struct discover_context *ctx; <br> + <br> + test_add_file_string(test, test->ctx->device, <br> + "/boot/grub2/grubenv", <br> + "# GRUB Environment Block\n" <br> + "kernelopts=root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/root\n"); <br> + <br> + test_add_file_string(test, test->ctx->device, <br> + "/loader/entries/6c063c8e48904f2684abde8eea303f41-4.15.2-302.fc28.x86_64.conf",<br> + "title Fedora (4.15.2-302.fc28.x86_64) 28 (Twenty Eight)\n" <br> + "linux /vmlinuz-4.15.2-302.fc28.x86_64\n" <br> + "initrd /initramfs-4.15.2-302.fc28.x86_64.img\n" <br> + "options $kernelopts\n"); <br> + <br> + test_read_conf_embedded(test, "/boot/grub2/grub.cfg"); <br> + <br> + test_run_parser(test, "grub2"); <br> + <br> + ctx = test->ctx; <br> + <br> + opt = get_boot_option(ctx, 0); <br> + <br> + check_args(opt, "root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/root"); <br> +} <br> diff --git a/test/parser/utils.c b/test/parser/utils.c <br> index 8900bd72bebd..683bba7d0379 100644 <br> --- a/test/parser/utils.c <br> +++ b/test/parser/utils.c <br> @@ -309,6 +309,65 @@ int parser_replace_file(struct discover_context *ctx, <br> return 0; <br> } <br> <br> +int parser_scandir(struct discover_context *ctx, const char *dirname, <br> + struct dirent ***files, int (*filter)(const struct dirent *) <br> + __attribute__((unused)), <br> + int (*comp)(const struct dirent **, const struct dirent **) <br> + __attribute__((unused))) <br> +{ <br> + struct parser_test *test = ctx->test_data; <br> + struct test_file *f; <br> + char *filename; <br> + struct dirent **dirents = NULL, **new_dirents; <br> + int n = 0, namelen; <br> + <br> + list_for_each_entry(&test->files, f, list) { <br> + if (f->dev != ctx->device) <br> + continue; <br> + <br> + filename = strrchr(f->name, '/'); <br> + if (!filename) <br> + continue; <br> + <br> + namelen = strlen(filename); <br> + <br> + if (strncmp(f->name, dirname, strlen(f->name) - namelen)) <br> + continue; <br> + <br> + if (!dirents) { <br> + dirents = malloc(sizeof(struct dirent *)); <br> + } else { <br> + new_dirents = realloc(dirents, sizeof(struct dirent *) <br> + * (n + 1)); <br> + if (!new_dirents) <br> + goto err_cleanup; <br> + <br> + dirents = new_dirents; <br> + } <br> + <br> + dirents[n] = malloc(sizeof(struct dirent) + namelen + 1); <br> + <br> + if (!dirents[n]) <br> + goto err_cleanup; <br> + <br> + strcpy(dirents[n]->d_name, filename + 1); <br> + n++; <br> + } <br> + <br> + *files = dirents; <br> + <br> + return n; <br> + <br> +err_cleanup: <br> + do { <br> + free(dirents[n]); <br> + } while (n-- > 0); <br> + <br> + free(dirents); <br> + <br> + return -1; <br> +} <br> + <br> struct load_url_result *load_url_async(void *ctx, struct pb_url *url, <br> load_url_complete async_cb, void *async_data, <br> waiter_cb stdout_cb, void *stdout_data) <br> </blockquote> <br> _______________________________________________ <br> Petitboot mailing list <br> <a class="moz-txt-link-abbreviated" href="mailto:Petitboot@lists.ozlabs.org">Petitboot@lists.ozlabs.org</a> <br> <a class="moz-txt-link-freetext" href="https://lists.ozlabs.org/listinfo/petitboot">https://lists.ozlabs.org/listinfo/petitboot</a> <br> </blockquote> <br> </body> </html>
On 03/20/2018 11:45 PM, Brett Grandbois wrote: > Tested it out and it works. I just noticed however that there is no > support for default in it so somewhere in bls_finish it would be nice to Right, I wrongly assumed that this would already work by just setting a default env var in grub.cfg, but now see that was missing support for it. > have an option->is_default check. The concept of index doesn't seem to > apply in BLS like it does in a list of menuentries so probably the best > way to go is to do the default env comparison on title or machine_id if > either exist. > There's already an option_is_default() in discover/grub2/script.c so I think we should use it for consistency. The logic there checks if there is a default env var and if it's a number, then uses it as an index. If isn't a number, then it compares first with opt->id or as a fallback to opt->name. So since users expects to set default to the menu entry name as shown in the UI (as is the case for grub2 without BLS) then we should either set opt->id to opt->name or not set opt->id at all. So I think it should be if title else if machine_id && version else if version else if dev->device->id && state->image > I know I originally suggested it, but looking at the implementation I > can see that having option->name be a mash up of machine_id and version > isn't the way to go. It looks much cleaner to just have: > > if title > > else if machine_id > But the different BLS fragments will have the same machine_id, so this can't be used alone. > else if version > > else > > > which then makes the default check easy has you can do it based on an > option->name comparison afterwards. > > Brett > Best regards,
On 21/03/18 09:59, Javier Martinez Canillas wrote: > On 03/20/2018 11:45 PM, Brett Grandbois wrote: >> Tested it out and it works. I just noticed however that there is no >> support for default in it so somewhere in bls_finish it would be nice to > Right, I wrongly assumed that this would already work by just setting a > default env var in grub.cfg, but now see that was missing support for it. > >> have an option->is_default check. The concept of index doesn't seem to >> apply in BLS like it does in a list of menuentries so probably the best >> way to go is to do the default env comparison on title or machine_id if >> either exist. >> > There's already an option_is_default() in discover/grub2/script.c so I > think we should use it for consistency. The logic there checks if there > is a default env var and if it's a number, then uses it as an index. If > isn't a number, then it compares first with opt->id or as a fallback to > opt->name. > > So since users expects to set default to the menu entry name as shown > in the UI (as is the case for grub2 without BLS) then we should either > set opt->id to opt->name or not set opt->id at all. But option_is_default is called from within the menuentry parser and expects that syntax environment where BLS is a different syntax so I don't think consistency is required here if it turns out option_is_default is unsuitable. Do all grub users expect to set the default to the UI menu display entry name? For systems with more automated startup scripting (like ours) using a unique --id (or equivalent) field makes automatic boot determination in the grub.cfg simpler. In the current menuentry parser the option->id is first populated to the --id field if present and only falls back to name if id isn't present. For BLS as I mentioned in my follow-up email, I actually now think the id should be the filename minus the .conf extension as the spec states: > The file name of the file is used for identification of the boot item Which sounds to me like the perfect candidate for the option->id field. Then any combination of title/machine-id/version can be displayed to the UI without any fear of an id name collision. > > So I think it should be > > if title > else if machine_id && version > else if version > else if dev->device->id && state->image > >> I know I originally suggested it, but looking at the implementation I >> can see that having option->name be a mash up of machine_id and version >> isn't the way to go. It looks much cleaner to just have: >> >> if title >> >> else if machine_id >> > But the different BLS fragments will have the same machine_id, so this > can't be used alone. Ah sorry, you're right there. I had misinterpreted that key. >> else if version >> >> else >> >> >> which then makes the default check easy has you can do it based on an >> option->name comparison afterwards. >> >> Brett >> > Best regards, <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <p><br> </p> <br> <div class="moz-cite-prefix">On 21/03/18 09:59, Javier Martinez Canillas wrote:<br> </div> <blockquote type="cite" cite="mid:b55c1cc9-23fc-5a33-4661-b859ae53834b@redhat.com"> <pre wrap="">On 03/20/2018 11:45 PM, Brett Grandbois wrote: </pre> <blockquote type="cite"> <pre wrap="">Tested it out and it works. I just noticed however that there is no support for default in it so somewhere in bls_finish it would be nice to </pre> </blockquote> <pre wrap=""> Right, I wrongly assumed that this would already work by just setting a default env var in grub.cfg, but now see that was missing support for it. </pre> <blockquote type="cite"> <pre wrap="">have an option->is_default check. The concept of index doesn't seem to apply in BLS like it does in a list of menuentries so probably the best way to go is to do the default env comparison on title or machine_id if either exist. </pre> </blockquote> <pre wrap=""> There's already an option_is_default() in discover/grub2/script.c so I think we should use it for consistency. The logic there checks if there is a default env var and if it's a number, then uses it as an index. If isn't a number, then it compares first with opt->id or as a fallback to opt->name. So since users expects to set default to the menu entry name as shown in the UI (as is the case for grub2 without BLS) then we should either set opt->id to opt->name or not set opt->id at all.</pre> </blockquote> But option_is_default is called from within the menuentry parser and expects that syntax environment where BLS is a different syntax so I don't think consistency is required here if it turns out option_is_default is unsuitable.<br> <br> Do all grub users expect to set the default to the UI menu display entry name? For systems with more automated startup scripting (like ours) using a unique --id (or equivalent) field makes automatic boot determination in the grub.cfg simpler. In the current menuentry parser the option->id is first populated to the --id field if present and only falls back to name if id isn't present. <br> <br> For BLS as I mentioned in my follow-up email, I actually now think the id should be the filename minus the .conf extension as the spec states:<br> <br> <blockquote type="cite"><span style="color: rgb(0, 0, 0); font-family: "Times New Roman"; font-size: 16.16px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); text-decoration-style: initial; text-decoration-color: initial; display: inline !important; float: none;">The file name of the file is used for identification of the boot item</span></blockquote> Which sounds to me like the perfect candidate for the option->id field. Then any combination of title/machine-id/version can be displayed to the UI without any fear of an id name collision.<br> <br> <br> <br> <blockquote type="cite" cite="mid:b55c1cc9-23fc-5a33-4661-b859ae53834b@redhat.com"> <pre wrap=""> So I think it should be if title else if machine_id && version else if version else if dev->device->id && state->image </pre> <blockquote type="cite"> <pre wrap="">I know I originally suggested it, but looking at the implementation I can see that having option->name be a mash up of machine_id and version isn't the way to go. It looks much cleaner to just have: if title else if machine_id </pre> </blockquote> <pre wrap=""> But the different BLS fragments will have the same machine_id, so this can't be used alone. </pre> </blockquote> Ah sorry, you're right there. I had misinterpreted that key.<br> <blockquote type="cite" cite="mid:b55c1cc9-23fc-5a33-4661-b859ae53834b@redhat.com"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">else if version else which then makes the default check easy has you can do it based on an option->name comparison afterwards. Brett </pre> </blockquote> <pre wrap=""> Best regards, </pre> </blockquote> <br> </body> </html>
On 03/21/2018 01:50 AM, Brett Grandbois wrote: > > > On 21/03/18 09:59, Javier Martinez Canillas wrote: >> On 03/20/2018 11:45 PM, Brett Grandbois wrote: >>> Tested it out and it works. I just noticed however that there is no >>> support for default in it so somewhere in bls_finish it would be nice to >> Right, I wrongly assumed that this would already work by just setting a >> default env var in grub.cfg, but now see that was missing support for it. >> >>> have an option->is_default check. The concept of index doesn't seem to >>> apply in BLS like it does in a list of menuentries so probably the best >>> way to go is to do the default env comparison on title or machine_id if >>> either exist. >>> >> There's already an option_is_default() in discover/grub2/script.c so I >> think we should use it for consistency. The logic there checks if there >> is a default env var and if it's a number, then uses it as an index. If >> isn't a number, then it compares first with opt->id or as a fallback to >> opt->name. >> >> So since users expects to set default to the menu entry name as shown >> in the UI (as is the case for grub2 without BLS) then we should either >> set opt->id to opt->name or not set opt->id at all. > But option_is_default is called from within the menuentry parser and > expects that syntax environment where BLS is a different syntax so I > don't think consistency is required here if it turns out > option_is_default is unsuitable. > > Do all grub users expect to set the default to the UI menu display entry > name? For systems with more automated startup scripting (like ours) Well, from my point of view BLS should be transparent to the user so if they already are used to have something like: saved_entry=Fedora (4.15.9-300.fc27.x86_64) 27 (Workstation Edition) in their grubenv and then set default="${saved_entry}" in the grub.cfg, it should still work after switching to use BLS to populate the menu entries. > using a unique --id (or equivalent) field makes automatic boot > determination in the grub.cfg simpler. In the current menuentry parser > the option->id is first populated to the --id field if present and only > falls back to name if id isn't present. > Right, there's no equivalent of --id in BLS though. We do have an id field in our generated bls.conf but I didn't add it since is a slightly deviation from the spec: https://src.fedoraproject.org/rpms/kernel/blob/master/f/generate_bls_conf.sh But if you think is useful, I can add it as another optional field and set opt->name to it as another fallback. > For BLS as I mentioned in my follow-up email, I actually now think the > id should be the filename minus the .conf extension as the spec states: > >> The file name of the file is used for identification of the boot item > Which sounds to me like the perfect candidate for the option->id field. > Then any combination of title/machine-id/version can be displayed to the > UI without any fear of an id name collision. > Yes, sorry for missing that follow-up email. I don't mind to use the file name to set option->id, but as mentioned I really think we should use the option->name to check the default option to avoid changing grub2 behavior. Best regards,
On 21/03/18 11:15, Javier Martinez Canillas wrote: > On 03/21/2018 01:50 AM, Brett Grandbois wrote: >> >> On 21/03/18 09:59, Javier Martinez Canillas wrote: >>> On 03/20/2018 11:45 PM, Brett Grandbois wrote: >>>> Tested it out and it works. I just noticed however that there is no >>>> support for default in it so somewhere in bls_finish it would be nice to >>> Right, I wrongly assumed that this would already work by just setting a >>> default env var in grub.cfg, but now see that was missing support for it. >>> >>>> have an option->is_default check. The concept of index doesn't seem to >>>> apply in BLS like it does in a list of menuentries so probably the best >>>> way to go is to do the default env comparison on title or machine_id if >>>> either exist. >>>> >>> There's already an option_is_default() in discover/grub2/script.c so I >>> think we should use it for consistency. The logic there checks if there >>> is a default env var and if it's a number, then uses it as an index. If >>> isn't a number, then it compares first with opt->id or as a fallback to >>> opt->name. >>> >>> So since users expects to set default to the menu entry name as shown >>> in the UI (as is the case for grub2 without BLS) then we should either >>> set opt->id to opt->name or not set opt->id at all. >> But option_is_default is called from within the menuentry parser and >> expects that syntax environment where BLS is a different syntax so I >> don't think consistency is required here if it turns out >> option_is_default is unsuitable. >> >> Do all grub users expect to set the default to the UI menu display entry >> name? For systems with more automated startup scripting (like ours) > Well, from my point of view BLS should be transparent to the user so if > they already are used to have something like: > > saved_entry=Fedora (4.15.9-300.fc27.x86_64) 27 (Workstation Edition) > > in their grubenv and then set default="${saved_entry}" in the grub.cfg, it > should still work after switching to use BLS to populate the menu entries. Well OK, but wouldn't that still work just by explicitly checking id and name? In bls_finish something like: const char *var = script_env_get(script, "default"); opt->is_default = (var && (!strcmp(option->id, var) || !strcmp(option->name, var))); With the assumption that id is the filename and name has been determined by the walk-through of title/version/etc. > >> using a unique --id (or equivalent) field makes automatic boot >> determination in the grub.cfg simpler. In the current menuentry parser >> the option->id is first populated to the --id field if present and only >> falls back to name if id isn't present. >> > Right, there's no equivalent of --id in BLS though. We do have an id field > in our generated bls.conf but I didn't add it since is a slightly deviation > from the spec: > > https://src.fedoraproject.org/rpms/kernel/blob/master/f/generate_bls_conf.sh > > But if you think is useful, I can add it as another optional field and set > opt->name to it as another fallback. > >> For BLS as I mentioned in my follow-up email, I actually now think the >> id should be the filename minus the .conf extension as the spec states: >> >>> The file name of the file is used for identification of the boot item >> Which sounds to me like the perfect candidate for the option->id field. >> Then any combination of title/machine-id/version can be displayed to the >> UI without any fear of an id name collision. >> > Yes, sorry for missing that follow-up email. I don't mind to use the file > name to set option->id, but as mentioned I really think we should use the > option->name to check the default option to avoid changing grub2 behavior. > > Best regards,
On 03/21/2018 02:45 AM, Brett Grandbois wrote: > > > On 21/03/18 11:15, Javier Martinez Canillas wrote: >> On 03/21/2018 01:50 AM, Brett Grandbois wrote: >>> >>> On 21/03/18 09:59, Javier Martinez Canillas wrote: >>>> On 03/20/2018 11:45 PM, Brett Grandbois wrote: >>>>> Tested it out and it works. I just noticed however that there is no >>>>> support for default in it so somewhere in bls_finish it would be nice to >>>> Right, I wrongly assumed that this would already work by just setting a >>>> default env var in grub.cfg, but now see that was missing support for it. >>>> >>>>> have an option->is_default check. The concept of index doesn't seem to >>>>> apply in BLS like it does in a list of menuentries so probably the best >>>>> way to go is to do the default env comparison on title or machine_id if >>>>> either exist. >>>>> >>>> There's already an option_is_default() in discover/grub2/script.c so I >>>> think we should use it for consistency. The logic there checks if there >>>> is a default env var and if it's a number, then uses it as an index. If >>>> isn't a number, then it compares first with opt->id or as a fallback to >>>> opt->name. >>>> >>>> So since users expects to set default to the menu entry name as shown >>>> in the UI (as is the case for grub2 without BLS) then we should either >>>> set opt->id to opt->name or not set opt->id at all. >>> But option_is_default is called from within the menuentry parser and >>> expects that syntax environment where BLS is a different syntax so I >>> don't think consistency is required here if it turns out >>> option_is_default is unsuitable. >>> >>> Do all grub users expect to set the default to the UI menu display entry >>> name? For systems with more automated startup scripting (like ours) >> Well, from my point of view BLS should be transparent to the user so if >> they already are used to have something like: >> >> saved_entry=Fedora (4.15.9-300.fc27.x86_64) 27 (Workstation Edition) >> >> in their grubenv and then set default="${saved_entry}" in the grub.cfg, it >> should still work after switching to use BLS to populate the menu entries. > Well OK, but wouldn't that still work just by explicitly checking id and > name? In bls_finish something like: > > const char *var = script_env_get(script, "default"); > opt->is_default = (var && (!strcmp(option->id, var) || > !strcmp(option->name, var))); > > With the assumption that id is the filename and name has been determined > by the walk-through of title/version/etc. > Yes, I wanted to use the current option_is_default() but you are right that it makes no sense to attempt to be consistent on how the default is selected since for BLS the index concept doesn't apply anyway. I'll wait some time to see if Samuel has any other feedback and then post a new version doing what you suggest. Thanks again! Best regards,
On Wed, 2018-03-21 at 02:52 +0100, Javier Martinez Canillas wrote: > On 03/21/2018 02:45 AM, Brett Grandbois wrote: > > > > > > On 21/03/18 11:15, Javier Martinez Canillas wrote: > > > On 03/21/2018 01:50 AM, Brett Grandbois wrote: > > > > > > > > On 21/03/18 09:59, Javier Martinez Canillas wrote: > > > > > On 03/20/2018 11:45 PM, Brett Grandbois wrote: > > > > > > Tested it out and it works. I just noticed however that there is no > > > > > > support for default in it so somewhere in bls_finish it would be nice to > > > > > > > > > > Right, I wrongly assumed that this would already work by just setting a > > > > > default env var in grub.cfg, but now see that was missing support for it. > > > > > > > > > > > have an option->is_default check. The concept of index doesn't seem to > > > > > > apply in BLS like it does in a list of menuentries so probably the best > > > > > > way to go is to do the default env comparison on title or machine_id if > > > > > > either exist. > > > > > > > > > > > > > > > > There's already an option_is_default() in discover/grub2/script.c so I > > > > > think we should use it for consistency. The logic there checks if there > > > > > is a default env var and if it's a number, then uses it as an index. If > > > > > isn't a number, then it compares first with opt->id or as a fallback to > > > > > opt->name. > > > > > > > > > > So since users expects to set default to the menu entry name as shown > > > > > in the UI (as is the case for grub2 without BLS) then we should either > > > > > set opt->id to opt->name or not set opt->id at all. > > > > > > > > But option_is_default is called from within the menuentry parser and > > > > expects that syntax environment where BLS is a different syntax so I > > > > don't think consistency is required here if it turns out > > > > option_is_default is unsuitable. > > > > > > > > Do all grub users expect to set the default to the UI menu display entry > > > > name? For systems with more automated startup scripting (like ours) > > > > > > Well, from my point of view BLS should be transparent to the user so if > > > they already are used to have something like: > > > > > > saved_entry=Fedora (4.15.9-300.fc27.x86_64) 27 (Workstation Edition) > > > > > > in their grubenv and then set default="${saved_entry}" in the grub.cfg, it > > > should still work after switching to use BLS to populate the menu entries. > > > > Well OK, but wouldn't that still work just by explicitly checking id and > > name? In bls_finish something like: > > > > const char *var = script_env_get(script, "default"); > > opt->is_default = (var && (!strcmp(option->id, var) || > > !strcmp(option->name, var))); > > > > With the assumption that id is the filename and name has been determined > > by the walk-through of title/version/etc. > > > > Yes, I wanted to use the current option_is_default() but you are right that > it makes no sense to attempt to be consistent on how the default is selected > since for BLS the index concept doesn't apply anyway. > > I'll wait some time to see if Samuel has any other feedback and then post a > new version doing what you suggest. Thanks again! I leaned towards option_is_default() initially as well but it's true that we're technically parsing a completely different format here so Brett's suggestion is good. One thing to confirm - is each BLS fragment only meant to contain one boot option? In that case using filename as option->id is fine, but otherwise we'd obviously have conflicts. The spec seems to say only one option per file though: > $BOOT/loader/entries/ is the directory containing the drop-in snippets. > This directory contains one .conf file for each boot menu item. Regards, Sam
Hi Samuel, On 03/22/2018 01:20 AM, Samuel Mendoza-Jonas wrote: > On Wed, 2018-03-21 at 02:52 +0100, Javier Martinez Canillas wrote: >> On 03/21/2018 02:45 AM, Brett Grandbois wrote: >>> >>> >>> On 21/03/18 11:15, Javier Martinez Canillas wrote: >>>> On 03/21/2018 01:50 AM, Brett Grandbois wrote: >>>>> >>>>> On 21/03/18 09:59, Javier Martinez Canillas wrote: >>>>>> On 03/20/2018 11:45 PM, Brett Grandbois wrote: >>>>>>> Tested it out and it works. I just noticed however that there is no >>>>>>> support for default in it so somewhere in bls_finish it would be nice to >>>>>> >>>>>> Right, I wrongly assumed that this would already work by just setting a >>>>>> default env var in grub.cfg, but now see that was missing support for it. >>>>>> >>>>>>> have an option->is_default check. The concept of index doesn't seem to >>>>>>> apply in BLS like it does in a list of menuentries so probably the best >>>>>>> way to go is to do the default env comparison on title or machine_id if >>>>>>> either exist. >>>>>>> >>>>>> >>>>>> There's already an option_is_default() in discover/grub2/script.c so I >>>>>> think we should use it for consistency. The logic there checks if there >>>>>> is a default env var and if it's a number, then uses it as an index. If >>>>>> isn't a number, then it compares first with opt->id or as a fallback to >>>>>> opt->name. >>>>>> >>>>>> So since users expects to set default to the menu entry name as shown >>>>>> in the UI (as is the case for grub2 without BLS) then we should either >>>>>> set opt->id to opt->name or not set opt->id at all. >>>>> >>>>> But option_is_default is called from within the menuentry parser and >>>>> expects that syntax environment where BLS is a different syntax so I >>>>> don't think consistency is required here if it turns out >>>>> option_is_default is unsuitable. >>>>> >>>>> Do all grub users expect to set the default to the UI menu display entry >>>>> name? For systems with more automated startup scripting (like ours) >>>> >>>> Well, from my point of view BLS should be transparent to the user so if >>>> they already are used to have something like: >>>> >>>> saved_entry=Fedora (4.15.9-300.fc27.x86_64) 27 (Workstation Edition) >>>> >>>> in their grubenv and then set default="${saved_entry}" in the grub.cfg, it >>>> should still work after switching to use BLS to populate the menu entries. >>> >>> Well OK, but wouldn't that still work just by explicitly checking id and >>> name? In bls_finish something like: >>> >>> const char *var = script_env_get(script, "default"); >>> opt->is_default = (var && (!strcmp(option->id, var) || >>> !strcmp(option->name, var))); >>> >>> With the assumption that id is the filename and name has been determined >>> by the walk-through of title/version/etc. >>> >> >> Yes, I wanted to use the current option_is_default() but you are right that >> it makes no sense to attempt to be consistent on how the default is selected >> since for BLS the index concept doesn't apply anyway. >> >> I'll wait some time to see if Samuel has any other feedback and then post a >> new version doing what you suggest. Thanks again! > > I leaned towards option_is_default() initially as well but it's true that > we're technically parsing a completely different format here so Brett's > suggestion is good. > Awesome. My question was more general though and I wanted to know if you had other things to point out, so I could address those in the next version. But I'll assume that you are OK with the rest of the patch. > One thing to confirm - is each BLS fragment only meant to contain one > boot option? In that case using filename as option->id is fine, but > otherwise we'd obviously have conflicts. The spec seems to say only one > option per file though: > >> $BOOT/loader/entries/ is the directory containing the drop-in snippets. >> This directory contains one .conf file for each boot menu item. > That's correct, each BLS fragment only contains a single boot option. > Regards, > Sam > > Best regards,
On Thu, 2018-03-22 at 09:32 +0100, Javier Martinez Canillas wrote: > Hi Samuel, > > On 03/22/2018 01:20 AM, Samuel Mendoza-Jonas wrote: > > On Wed, 2018-03-21 at 02:52 +0100, Javier Martinez Canillas wrote: > > > On 03/21/2018 02:45 AM, Brett Grandbois wrote: > > > > > > > > > > > > On 21/03/18 11:15, Javier Martinez Canillas wrote: > > > > > On 03/21/2018 01:50 AM, Brett Grandbois wrote: > > > > > > > > > > > > On 21/03/18 09:59, Javier Martinez Canillas wrote: > > > > > > > On 03/20/2018 11:45 PM, Brett Grandbois wrote: > > > > > > > > Tested it out and it works. I just noticed however that there is no > > > > > > > > support for default in it so somewhere in bls_finish it would be nice to > > > > > > > > > > > > > > Right, I wrongly assumed that this would already work by just setting a > > > > > > > default env var in grub.cfg, but now see that was missing support for it. > > > > > > > > > > > > > > > have an option->is_default check. The concept of index doesn't seem to > > > > > > > > apply in BLS like it does in a list of menuentries so probably the best > > > > > > > > way to go is to do the default env comparison on title or machine_id if > > > > > > > > either exist. > > > > > > > > > > > > > > > > > > > > > > There's already an option_is_default() in discover/grub2/script.c so I > > > > > > > think we should use it for consistency. The logic there checks if there > > > > > > > is a default env var and if it's a number, then uses it as an index. If > > > > > > > isn't a number, then it compares first with opt->id or as a fallback to > > > > > > > opt->name. > > > > > > > > > > > > > > So since users expects to set default to the menu entry name as shown > > > > > > > in the UI (as is the case for grub2 without BLS) then we should either > > > > > > > set opt->id to opt->name or not set opt->id at all. > > > > > > > > > > > > But option_is_default is called from within the menuentry parser and > > > > > > expects that syntax environment where BLS is a different syntax so I > > > > > > don't think consistency is required here if it turns out > > > > > > option_is_default is unsuitable. > > > > > > > > > > > > Do all grub users expect to set the default to the UI menu display entry > > > > > > name? For systems with more automated startup scripting (like ours) > > > > > > > > > > Well, from my point of view BLS should be transparent to the user so if > > > > > they already are used to have something like: > > > > > > > > > > saved_entry=Fedora (4.15.9-300.fc27.x86_64) 27 (Workstation Edition) > > > > > > > > > > in their grubenv and then set default="${saved_entry}" in the grub.cfg, it > > > > > should still work after switching to use BLS to populate the menu entries. > > > > > > > > Well OK, but wouldn't that still work just by explicitly checking id and > > > > name? In bls_finish something like: > > > > > > > > const char *var = script_env_get(script, "default"); > > > > opt->is_default = (var && (!strcmp(option->id, var) || > > > > !strcmp(option->name, var))); > > > > > > > > With the assumption that id is the filename and name has been determined > > > > by the walk-through of title/version/etc. > > > > > > > > > > Yes, I wanted to use the current option_is_default() but you are right that > > > it makes no sense to attempt to be consistent on how the default is selected > > > since for BLS the index concept doesn't apply anyway. > > > > > > I'll wait some time to see if Samuel has any other feedback and then post a > > > new version doing what you suggest. Thanks again! > > > > I leaned towards option_is_default() initially as well but it's true that > > we're technically parsing a completely different format here so Brett's > > suggestion is good. > > > > Awesome. My question was more general though and I wanted to know if you had > other things to point out, so I could address those in the next version. But > I'll assume that you are OK with the rest of the patch. Haha yes I should have mentioned - no other issues to report. > > > One thing to confirm - is each BLS fragment only meant to contain one > > boot option? In that case using filename as option->id is fine, but > > otherwise we'd obviously have conflicts. The spec seems to say only one > > option per file though: > > > > > $BOOT/loader/entries/ is the directory containing the drop-in snippets. > > > This directory contains one .conf file for each boot menu item. > > That's correct, each BLS fragment only contains a single boot option. Perfect. > > > Regards, > > Sam > > > > > > Best regards,
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..5677aa081531 --- /dev/null +++ b/discover/grub2/blscfg.c @@ -0,0 +1,221 @@ + +#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 *title; + const char *version; + const char *machine_id; + const char *image; + const char *initrd; + const char *dtb; +}; + +static void bls_process_pair(struct conf_context *conf, const char *name, + char *value) +{ + 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")) { + state->title = talloc_strdup(state, value); + return; + } + + if (streq(name, "version")) { + state->version = talloc_strdup(state, value); + return; + } + + if (streq(name, "machine-id")) { + state->machine_id = talloc_strdup(state, 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, "devicetree")) { + state->dtb = 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 discover_device *dev = conf->dc->device; + 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 (!state->image) { + device_handler_status_dev_info(dc->handler, dc->device, + _("linux field not found in %s"), + state->filename); + return; + } + + option->id = talloc_asprintf(option, "%s#%s", dev->device->id, + state->image); + + if (state->title) + option->name = talloc_strdup(option, state->title); + else if (state->machine_id && state->version) + option->name = talloc_asprintf(option, "%s %s", + state->machine_id, + state->version); + else if (state->version) + option->name = talloc_strdup(option, state->version); + else + option->name = talloc_strdup(option, option->id); + + root = script_env_get(state->script, "root"); + + opt->boot_image = create_grub2_resource(opt, conf->dc->device, + root, state->image); + + if (state->initrd) + opt->initrd = create_grub2_resource(opt, conf->dc->device, + root, state->initrd); + + if (state->dtb) + opt->dtb = create_grub2_resource(opt, conf->dc->device, + root, state->dtb); + 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) +{ + int offset = strlen(ent->d_name) - strlen(".conf"); + + 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 (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)
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. This can be tested by creating a BLS config under /boot/loader/entries, for example following /boot/loader/entries/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 a grub.cfg that calls the blscfg command, it could just be the following: blscfg Best regards, Javier Changes in v3: - Populate boot option id using the linux field instead of title field. - Don't fill boot option name using the optional title field, instead fill this from optional fields that are present in the BLS fragment. - Add support for the devicetree field. Changes in v2: - Allow optional fields, only require the linux field to be present. - Remove unused identifier from bls_filter() struct dirent * param. - Remove redundant check in builtin_blscfg() while loop. discover/grub2/Makefile.am | 1 + discover/grub2/blscfg.c | 221 +++++++++++++++++++++++++++ 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, 423 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