From 9817b7dbc9294d86973e0e42150315955edf9d38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 18 Mar 2022 18:49:07 +0100 Subject: [PATCH 1/3] shared/bootspec: reduce scope of variables --- src/shared/bootspec.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/shared/bootspec.c b/src/shared/bootspec.c index 99abeac34b..96ec6ecb9e 100644 --- a/src/shared/bootspec.c +++ b/src/shared/bootspec.c @@ -172,8 +172,6 @@ static int boot_entry_load( } void boot_config_free(BootConfig *config) { - size_t i; - assert(config); free(config->default_pattern); @@ -189,7 +187,7 @@ void boot_config_free(BootConfig *config) { free(config->entry_default); free(config->entry_selected); - for (i = 0; i < config->n_entries; i++) + for (size_t i = 0; i < config->n_entries; i++) boot_entry_free(config->entries + i); free(config->entries); } @@ -437,12 +435,9 @@ static int find_sections( _cleanup_free_ struct PeSectionHeader *sections = NULL; _cleanup_free_ char *osrelease = NULL, *cmdline = NULL; - size_t i, n_sections; - struct DosFileHeader dos; - struct PeHeader pe; - uint64_t start; ssize_t n; + struct DosFileHeader dos; n = pread(fd, &dos, sizeof(dos), 0); if (n < 0) return log_error_errno(errno, "Failed read DOS header: %m"); @@ -452,7 +447,9 @@ static int find_sections( if (dos.Magic[0] != 'M' || dos.Magic[1] != 'Z') return log_error_errno(SYNTHETIC_ERRNO(EBADMSG), "DOS executable magic missing, refusing."); - start = unaligned_read_le32(&dos.ExeHeader); + uint64_t start = unaligned_read_le32(&dos.ExeHeader); + + struct PeHeader pe; n = pread(fd, &pe, sizeof(pe), start); if (n < 0) return log_error_errno(errno, "Failed to read PE header: %m"); @@ -462,7 +459,7 @@ static int find_sections( if (pe.Magic[0] != 'P' || pe.Magic[1] != 'E' || pe.Magic[2] != 0 || pe.Magic[3] != 0) return log_error_errno(SYNTHETIC_ERRNO(EBADMSG), "PE executable magic missing, refusing."); - n_sections = unaligned_read_le16(&pe.FileHeader.NumberOfSections); + size_t n_sections = unaligned_read_le16(&pe.FileHeader.NumberOfSections); if (n_sections > 96) return log_error_errno(SYNTHETIC_ERRNO(EBADMSG), "PE header has too many sections, refusing."); @@ -478,7 +475,7 @@ static int find_sections( if ((size_t) n != n_sections * sizeof(struct PeSectionHeader)) return log_error_errno(SYNTHETIC_ERRNO(EIO), "Short read while reading sections, refusing."); - for (i = 0; i < n_sections; i++) { + for (size_t i = 0; i < n_sections; i++) { _cleanup_free_ char *k = NULL; uint32_t offset, size; char **b; From 523487f713bcc24302623a2326bc10ff4d5f03d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 18 Mar 2022 19:00:10 +0100 Subject: [PATCH 2/3] efi: use CMP() more --- src/boot/efi/boot.c | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index ae25284527..9ed8daed42 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -1655,10 +1655,9 @@ static INTN config_entry_compare(const ConfigEntry *a, const ConfigEntry *b) { assert(b); /* 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; + r = CMP(a->tries_left == 0, b->tries_left == 0); + if (r != 0) + return r; /* 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 @@ -1667,8 +1666,8 @@ static INTN config_entry_compare(const ConfigEntry *a, const ConfigEntry *b) { 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) { + if (a->sort_key && b->sort_key) { r = strcmp(a->sort_key, b->sort_key); if (r != 0) return r; @@ -1691,23 +1690,16 @@ static INTN config_entry_compare(const ConfigEntry *a, const ConfigEntry *b) { if (r != 0) return r; - if (a->tries_left == UINTN_MAX || - b->tries_left == UINTN_MAX) + if (a->tries_left == UINTN_MAX || 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 (a->tries_left < b->tries_left) - return 1; - if (a->tries_left > b->tries_left) - return -1; + r = -CMP(a->tries_left, b->tries_left); + if (r != 0) + return r; /* 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; - - return 0; + return CMP(a->tries_done, b->tries_done); } static UINTN config_entry_find(Config *config, const CHAR16 *needle) { From 62a4b584bba4d152161f5e5974392e2017c7b42c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 18 Mar 2022 19:05:03 +0100 Subject: [PATCH 3/3] sd-boot+bootctl: invert order of entries w/o sort-key With the changes in 20ec8f534f90c94669ac8f9a50869f22f94fd4c8, we would sort entries with sort-key as expected (higher versions earlier, i.e. at the top of the menu), but entries without the sort-key as before, with higher versions later. When we have a bunch of boot entries grouped by machine-id (or even in the typical case of all boot entries having the same machine id), sorting by id should generally give good results. Entries will be grouped by installation, and then newer entries should generally be at the top of the menu. --- src/boot/efi/boot.c | 8 ++++---- src/shared/bootspec.c | 3 ++- src/test/test-bootspec.c | 4 ++-- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index 9ed8daed42..e5399f3728 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -1683,10 +1683,10 @@ static INTN config_entry_compare(const ConfigEntry *a, const ConfigEntry *b) { 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); + /* Now order by ID. The version is likely part of the ID, thus note that this will generatelly put + * the newer versions earlier. Specifying a sort key explicitly is preferable, because it gives an + * explicit sort order. */ + r = -strverscmp_improved(a->id, b->id); if (r != 0) return r; diff --git a/src/shared/bootspec.c b/src/shared/bootspec.c index 96ec6ecb9e..a87cce4ea5 100644 --- a/src/shared/bootspec.c +++ b/src/shared/bootspec.c @@ -272,6 +272,7 @@ static int boot_entry_compare(const BootEntry *a, const BootEntry *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) @@ -286,7 +287,7 @@ static int boot_entry_compare(const BootEntry *a, const BootEntry *b) { return r; } - return strverscmp_improved(a->id, b->id); + return -strverscmp_improved(a->id, b->id); } static int boot_entries_find( diff --git a/src/test/test-bootspec.c b/src/test/test-bootspec.c index 47d0f58c96..7ba44744ba 100644 --- a/src/test/test-bootspec.c +++ b/src/test/test-bootspec.c @@ -86,9 +86,9 @@ TEST_RET(bootspec_sort) { 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[3].id, "b.conf")); assert_se(streq(config.entries[4].id, "a-10.conf")); - assert_se(streq(config.entries[5].id, "b.conf")); + assert_se(streq(config.entries[5].id, "a-5.conf")); return 0; }