From f65a33269ea0f4eb298d52d741326b238491aaa3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 15 Feb 2022 10:25:37 +0100 Subject: [PATCH 1/7] Revert "boot: Change boot entry sorting" This reverts commit 9818ec8ea56e14902ac8e548a0f366dbb259f051. --- man/systemd-boot.xml | 8 ++++---- src/boot/efi/boot.c | 48 ++++++++++++++++++-------------------------- 2 files changed, 23 insertions(+), 33 deletions(-) diff --git a/man/systemd-boot.xml b/man/systemd-boot.xml index d503f09548..ceea368ef2 100644 --- a/man/systemd-boot.xml +++ b/man/systemd-boot.xml @@ -501,10 +501,10 @@ considered 'good' from then on. The boot menu takes the 'tries left' counter into account when sorting the menu entries: entries in 'bad' - state are ordered towards the end of the list, and entries in 'good' or 'indeterminate' towards the beginning. - The user can freely choose to boot any entry of the menu, including those already marked 'bad'. If the menu entry - to boot is automatically determined, this means that 'good' or 'indeterminate' entries are generally preferred as - boot entries are tried in sort order, and 'bad' entries will only be considered if there are no 'good' or + state are ordered at the beginning of the list, and entries in 'good' or 'indeterminate' at the end. The user can + freely choose to boot any entry of the menu, including those already marked 'bad'. If the menu entry to boot is + automatically determined, this means that 'good' or 'indeterminate' entries are generally preferred (as the bottom + item of the menu is the one booted by default), and 'bad' entries will only be considered if there are no 'good' or 'indeterminate' entries left. The kernel-install8 kernel diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index 82954ddac1..c2b799f007 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -1531,6 +1531,7 @@ static void config_entry_add_from_file( entry->device = device; entry->id = xstrdup(file); + StrLwr(entry->id); config_add_entry(config, entry); @@ -1642,24 +1643,13 @@ static INTN config_entry_compare(const ConfigEntry *a, const ConfigEntry *b) { assert(a); assert(b); - /* Order entries that have no tries left towards the end of the list. They have - * proven to be bad and should not be selected automatically. */ + /* Order entries that have no tries left to the beginning of the list */ if (a->tries_left != 0 && b->tries_left == 0) - return -1; - if (a->tries_left == 0 && b->tries_left != 0) return 1; + if (a->tries_left == 0 && b->tries_left != 0) + return -1; - r = strcasecmp_ptr(a->title ?: a->id, b->title ?: b->id); - if (r != 0) - return r; - - /* Sort by machine id now so that different installations don't interleave their versions. */ - r = strcasecmp_ptr(a->machine_id, b->machine_id); - if (r != 0) - return r; - - /* Reverse version comparison order so that higher versions are preferred. */ - r = strverscmp_improved(b->version, a->version); + r = strverscmp_improved(a->id, b->id); if (r != 0) return r; @@ -1667,20 +1657,19 @@ static INTN config_entry_compare(const ConfigEntry *a, const ConfigEntry *b) { b->tries_left == UINTN_MAX) return 0; - /* If both items have boot counting, and otherwise are identical, put the entry with more tries left first */ + /* If both items have boot counting, and otherwise are identical, put the entry with more tries left last */ if (a->tries_left > b->tries_left) - return -1; - if (a->tries_left < b->tries_left) return 1; + if (a->tries_left < b->tries_left) + return -1; /* If they have the same number of tries left, then let the one win which was tried fewer times so far */ if (a->tries_done < b->tries_done) - return -1; - if (a->tries_done > b->tries_done) return 1; + if (a->tries_done > b->tries_done) + return -1; - /* As a last resort, use the id (file name). */ - return strverscmp_improved(a->id, b->id); + return 0; } static UINTN config_entry_find(Config *config, const CHAR16 *needle) { @@ -1689,7 +1678,7 @@ static UINTN config_entry_find(Config *config, const CHAR16 *needle) { if (!needle) return IDX_INVALID; - for (UINTN i = 0; i < config->entry_count; i++) + for (INTN i = config->entry_count - 1; i >= 0; i--) if (MetaiMatch(config->entries[i]->id, (CHAR16*) needle)) return i; @@ -1724,8 +1713,9 @@ static void config_default_entry_select(Config *config) { return; } - /* Select the first suitable entry. */ - for (i = 0; i < config->entry_count; i++) { + /* select the last suitable entry */ + i = config->entry_count; + while (i--) { if (config->entries[i]->type == LOADER_AUTO || config->entries[i]->call) continue; config->idx_default = i; @@ -1876,6 +1866,8 @@ static ConfigEntry *config_entry_add_loader( .tries_left = UINTN_MAX, }; + StrLwr(entry->id); + config_add_entry(config, entry); return entry; } @@ -2449,14 +2441,12 @@ static void config_load_all_entries( /* Similar, but on any XBOOTLDR partition */ config_load_xbootldr(config, loaded_image->DeviceHandle); - /* Add these now, so they get sorted with the rest. */ - config_entry_add_osx(config); - config_entry_add_windows(config, loaded_image->DeviceHandle, root_dir); - /* sort entries after version number */ sort_pointer_array((void **) config->entries, config->entry_count, (compare_pointer_func_t) config_entry_compare); /* if we find some well-known loaders, add them to the end of the list */ + config_entry_add_osx(config); + config_entry_add_windows(config, loaded_image->DeviceHandle, root_dir); config_entry_add_loader_auto(config, loaded_image->DeviceHandle, root_dir, NULL, L"auto-efi-shell", 's', L"EFI Shell", L"\\shell" EFI_MACHINE_TYPE_NAME ".efi"); config_entry_add_loader_auto(config, loaded_image->DeviceHandle, root_dir, loaded_image_path, From 10119357855c1b2b5c6c405ef7008435b7821bb7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 15 Feb 2022 14:19:40 +0100 Subject: [PATCH 2/7] docs: add new "sort-key" field to boot loader spec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This allows snippet generators to explicitly order entries: any string can be set as an entry's "sort key". If set, sd-boot will use it to sort entries on display. New logic is hence (ignore the boot counting logic) sort-key is set → primary sort key: sort-key (lexicographically increasing order) → secondary sort key: machine-id (also increasing order) → tertiary sort key: version (lexicographically decreasing order!) sort-key is not set → primary sort key: entry filename (aka id), lexicographically increasing order) With this scheme we can order OSes by their names from A-Z but then put within the same OS still the newest version first. This should clean up the order to match expectations more. Based on discussions here: https://github.com/systemd/systemd/pull/22391#issuecomment-1040092633 --- docs/BOOT_LOADER_SPECIFICATION.md | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/docs/BOOT_LOADER_SPECIFICATION.md b/docs/BOOT_LOADER_SPECIFICATION.md index 8653a5bc00..7f15a23164 100644 --- a/docs/BOOT_LOADER_SPECIFICATION.md +++ b/docs/BOOT_LOADER_SPECIFICATION.md @@ -232,6 +232,16 @@ spaces from its value. The following keys are known: other installed operating systems. This ID shall be formatted as 32 lower case hexadecimal characters (i.e. without any UUID formatting). This key is optional. Example: `4098b3f648d74c13b1f04ccfba7798e8`. +* `sort-key` shall contain a short string used for sorting entries on + display. This can be defined freely though should typically be initialized + from `IMAGE_ID=` or `ID=` from `/etc/os-release` of the relevant entry, + possibly suffixed. This field is optional. If set, it is used as primary + sorting key for the entries on display (lexicographically increasing). It + does not have to be unique (and usually is not). If non-unique the the + `machine-id` (lexicographically increasing) and `version` (lexicographically + decreasing, i.e. newest version first) fields described above are used as + secondary/ternary sorting keys. If this field is not set entries are + typically sorted by the `.conf` file name of the entry. * `linux` refers to the Linux kernel to spawn and shall be a path relative to `$BOOT`. It is recommended that every distribution creates a machine id and version specific subdirectory below `$BOOT` and places its kernels and @@ -269,8 +279,9 @@ key and is otherwise not valid. Here's an example for a complete drop-in file: # /boot/loader/entries/6a9857a393724b7a981ebb5b8495b9ea-3.8.0-2.fc19.x86_64.conf title Fedora 19 (Rawhide) - version 3.8.0-2.fc19.x86_64 + sort-key fedora machine-id 6a9857a393724b7a981ebb5b8495b9ea + version 3.8.0-2.fc19.x86_64 options root=UUID=6d3376e4-fc93-4509-95ec-a21d68011da2 architecture x64 linux /6a9857a393724b7a981ebb5b8495b9ea/3.8.0-2.fc19.x86_64/linux @@ -358,10 +369,10 @@ simply reads all files `$BOOT/loader/entries/*.conf`, and populates its boot menu with this. On EFI, it then extends this with any unified kernel images found in `$BOOT/EFI/Linux/*.efi`. It may also add additional entries, for example a "Reboot into firmware" option. Optionally it may sort the menu based -on the `machine-id` and `version` fields, and possibly others. It uses the file -name to identify specific items, for example in case it supports storing away -default entry information somewhere. A boot loader should generally not modify -these files. +on the `sort-key`, `machine-id` and `version` fields, and possibly others. It +uses the file name to identify specific items, for example in case it supports +storing away default entry information somewhere. A boot loader should +generally not modify these files. For "Boot Loader Specification Entries" (Type #1), the _kernel package installer_ installs the kernel and initrd images to `$BOOT` (it is recommended From 20ec8f534f90c94669ac8f9a50869f22f94fd4c8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 15 Feb 2022 14:24:53 +0100 Subject: [PATCH 3/7] sd-boot: make use of new "sort-key" boot loader spec field --- src/boot/bootctl.c | 2 + src/boot/efi/boot.c | 77 +++++++++++++----- src/fundamental/bootspec-fundamental.c | 20 +++-- src/fundamental/bootspec-fundamental.h | 5 +- src/shared/bootspec.c | 105 +++++++++++++++++-------- src/shared/bootspec.h | 1 + 6 files changed, 150 insertions(+), 60 deletions(-) diff --git a/src/boot/bootctl.c b/src/boot/bootctl.c index e2900291bf..557b66b1e5 100644 --- a/src/boot/bootctl.c +++ b/src/boot/bootctl.c @@ -590,6 +590,8 @@ static int boot_entry_show( printf(" source: %s\n", link ?: e->path); } + if (e->sort_key) + printf(" sort-key: %s\n", e->sort_key); if (e->version) printf(" version: %s\n", e->version); if (e->machine_id) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index c2b799f007..1f254198ee 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -53,6 +53,7 @@ typedef struct { CHAR16 *id; /* The unique identifier for this entry (typically the filename of the file defining the entry) */ CHAR16 *title_show; /* The string to actually display (this is made unique before showing) */ CHAR16 *title; /* The raw (human readable) title string of the entry (not necessarily unique) */ + CHAR16 *sort_key; /* The string to use as primary sory key, usually ID= from os-release, possibly suffixed */ CHAR16 *version; /* The raw (human readable) version string of the entry */ CHAR16 *machine_id; EFI_HANDLE *device; @@ -540,6 +541,7 @@ static void print_status(Config *config, CHAR16 *loaded_image_path) { ps_string(L" id: %s\n", entry->id); ps_string(L" title: %s\n", entry->title); ps_string(L" title show: %s\n", streq_ptr(entry->title, entry->title_show) ? NULL : entry->title_show); + ps_string(L" sort key: %s\n", entry->sort_key); ps_string(L" version: %s\n", entry->version); ps_string(L" machine-id: %s\n", entry->machine_id); if (entry->device) @@ -1026,6 +1028,7 @@ static void config_entry_free(ConfigEntry *entry) { FreePool(entry->id); FreePool(entry->title_show); FreePool(entry->title); + FreePool(entry->sort_key); FreePool(entry->version); FreePool(entry->machine_id); FreePool(entry->loader); @@ -1427,6 +1430,12 @@ static void config_entry_add_from_file( continue; } + if (strcmpa((CHAR8 *)"sort-key", key) == 0) { + FreePool(entry->sort_key); + entry->sort_key = xstra_to_str(value); + continue; + } + if (strcmpa((CHAR8 *)"version", key) == 0) { FreePool(entry->version); entry->version = xstra_to_str(value); @@ -1643,12 +1652,39 @@ static INTN config_entry_compare(const ConfigEntry *a, const ConfigEntry *b) { assert(a); assert(b); - /* Order entries that have no tries left to the beginning of the list */ - if (a->tries_left != 0 && b->tries_left == 0) - return 1; + /* Order entries that have no tries left to the end of the list */ if (a->tries_left == 0 && b->tries_left != 0) + return 1; + if (a->tries_left != 0 && b->tries_left == 0) return -1; + /* If there's a sort key defined for *both* entries, then we do new-style ordering, i.e. by + * sort-key/machine-id/version, with a final fallback to id. If there's no sort key for either, we do + * old-style ordering, i.e. by id only. If one has sort key and the other does not, we put new-style + * before old-style. */ + r = CMP(!a->sort_key, !b->sort_key); + if (r != 0) /* one is old-style, one new-style */ + return r; + if (a->sort_key && b->sort_key) { + + r = strcmp(a->sort_key, b->sort_key); + if (r != 0) + return r; + + /* If multiple installations of the same OS are around, group by machine ID */ + r = strcmp_ptr(a->machine_id, b->machine_id); + if (r != 0) + return r; + + /* If the sort key was defined, then order by version now (downwards, putting the newest first) */ + r = -strverscmp_improved(a->version, b->version); + if (r != 0) + return r; + } + + /* Now order by ID (the version is likely part of the ID, thus note that this might put the oldest + * version last, not first, i.e. specifying a sort key explicitly is thus generally preferable, to + * take benefit of the explicit sorting above.) */ r = strverscmp_improved(a->id, b->id); if (r != 0) return r; @@ -1657,16 +1693,16 @@ static INTN config_entry_compare(const ConfigEntry *a, const ConfigEntry *b) { b->tries_left == UINTN_MAX) return 0; - /* If both items have boot counting, and otherwise are identical, put the entry with more tries left last */ - if (a->tries_left > b->tries_left) - return 1; + /* If both items have boot counting, and otherwise are identical, put the entry with more tries left first */ if (a->tries_left < b->tries_left) + return 1; + if (a->tries_left > b->tries_left) return -1; /* If they have the same number of tries left, then let the one win which was tried fewer times so far */ - if (a->tries_done < b->tries_done) - return 1; if (a->tries_done > b->tries_done) + return 1; + if (a->tries_done < b->tries_done) return -1; return 0; @@ -1678,7 +1714,7 @@ static UINTN config_entry_find(Config *config, const CHAR16 *needle) { if (!needle) return IDX_INVALID; - for (INTN i = config->entry_count - 1; i >= 0; i--) + for (UINTN i = 0; i < config->entry_count; i++) if (MetaiMatch(config->entries[i]->id, (CHAR16*) needle)) return i; @@ -1713,9 +1749,8 @@ static void config_default_entry_select(Config *config) { return; } - /* select the last suitable entry */ - i = config->entry_count; - while (i--) { + /* select the first suitable entry */ + for (i = 0; i < config->entry_count; i++) { if (config->entries[i]->type == LOADER_AUTO || config->entries[i]->call) continue; config->idx_default = i; @@ -1843,6 +1878,7 @@ static ConfigEntry *config_entry_add_loader( CHAR16 key, const CHAR16 *title, const CHAR16 *loader, + const CHAR16 *sort_key, const CHAR16 *version) { ConfigEntry *entry; @@ -1861,6 +1897,7 @@ static ConfigEntry *config_entry_add_loader( .device = device, .loader = xstrdup(loader), .id = xstrdup(id), + .sort_key = xstrdup(sort_key), .key = key, .tries_done = UINTN_MAX, .tries_left = UINTN_MAX, @@ -1935,7 +1972,7 @@ static ConfigEntry *config_entry_add_loader_auto( if (EFI_ERROR(err)) return NULL; - return config_entry_add_loader(config, device, LOADER_AUTO, id, key, title, loader, NULL); + return config_entry_add_loader(config, device, LOADER_AUTO, id, key, title, loader, NULL, NULL); } static void config_entry_add_osx(Config *config) { @@ -2113,7 +2150,7 @@ static void config_entry_add_linux( _cleanup_freepool_ CHAR16 *os_pretty_name = NULL, *os_image_id = NULL, *os_name = NULL, *os_id = NULL, *os_image_version = NULL, *os_version = NULL, *os_version_id = NULL, *os_build_id = NULL, *path = NULL; - const CHAR16 *good_name, *good_version; + const CHAR16 *good_name, *good_version, *good_sort_key; _cleanup_freepool_ CHAR8 *content = NULL; UINTN offs[_SECTION_MAX] = {}; UINTN szs[_SECTION_MAX] = {}; @@ -2194,7 +2231,7 @@ static void config_entry_add_linux( } } - if (!bootspec_pick_name_version( + if (!bootspec_pick_name_version_sort_key( os_pretty_name, os_image_id, os_name, @@ -2204,7 +2241,8 @@ static void config_entry_add_linux( os_version_id, os_build_id, &good_name, - &good_version)) + &good_version, + &good_sort_key)) continue; path = xpool_print(L"\\EFI\\Linux\\%s", f->FileName); @@ -2212,10 +2250,11 @@ static void config_entry_add_linux( config, device, LOADER_UNIFIED_LINUX, - f->FileName, + /* id= */ f->FileName, /* key= */ 'l', - good_name, - path, + /* title= */ good_name, + /* loader= */ path, + /* sort_key= */ good_sort_key, good_version); config_entry_parse_tries(entry, L"\\EFI\\Linux", f->FileName, L".efi"); diff --git a/src/fundamental/bootspec-fundamental.c b/src/fundamental/bootspec-fundamental.c index 9c4aee9744..89e29f5982 100644 --- a/src/fundamental/bootspec-fundamental.c +++ b/src/fundamental/bootspec-fundamental.c @@ -2,7 +2,7 @@ #include "bootspec-fundamental.h" -sd_bool bootspec_pick_name_version( +sd_bool bootspec_pick_name_version_sort_key( const sd_char *os_pretty_name, const sd_char *os_image_id, const sd_char *os_name, @@ -12,12 +12,14 @@ sd_bool bootspec_pick_name_version( const sd_char *os_version_id, const sd_char *os_build_id, const sd_char **ret_name, - const sd_char **ret_version) { + const sd_char **ret_version, + const sd_char **ret_sort_key) { - const sd_char *good_name, *good_version; + const sd_char *good_name, *good_version, *good_sort_key; - /* Find the best human readable title and version string for a boot entry (using the os-release(5) - * fields). Precise is preferred over vague, and human readable over machine readable. Thus: + /* Find the best human readable title, version string and sort key for a boot entry (using the + * os-release(5) fields). Precise is preferred over vague, and human readable over machine + * readable. Thus: * * 1. First priority gets the PRETTY_NAME field, which is the primary string intended for display, * and should already contain both a nice description and a version indication (if that concept @@ -37,11 +39,12 @@ sd_bool bootspec_pick_name_version( * which case the version is shown too. * * Note that name/version determined here are used only for display purposes. Boot entry preference - * sorting (i.e. algorithmic ordering of boot entries) is done based on the order of the entry "id" - * string (i.e. not on os-release(5) data). */ + * sorting (i.e. algorithmic ordering of boot entries) is done based on the order of the sort key (if + * defined) or entry "id" string (i.e. entry file name) otherwise. */ good_name = os_pretty_name ?: (os_image_id ?: (os_name ?: os_id)); good_version = os_image_version ?: (os_version ?: (os_version_id ? : os_build_id)); + good_sort_key = os_image_id ?: os_id; if (!good_name || !good_version) return sd_false; @@ -52,5 +55,8 @@ sd_bool bootspec_pick_name_version( if (ret_version) *ret_version = good_version; + if (ret_sort_key) + *ret_sort_key = good_sort_key; + return sd_true; } diff --git a/src/fundamental/bootspec-fundamental.h b/src/fundamental/bootspec-fundamental.h index 2cb6d7daa1..ff88f8bc3f 100644 --- a/src/fundamental/bootspec-fundamental.h +++ b/src/fundamental/bootspec-fundamental.h @@ -3,7 +3,7 @@ #include "types-fundamental.h" -sd_bool bootspec_pick_name_version( +sd_bool bootspec_pick_name_version_sort_key( const sd_char *os_pretty_name, const sd_char *os_image_id, const sd_char *os_name, @@ -13,4 +13,5 @@ sd_bool bootspec_pick_name_version( const sd_char *os_version_id, const sd_char *os_build_id, const sd_char **ret_name, - const sd_char **ret_version); + const sd_char **ret_version, + const sd_char **ret_sort_key); diff --git a/src/shared/bootspec.c b/src/shared/bootspec.c index 524f0ccfcd..99abeac34b 100644 --- a/src/shared/bootspec.c +++ b/src/shared/bootspec.c @@ -45,6 +45,7 @@ static void boot_entry_free(BootEntry *entry) { free(entry->root); free(entry->title); free(entry->show_title); + free(entry->sort_key); free(entry->version); free(entry->machine_id); free(entry->architecture); @@ -131,6 +132,8 @@ static int boot_entry_load( if (streq(field, "title")) r = free_and_strdup(&tmp.title, p); + else if (streq(field, "sort-key")) + r = free_and_strdup(&tmp.sort_key, p); else if (streq(field, "version")) r = free_and_strdup(&tmp.version, p); else if (streq(field, "machine-id")) @@ -263,6 +266,28 @@ static int boot_loader_read_conf(const char *path, BootConfig *config) { } static int boot_entry_compare(const BootEntry *a, const BootEntry *b) { + int r; + + assert(a); + assert(b); + + r = CMP(!a->sort_key, !b->sort_key); + if (r != 0) + return r; + if (a->sort_key && b->sort_key) { + r = strcmp(a->sort_key, b->sort_key); + if (r != 0) + return r; + + r = strcmp_ptr(a->machine_id, b->machine_id); + if (r != 0) + return r; + + r = -strverscmp_improved(a->version, b->version); + if (r != 0) + return r; + } + return strverscmp_improved(a->id, b->id); } @@ -311,7 +336,7 @@ static int boot_entry_load_unified( _cleanup_(boot_entry_free) BootEntry tmp = { .type = BOOT_ENTRY_UNIFIED, }; - const char *k, *good_name, *good_version; + const char *k, *good_name, *good_version, *good_sort_key; _cleanup_fclose_ FILE *f = NULL; int r; @@ -339,7 +364,7 @@ static int boot_entry_load_unified( if (r < 0) return log_error_errno(r, "Failed to parse os-release data from unified kernel image %s: %m", path); - if (!bootspec_pick_name_version( + if (!bootspec_pick_name_version_sort_key( os_pretty_name, os_image_id, os_name, @@ -349,7 +374,8 @@ static int boot_entry_load_unified( os_version_id, os_build_id, &good_name, - &good_version)) + &good_version, + &good_sort_key)) return log_error_errno(SYNTHETIC_ERRNO(EBADMSG), "Missing fields in os-release data from unified kernel image %s, refusing.", path); r = path_extract_filename(path, &tmp.id); @@ -387,6 +413,10 @@ static int boot_entry_load_unified( if (!tmp.title) return log_oom(); + tmp.sort_key = strdup(good_sort_key); + if (!tmp.sort_key) + return log_oom(); + tmp.version = strdup(good_version); if (!tmp.version) return log_oom(); @@ -634,6 +664,19 @@ static int boot_entries_uniquify(BootEntry *entries, size_t n_entries) { return 0; } +static int boot_config_find(const BootConfig *config, const char *id) { + assert(config); + + if (!id) + return -1; + + for (size_t i = 0; i < config->n_entries; i++) + if (fnmatch(id, config->entries[i].id, FNM_CASEFOLD) == 0) + return i; + + return -1; +} + static int boot_entries_select_default(const BootConfig *config) { int i; @@ -645,47 +688,45 @@ static int boot_entries_select_default(const BootConfig *config) { return -1; /* -1 means "no default" */ } - if (config->entry_oneshot) - for (i = config->n_entries - 1; i >= 0; i--) - if (streq(config->entry_oneshot, config->entries[i].id)) { - log_debug("Found default: id \"%s\" is matched by LoaderEntryOneShot", - config->entries[i].id); - return i; - } + if (config->entry_oneshot) { + i = boot_config_find(config, config->entry_oneshot); + if (i >= 0) { + log_debug("Found default: id \"%s\" is matched by LoaderEntryOneShot", + config->entries[i].id); + return i; + } + } - if (config->entry_default) - for (i = config->n_entries - 1; i >= 0; i--) - if (streq(config->entry_default, config->entries[i].id)) { - log_debug("Found default: id \"%s\" is matched by LoaderEntryDefault", - config->entries[i].id); - return i; - } + if (config->entry_default) { + i = boot_config_find(config, config->entry_default); + if (i >= 0) { + log_debug("Found default: id \"%s\" is matched by LoaderEntryDefault", + config->entries[i].id); + return i; + } + } - if (config->default_pattern) - for (i = config->n_entries - 1; i >= 0; i--) - if (fnmatch(config->default_pattern, config->entries[i].id, FNM_CASEFOLD) == 0) { - log_debug("Found default: id \"%s\" is matched by pattern \"%s\"", - config->entries[i].id, config->default_pattern); - return i; - } + if (config->default_pattern) { + i = boot_config_find(config, config->default_pattern); + if (i >= 0) { + log_debug("Found default: id \"%s\" is matched by pattern \"%s\"", + config->entries[i].id, config->default_pattern); + return i; + } + } - log_debug("Found default: last entry \"%s\"", config->entries[config->n_entries - 1].id); - return config->n_entries - 1; + log_debug("Found default: first entry \"%s\"", config->entries[0].id); + return 0; } static int boot_entries_select_selected(const BootConfig *config) { - assert(config); assert(config->entries || config->n_entries == 0); if (!config->entry_selected || config->n_entries == 0) return -1; - for (int i = config->n_entries - 1; i >= 0; i--) - if (streq(config->entry_selected, config->entries[i].id)) - return i; - - return -1; + return boot_config_find(config, config->entry_selected); } static int boot_load_efi_entry_pointers(BootConfig *config) { diff --git a/src/shared/bootspec.h b/src/shared/bootspec.h index 16353746ae..64052af593 100644 --- a/src/shared/bootspec.h +++ b/src/shared/bootspec.h @@ -28,6 +28,7 @@ typedef struct BootEntry { char *root; /* The root path in which the drop-in was found, i.e. to which 'kernel', 'efi' and 'initrd' are relative */ char *title; char *show_title; + char *sort_key; char *version; char *machine_id; char *architecture; From cf5d9598b68e9f57247fa0f148711a04f610771b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 15 Feb 2022 14:26:50 +0100 Subject: [PATCH 4/7] sd-boot: add comments highlighting type 1 vs. type 2 --- src/boot/efi/boot.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index 1f254198ee..ae25284527 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -1619,6 +1619,8 @@ static void config_load_entries( assert(device); assert(root_dir); + /* Adds Boot Loader Type #1 entries (i.e. /loader/entries/….conf) */ + err = open_directory(root_dir, L"\\loader\\entries", &entries_dir); if (EFI_ERROR(err)) return; @@ -2126,6 +2128,8 @@ static void config_entry_add_linux( UINTN f_size = 0; EFI_STATUS err; + /* Adds Boot Loader Type #2 entries (i.e. /EFI/Linux/….efi) */ + assert(config); assert(device); assert(root_dir); From d23b3bfdd6bf476de38ad620c10e8b30b488feca Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 15 Feb 2022 14:27:01 +0100 Subject: [PATCH 5/7] kernel-install: automatically generate "sort-key" field Let's order by IMAGE_ID=/ID= by default. --- src/kernel-install/90-loaderentry.install | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/kernel-install/90-loaderentry.install b/src/kernel-install/90-loaderentry.install index c1d69aa824..0e57df775f 100644 --- a/src/kernel-install/90-loaderentry.install +++ b/src/kernel-install/90-loaderentry.install @@ -62,6 +62,9 @@ fi [ -n "$PRETTY_NAME" ] || PRETTY_NAME="Linux $KERNEL_VERSION" +SORT_KEY="$IMAGE_ID" +[ -z "$SORT_KEY" ] && SORT_KEY="$ID" + if [ -r /etc/kernel/cmdline ]; then BOOT_OPTIONS="$(tr -s "$IFS" ' ' Date: Mon, 7 Mar 2022 18:00:55 +0100 Subject: [PATCH 6/7] test: add test that verifies correct order of boot entries --- src/test/meson.build | 2 + src/test/test-bootspec.c | 96 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+) create mode 100644 src/test/test-bootspec.c diff --git a/src/test/meson.build b/src/test/meson.build index 88164f1262..1de502db8b 100644 --- a/src/test/meson.build +++ b/src/test/meson.build @@ -513,6 +513,8 @@ tests += [ [files('test-strbuf.c')], + [files('test-bootspec.c')], + [files('test-strv.c')], [files('test-path-util.c')], diff --git a/src/test/test-bootspec.c b/src/test/test-bootspec.c new file mode 100644 index 0000000000..47d0f58c96 --- /dev/null +++ b/src/test/test-bootspec.c @@ -0,0 +1,96 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ + +#include "bootspec.h" +#include "fileio.h" +#include "path-util.h" +#include "rm-rf.h" +#include "tests.h" +#include "tmpfile-util.h" + +TEST_RET(bootspec_sort) { + + static const struct { + const char *fname; + const char *contents; + } entries[] = { + { + .fname = "a-10.conf", + .contents = + "title A\n" + "version 10\n" + "machine-id dd235d00696545768f6f693bfd23b15f\n", + }, + { + .fname = "a-5.conf", + .contents = + "title A\n" + "version 5\n" + "machine-id dd235d00696545768f6f693bfd23b15f\n", + }, + { + .fname = "b.conf", + .contents = + "title B\n" + "version 3\n" + "machine-id b75451ad92f94feeab50b0b442768dbd\n", + }, + { + .fname = "c.conf", + .contents = + "title C\n" + "sort-key xxxx\n" + "version 5\n" + "machine-id 309de666fd5044268a9a26541ac93176\n", + }, + { + .fname = "cx.conf", + .contents = + "title C\n" + "sort-key xxxx\n" + "version 10\n" + "machine-id 309de666fd5044268a9a26541ac93176\n", + }, + { + .fname = "d.conf", + .contents = + "title D\n" + "sort-key kkkk\n" + "version 100\n" + "machine-id 81c6e3147cf544c19006af023e22b292\n", + }, + }; + + _cleanup_(rm_rf_physical_and_freep) char *d = NULL; + _cleanup_(boot_config_free) BootConfig config = {}; + + assert_se(mkdtemp_malloc("/tmp/bootspec-testXXXXXX", &d) >= 0); + + for (size_t i = 0; i < ELEMENTSOF(entries); i++) { + _cleanup_free_ char *j = NULL; + + j = path_join(d, "/loader/entries/", entries[i].fname); + assert_se(j); + + assert_se(write_string_file(j, entries[i].contents, WRITE_STRING_FILE_CREATE|WRITE_STRING_FILE_MKDIR_0755) >= 0); + } + + assert_se(boot_entries_load_config(d, NULL, &config) >= 0); + + assert_se(config.n_entries == 6); + + /* First, because has sort key, and its the lowest one */ + assert_se(streq(config.entries[0].id, "d.conf")); + + /* These two have a sort key, and newest must be first */ + assert_se(streq(config.entries[1].id, "cx.conf")); + assert_se(streq(config.entries[2].id, "c.conf")); + + /* The following ones have no sort key, hence order by version compared ids, lowest first */ + assert_se(streq(config.entries[3].id, "a-5.conf")); + assert_se(streq(config.entries[4].id, "a-10.conf")); + assert_se(streq(config.entries[5].id, "b.conf")); + + return 0; +} + +DEFINE_TEST_MAIN(LOG_INFO); From f620a36865b9525a3344f321f9eec8547eb37ee3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 15 Mar 2022 14:43:58 +0100 Subject: [PATCH 7/7] update TODO --- TODO | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/TODO b/TODO index cdca521ad2..b4ee6d135c 100644 --- a/TODO +++ b/TODO @@ -1129,6 +1129,10 @@ Features: - teach it to prepare an ESP wholesale, i.e. with mkfs.vfat invocation - teach it to copy in unified kernel images and maybe type #1 boot loader spec entries from host - make it operate on loopback files, dissecting enough to find ESP to operate on + - bootspec: properly support boot attempt counters when parsing entry file names + +* kernel-install: + - optionally, support generating type #2 entries instead of type #1, including signing them * logind: - logind: optionally, ignore idle-hint logic for autosuspend, block suspend as long as a session is around