From 08cd8c4307a9facc7cc8acbdbe06837869725dd0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 9 Nov 2021 23:47:45 +0100 Subject: [PATCH 01/10] boot: add comments what closely related ConfigEntry fields are about --- src/boot/efi/boot.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index 5992a28cdd..9dd14bd7b3 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -44,10 +44,10 @@ enum loader_type { }; typedef struct { - CHAR16 *id; /* The unique identifier for this entry */ - CHAR16 *title_show; - CHAR16 *title; - CHAR16 *version; + 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 *version; /* The raw (human readable) version string of the entry */ CHAR16 *machine_id; EFI_HANDLE *device; enum loader_type type; From b78a282181fc6bb347a682fe07c8d5579d0c4f3d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 9 Nov 2021 23:48:37 +0100 Subject: [PATCH 02/10] boot: ternary op is your friend --- src/boot/efi/boot.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index 9dd14bd7b3..ad0c71cb21 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -1732,13 +1732,9 @@ static void config_title_generate(Config *config) { /* set title */ for (UINTN i = 0; i < config->entry_count; i++) { - CHAR16 *title; - FreePool(config->entries[i]->title_show); - title = config->entries[i]->title; - if (!title) - title = config->entries[i]->id; - config->entries[i]->title_show = StrDuplicate(title); + config->entries[i]->title_show = StrDuplicate( + config->entries[i]->title ?: config->entries[i]->id); } if (!find_nonunique(config->entries, config->entry_count)) From 1ce915a91c84fd775de93c54f4eb2e8735e18153 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 9 Nov 2021 23:49:01 +0100 Subject: [PATCH 03/10] boot: const arguments should be const Unfortunately they forgot the "const" decoration on the MetaiMatch() prototype, but let that omission not leak into our code, let's hide it away in the innermost use. --- src/boot/efi/boot.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index ad0c71cb21..68fb94cbdc 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -1651,14 +1651,14 @@ static void config_sort_entries(Config *config) { sort_pointer_array((void**) config->entries, config->entry_count, (compare_pointer_func_t) config_entry_compare); } -static INTN config_entry_find(Config *config, CHAR16 *needle) { +static INTN config_entry_find(Config *config, const CHAR16 *needle) { assert(config); if (!needle) return -1; for (UINTN i = 0; i < config->entry_count; i++) - if (MetaiMatch(config->entries[i]->id, needle)) + if (MetaiMatch(config->entries[i]->id, (CHAR16*) needle)) return (INTN) i; return -1; From 31f7765762105f82eacecfc6f510dc68b070fef7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 9 Nov 2021 23:51:35 +0100 Subject: [PATCH 04/10] bootspec: fix comment that says exactly the opposite of what is true --- src/shared/bootspec.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/bootspec.h b/src/shared/bootspec.h index c7b4a2ee02..81845f47e3 100644 --- a/src/shared/bootspec.h +++ b/src/shared/bootspec.h @@ -20,7 +20,7 @@ typedef enum BootEntryType { typedef struct BootEntry { BootEntryType type; - char *id; /* This is the file basename without extension */ + char *id; /* This is the file basename (including extension!) */ char *id_old; /* Old-style ID, for deduplication purposes. */ char *path; /* This is the full path to the drop-in file */ char *root; /* The root path in which the drop-in was found, i.e. to which 'kernel', 'efi' and 'initrd' are relative */ From 18fde343a845a909260e3443f0a0734ba00cabe0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 9 Nov 2021 23:54:10 +0100 Subject: [PATCH 05/10] boot: clean up unified boot loader entry name/version extraction Let's make sure IMAGE_ID/IMAGE_VERSION are properly honoured, and explain in a long comment why. Let's also use ID= field again, which was lost by accident. (While we are at it do some minimal OOM checks wherever we touch something) --- src/boot/efi/boot.c | 124 +++++++++++++++---------- src/fundamental/bootspec-fundamental.c | 56 +++++++++++ src/fundamental/bootspec-fundamental.h | 16 ++++ src/fundamental/meson.build | 4 +- 4 files changed, 148 insertions(+), 52 deletions(-) create mode 100644 src/fundamental/bootspec-fundamental.c create mode 100644 src/fundamental/bootspec-fundamental.h diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index 68fb94cbdc..89888c3aa9 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -4,6 +4,7 @@ #include #include +#include "bootspec-fundamental.h" #include "console.h" #include "devicetree.h" #include "disk.h" @@ -2040,8 +2041,10 @@ static void config_entry_add_linux( NULL, }; - _cleanup_freepool_ CHAR16 *os_name_pretty = NULL, *os_name = NULL, *os_id = NULL, - *os_version = NULL, *os_version_id = NULL, *os_build_id = NULL, *os_image_version = NULL; + _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; _cleanup_freepool_ CHAR8 *content = NULL; UINTN offs[_SECTION_MAX] = {}; UINTN szs[_SECTION_MAX] = {}; @@ -2073,82 +2076,101 @@ static void config_entry_add_linux( /* read properties from the embedded os-release file */ while ((line = line_get_key_value(content, (CHAR8 *)"=", &pos, &key, &value))) { - if (strcmpa((CHAR8 *)"PRETTY_NAME", key) == 0) { - FreePool(os_name_pretty); - os_name_pretty = stra_to_str(value); + if (strcmpa((const CHAR8*) "PRETTY_NAME", key) == 0) { + FreePool(os_pretty_name); + os_pretty_name = stra_to_str(value); continue; } - if (strcmpa((CHAR8 *)"NAME", key) == 0) { + if (strcmpa((const CHAR8*) "IMAGE_ID", key) == 0) { + FreePool(os_image_id); + os_image_id = stra_to_str(value); + continue; + } + + if (strcmpa((const CHAR8*) "NAME", key) == 0) { FreePool(os_name); os_name = stra_to_str(value); continue; } - if (strcmpa((CHAR8 *)"ID", key) == 0) { + if (strcmpa((const CHAR8*) "ID", key) == 0) { FreePool(os_id); os_id = stra_to_str(value); continue; } - if (strcmpa((CHAR8 *)"VERSION", key) == 0) { - FreePool(os_version); - os_version = stra_to_str(value); - continue; - } - - if (strcmpa((CHAR8 *)"VERSION_ID", key) == 0) { - FreePool(os_version_id); - os_version_id = stra_to_str(value); - continue; - } - - if (strcmpa((CHAR8 *)"BUILD_ID", key) == 0) { - FreePool(os_build_id); - os_build_id = stra_to_str(value); - continue; - } - if (strcmpa((const CHAR8*) "IMAGE_VERSION", key) == 0) { FreePool(os_image_version); os_image_version = stra_to_str(value); continue; } + + if (strcmpa((const CHAR8*) "VERSION", key) == 0) { + FreePool(os_version); + os_version = stra_to_str(value); + continue; + } + + if (strcmpa((const CHAR8*) "VERSION_ID", key) == 0) { + FreePool(os_version_id); + os_version_id = stra_to_str(value); + continue; + } + + if (strcmpa((const CHAR8*) "BUILD_ID", key) == 0) { + FreePool(os_build_id); + os_build_id = stra_to_str(value); + continue; + } } - if ((os_name_pretty || os_name) && os_id && (os_image_version || os_version || os_version_id || os_build_id)) { - _cleanup_freepool_ CHAR16 *path = NULL; + if (!bootspec_pick_name_version( + os_pretty_name, + os_image_id, + os_name, + os_id, + os_image_version, + os_version, + os_version_id, + os_build_id, + &good_name, + &good_version)) + continue; - path = PoolPrint(L"\\EFI\\Linux\\%s", f->FileName); + path = PoolPrint(L"\\EFI\\Linux\\%s", f->FileName); + if (!path) + return (void) log_oom(); - entry = config_entry_add_loader( - config, - device, - LOADER_LINUX, - f->FileName, - /* key= */ 'l', - os_name_pretty ?: os_name, - path, - os_image_version ?: (os_version ?: (os_version_id ? : os_build_id))); + entry = config_entry_add_loader( + config, + device, + LOADER_LINUX, + f->FileName, + /* key= */ 'l', + good_name, + path, + good_version); + if (!entry) + return (void) log_oom(); - config_entry_parse_tries(entry, L"\\EFI\\Linux", f->FileName, L".efi"); + config_entry_parse_tries(entry, L"\\EFI\\Linux", f->FileName, L".efi"); - if (szs[SECTION_CMDLINE] == 0) - continue; + if (szs[SECTION_CMDLINE] == 0) + continue; - FreePool(content); - content = NULL; + content = mfree(content); - /* read the embedded cmdline file */ - err = file_read(linux_dir, f->FileName, offs[SECTION_CMDLINE], szs[SECTION_CMDLINE], &content, NULL); - if (!EFI_ERROR(err)) { + /* read the embedded cmdline file */ + err = file_read(linux_dir, f->FileName, offs[SECTION_CMDLINE], szs[SECTION_CMDLINE], &content, NULL); + if (!EFI_ERROR(err)) { + /* chomp the newline */ + if (content[szs[SECTION_CMDLINE] - 1] == '\n') + content[szs[SECTION_CMDLINE] - 1] = '\0'; - /* chomp the newline */ - if (content[szs[SECTION_CMDLINE] - 1] == '\n') - content[szs[SECTION_CMDLINE] - 1] = '\0'; - - entry->options = stra_to_str(content); - } + entry->options = stra_to_str(content); + if (!entry->options) + return (void) log_oom(); } } } diff --git a/src/fundamental/bootspec-fundamental.c b/src/fundamental/bootspec-fundamental.c new file mode 100644 index 0000000000..51078397b6 --- /dev/null +++ b/src/fundamental/bootspec-fundamental.c @@ -0,0 +1,56 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ + +#include "bootspec-fundamental.h" + +sd_bool bootspec_pick_name_version( + const sd_char *os_pretty_name, + const sd_char *os_image_id, + const sd_char *os_name, + const sd_char *os_id, + const sd_char *os_image_version, + const sd_char *os_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 *good_name, *good_version; + + /* 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: + * + * 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 + * applies). + * + * 2. Otherwise we go for IMAGE_ID and IMAGE_VERSION (thus we show details about the image, + * i.e. specific combination of packages and configuration), if that concept applies. + * + * 3. Otherwise we go for NAME and VERSION (i.e. human readable OS name and version) + * + * 4. Otherwise we go for ID and VERSION_ID (i.e. machine readable OS name and version) + * + * 5. Finally, for the version we'll use BUILD_ID (i.e. a machine readable version that identifies + * the original OS build used during installation) + * + * Note that the display logic will show only the name by default, except if that isn't unique in + * 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). */ + + 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)); + + if (!good_name || !good_version) + return false; + + if (ret_name) + *ret_name = good_name; + + if (ret_version) + *ret_version = good_version; + + return true; +} diff --git a/src/fundamental/bootspec-fundamental.h b/src/fundamental/bootspec-fundamental.h new file mode 100644 index 0000000000..0a1fe5c5ba --- /dev/null +++ b/src/fundamental/bootspec-fundamental.h @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +#pragma once + +#include "type.h" + +sd_bool bootspec_pick_name_version( + const sd_char *os_pretty_name, + const sd_char *os_image_id, + const sd_char *os_name, + const sd_char *os_id, + const sd_char *os_image_version, + const sd_char *os_version, + const sd_char *os_version_id, + const sd_char *os_build_id, + const sd_char **ret_name, + const sd_char **ret_version); diff --git a/src/fundamental/meson.build b/src/fundamental/meson.build index 3c9f07b191..26859653e6 100644 --- a/src/fundamental/meson.build +++ b/src/fundamental/meson.build @@ -3,13 +3,15 @@ fundamental_path = meson.current_source_dir() fundamental_headers = files( + 'bootspec-fundamental.h', 'efivars-fundamental.h', 'macro-fundamental.h', - 'string-util-fundamental.h', 'sha256.h', + 'string-util-fundamental.h', 'type.h') sources = ''' + bootspec-fundamental.c efivars-fundamental.c string-util-fundamental.c sha256.c From df331784164226e6f9f3caf36a07a22f9ef4d19d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 9 Nov 2021 23:57:49 +0100 Subject: [PATCH 06/10] boot: when we can't boot use the right boot loader entry display title in log message --- src/boot/efi/boot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index 89888c3aa9..ccbdca2271 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -2498,7 +2498,7 @@ EFI_STATUS efi_main(EFI_HANDLE image, EFI_SYSTEM_TABLE *sys_table) { err = image_start(root_dir, image, &config, entry); if (EFI_ERROR(err)) { graphics_mode(FALSE); - log_error_stall(L"Failed to execute %s (%s): %r", entry->title, entry->loader, err); + log_error_stall(L"Failed to execute %s (%s): %r", entry->title_show, entry->loader, err); goto out; } From c2caeb5d54b6065d46f7be9dccd46ebed3775e80 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 9 Nov 2021 23:58:08 +0100 Subject: [PATCH 07/10] bootspec: catch up with sd-boot's bootspec implementation Let's parse the same fields and use them the same way as in sd-boot. Fixes: #20093 --- src/shared/bootspec.c | 49 ++++++++++++++++++++++++++++++++----------- 1 file changed, 37 insertions(+), 12 deletions(-) diff --git a/src/shared/bootspec.c b/src/shared/bootspec.c index e538e2bd87..8fdb5272e9 100644 --- a/src/shared/bootspec.c +++ b/src/shared/bootspec.c @@ -9,6 +9,7 @@ #include "alloc-util.h" #include "blkid-util.h" +#include "bootspec-fundamental.h" #include "bootspec.h" #include "conf-files.h" #include "def.h" @@ -287,13 +288,13 @@ static int boot_entry_load_unified( const char *cmdline, BootEntry *ret) { - _cleanup_free_ char *os_pretty_name = NULL, *os_id = NULL, *version_id = NULL, *build_id = NULL; + _cleanup_free_ char *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; _cleanup_(boot_entry_free) BootEntry tmp = { .type = BOOT_ENTRY_UNIFIED, }; + const char *k, *good_name, *good_version; _cleanup_fclose_ FILE *f = NULL; - const char *k; - char *b; int r; assert(root); @@ -310,24 +311,42 @@ static int boot_entry_load_unified( r = parse_env_file(f, "os-release", "PRETTY_NAME", &os_pretty_name, + "IMAGE_ID", &os_image_id, + "NAME", &os_name, "ID", &os_id, - "VERSION_ID", &version_id, - "BUILD_ID", &build_id); + "IMAGE_VERSION", &os_image_version, + "VERSION", &os_version, + "VERSION_ID", &os_version_id, + "BUILD_ID", &os_build_id); if (r < 0) return log_error_errno(r, "Failed to parse os-release data from unified kernel image %s: %m", path); - if (!os_pretty_name || !os_id || !(version_id || build_id)) + if (!bootspec_pick_name_version( + os_pretty_name, + os_image_id, + os_name, + os_id, + os_image_version, + os_version, + os_version_id, + os_build_id, + &good_name, + &good_version)) return log_error_errno(SYNTHETIC_ERRNO(EBADMSG), "Missing fields in os-release data from unified kernel image %s, refusing.", path); - b = basename(path); - tmp.id = strdup(b); - tmp.id_old = strjoin(os_id, "-", version_id ?: build_id); - if (!tmp.id || !tmp.id_old) - return log_oom(); + r = path_extract_filename(path, &tmp.id); + if (r < 0) + return log_error_errno(r, "Failed to extract file name from '%s': %m", path); if (!efi_loader_entry_name_valid(tmp.id)) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Invalid loader entry name: %s", tmp.id); + if (os_id && os_version_id) { + tmp.id_old = strjoin(os_id, "-", os_version_id); + if (!tmp.id_old) + return log_oom(); + } + tmp.path = strdup(path); if (!tmp.path) return log_oom(); @@ -346,7 +365,13 @@ static int boot_entry_load_unified( delete_trailing_chars(tmp.options[0], WHITESPACE); - tmp.title = TAKE_PTR(os_pretty_name); + tmp.title = strdup(good_name); + if (!tmp.title) + return log_oom(); + + tmp.version = strdup(good_version); + if (!tmp.version) + return log_oom(); *ret = tmp; tmp = (BootEntry) {}; From 81d9db68052f793454babf3c127eb8da16277e19 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 11 Nov 2021 15:20:51 +0100 Subject: [PATCH 08/10] boot: line-break magic[] array to match osrel[] line breaks --- src/boot/efi/boot.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index ccbdca2271..0bacfb4bbe 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -29,8 +29,9 @@ #define TEXT_ATTR_SWAP(c) EFI_TEXT_ATTR(((c) & 0b11110000) >> 4, (c) & 0b1111) -/* magic string to find in the binary image */ -_used_ _section_(".sdmagic") static const char magic[] = "#### LoaderInfo: systemd-boot " GIT_VERSION " ####"; +/* Magic string for recognizing our own binaries */ +_used_ _section_(".sdmagic") static const char magic[] = + "#### LoaderInfo: systemd-boot " GIT_VERSION " ####"; /* Makes systemd-boot available from \EFI\Linux\ for testing purposes. */ _used_ _section_(".osrel") static const char osrel[] = From e514b5071c75c1a4d25a9fc925e5574007dd0ac3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 11 Nov 2021 15:29:53 +0100 Subject: [PATCH 09/10] =?UTF-8?q?fundamental:=20rename=20type.h=20?= =?UTF-8?q?=E2=86=92=20types-fundamental.h?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "type.h" is a very generic name, but this header is very specific to making the "fundaemtnal" stuff work, it maps genric types in two distinct ways. Hence let's make clear in the header name already what this is about. --- src/fundamental/bootspec-fundamental.h | 2 +- src/fundamental/macro-fundamental.h | 2 +- src/fundamental/meson.build | 2 +- src/fundamental/sha256.h | 2 +- src/fundamental/{type.h => types-fundamental.h} | 0 5 files changed, 4 insertions(+), 4 deletions(-) rename src/fundamental/{type.h => types-fundamental.h} (100%) diff --git a/src/fundamental/bootspec-fundamental.h b/src/fundamental/bootspec-fundamental.h index 0a1fe5c5ba..2cb6d7daa1 100644 --- a/src/fundamental/bootspec-fundamental.h +++ b/src/fundamental/bootspec-fundamental.h @@ -1,7 +1,7 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ #pragma once -#include "type.h" +#include "types-fundamental.h" sd_bool bootspec_pick_name_version( const sd_char *os_pretty_name, diff --git a/src/fundamental/macro-fundamental.h b/src/fundamental/macro-fundamental.h index 44af0bd0a0..c52957a55c 100644 --- a/src/fundamental/macro-fundamental.h +++ b/src/fundamental/macro-fundamental.h @@ -6,7 +6,7 @@ #endif #include -#include "type.h" +#include "types-fundamental.h" #define _align_(x) __attribute__((__aligned__(x))) #define _const_ __attribute__((__const__)) diff --git a/src/fundamental/meson.build b/src/fundamental/meson.build index 26859653e6..287f0fe36a 100644 --- a/src/fundamental/meson.build +++ b/src/fundamental/meson.build @@ -8,7 +8,7 @@ fundamental_headers = files( 'macro-fundamental.h', 'sha256.h', 'string-util-fundamental.h', - 'type.h') + 'types-fundamental.h') sources = ''' bootspec-fundamental.c diff --git a/src/fundamental/sha256.h b/src/fundamental/sha256.h index 9fc090b4e0..abc4167628 100644 --- a/src/fundamental/sha256.h +++ b/src/fundamental/sha256.h @@ -6,7 +6,7 @@ #include #endif -#include "type.h" +#include "types-fundamental.h" struct sha256_ctx { uint32_t H[8]; diff --git a/src/fundamental/type.h b/src/fundamental/types-fundamental.h similarity index 100% rename from src/fundamental/type.h rename to src/fundamental/types-fundamental.h From f22abf384d1d2a6d1a303502a57366df7dd4b08e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 11 Nov 2021 15:31:17 +0100 Subject: [PATCH 10/10] types-fundamental: introduce sd_true + sd_false I think we should stick to the rule that stuff defined in types-fundamental.h either: 1. adds a prefixed concept "sd_xyz" that maps differently in the two environments 2. adds a non-prefixed concept "xyz" that adds a type otherwise missing in one of the two environments but with the same definition as in the other. i.e. if have have some concept that might differ the way its set up in the two environments it really should be prefixed by "sd_" to make clear it has semantics we defined. Only drop the prefix if it really means the exact same thin in all environments. Now, sd_bool is defined prefixed, because its either mapped to "BOOLEAN" (which is an integer) in UEFI or "bool" (which is C99 _Bool) in userspace. size_t is not defined prefixed, because it's mapped to the same thing ultimately (on the UEFI its mapped to UINTN, but that in turn is defined as being the type for the size of memory objects, thus it's really the same as userspace size_t). So far "true" and "false" where defined unprefixed even though they map to values of different types. typeof(true) in userspace would reveal _Bool, but typeof(false) in UEFI would reveal BOOLEAN. The distinction actually does matter in comparisons (i.e. (_Bool) 1 == (_Bool) 2 holds while (BOOLEAN) 1 == (BOOLEAN) 2 does not hold). Hence, let's add sd_true and sd_false, thus indicating we defined our own concept here, and it has similar but different semantics in UEFI and in userspace. --- src/fundamental/bootspec-fundamental.c | 4 ++-- src/fundamental/macro-fundamental.h | 8 ++++---- src/fundamental/types-fundamental.h | 22 ++++++++++++++++++++-- 3 files changed, 26 insertions(+), 8 deletions(-) diff --git a/src/fundamental/bootspec-fundamental.c b/src/fundamental/bootspec-fundamental.c index 51078397b6..9c4aee9744 100644 --- a/src/fundamental/bootspec-fundamental.c +++ b/src/fundamental/bootspec-fundamental.c @@ -44,7 +44,7 @@ sd_bool bootspec_pick_name_version( good_version = os_image_version ?: (os_version ?: (os_version_id ? : os_build_id)); if (!good_name || !good_version) - return false; + return sd_false; if (ret_name) *ret_name = good_name; @@ -52,5 +52,5 @@ sd_bool bootspec_pick_name_version( if (ret_version) *ret_version = good_version; - return true; + return sd_true; } diff --git a/src/fundamental/macro-fundamental.h b/src/fundamental/macro-fundamental.h index c52957a55c..cd9e60cf60 100644 --- a/src/fundamental/macro-fundamental.h +++ b/src/fundamental/macro-fundamental.h @@ -85,8 +85,8 @@ #define ONCE __ONCE(UNIQ_T(_once_, UNIQ)) #define __ONCE(o) \ ({ \ - static bool (o) = false; \ - __sync_bool_compare_and_swap(&(o), false, true); \ + static bool (o) = sd_false; \ + __sync_bool_compare_and_swap(&(o), sd_false, sd_true); \ }) #undef MAX @@ -236,7 +236,7 @@ #define IN_SET(x, ...) \ ({ \ - sd_bool _found = false; \ + sd_bool _found = sd_false; \ /* If the build breaks in the line below, you need to extend the case macros. (We use "long double" as \ * type for the array, in the hope that checkers such as ubsan don't complain that the initializers for \ * the array are not representable by the base type. Ideally we'd use typeof(x) as base type, but that \ @@ -245,7 +245,7 @@ assert_cc(ELEMENTSOF(__assert_in_set) <= 20); \ switch(x) { \ FOR_EACH_MAKE_CASE(__VA_ARGS__) \ - _found = true; \ + _found = sd_true; \ break; \ default: \ break; \ diff --git a/src/fundamental/types-fundamental.h b/src/fundamental/types-fundamental.h index 2a9a114bbc..5977e40c6c 100644 --- a/src/fundamental/types-fundamental.h +++ b/src/fundamental/types-fundamental.h @@ -1,6 +1,20 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ #pragma once +/* This defines a number of basic types that are one thing in userspace and another in the UEFI environment, + * but mostly the same in concept and behaviour. + * + * Note: if the definition of these types/values has slightly different semantics in userspace and in the + * UEFI environment then please prefix its name with "sd_" to make clear these types have special semantics, + * and *we* defined them. Otherwise, if the types are effectively 100% identical in behaviour in userspace + * and UEFI environment you can omit the prefix. (Examples: sd_char is 8 bit in userspace and 16 bit in UEFI + * space hence it should have the sd_ prefix; but size_t in userspace and UINTN in UEFI environment are 100% + * defined the same way ultimately, hence it's OK to just define size_t as alias to UINTN in UEFI + * environment, so that size_t can be used everywhere, without any "sd_" prefix.) + * + * Note: we generally prefer the userspace names of types and concepts. i.e. if in doubt please name types + * after the userspace vocabulary, and let's keep UEFI vocabulary specific to the UEFI build environment. */ + #ifdef SD_BOOT #include @@ -9,8 +23,8 @@ typedef CHAR16 sd_char; typedef INTN sd_int; typedef UINTN size_t; -#define true TRUE -#define false FALSE +#define sd_true TRUE +#define sd_false FALSE #else #include #include @@ -18,4 +32,8 @@ typedef UINTN size_t; typedef bool sd_bool; typedef char sd_char; typedef int sd_int; + +#define sd_true true +#define sd_false false + #endif