Merge pull request systemd#21285 from poettering/boot-os-rel-fix

sd-boot/bootspec: os-release parsing fixes

src/boot/efi/boot.c

@@ -4,6 +4,7 @@
 #include <efigpt.h>
 #include <efilib.h>
 
+#include "bootspec-fundamental.h"
 #include "console.h"
 #include "devicetree.h"
 #include "disk.h"
@@ -28,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[] =
@@ -44,10 +46,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;
@@ -1651,14 +1653,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;
@@ -1732,13 +1734,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))
@@ -2044,8 +2042,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] = {};
@@ -2077,82 +2077,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((const CHAR8*) "IMAGE_ID", key) == 0) {
+                                FreePool(os_image_id);
+                                os_image_id = stra_to_str(value);
                                 continue;
                         }
 
-                        if (strcmpa((CHAR8 *)"NAME", key) == 0) {
+                        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) {
+                        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((CHAR8 *)"VERSION_ID", key) == 0) {
+                        if (strcmpa((const CHAR8*) "VERSION_ID", key) == 0) {
                                 FreePool(os_version_id);
                                 os_version_id = stra_to_str(value);
                                 continue;
                         }
 
-                        if (strcmpa((CHAR8 *)"BUILD_ID", key) == 0) {
+                        if (strcmpa((const 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 ((os_name_pretty || os_name) && os_id && (os_image_version || os_version || os_version_id || os_build_id)) {
-                        _cleanup_freepool_ CHAR16 *path = NULL;
-
-                        path = PoolPrint(L"\\EFI\\Linux\\%s", f->FileName);
-
-                        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)));
-
-                        config_entry_parse_tries(entry, L"\\EFI\\Linux", f->FileName, L".efi");
-
-                        if (szs[SECTION_CMDLINE] == 0)
-                                continue;
+                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;
 
-                        FreePool(content);
-                        content = NULL;
+                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',
+                                good_name,
+                                path,
+                                good_version);
+                if (!entry)
+                        return (void) log_oom();
+
+                config_entry_parse_tries(entry, L"\\EFI\\Linux", f->FileName, L".efi");
+
+                if (szs[SECTION_CMDLINE] == 0)
+                        continue;
 
-                        /* read the embedded cmdline file */
-                        err = file_read(linux_dir, f->FileName, offs[SECTION_CMDLINE], szs[SECTION_CMDLINE], &content, NULL);
-                        if (!EFI_ERROR(err)) {
+                content = mfree(content);
 
-                                /* chomp the newline */
-                                if (content[szs[SECTION_CMDLINE] - 1] == '\n')
-                                        content[szs[SECTION_CMDLINE] - 1] = '\0';
+                /* 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';
 
-                                entry->options = stra_to_str(content);
-                        }
+                        entry->options = stra_to_str(content);
+                        if (!entry->options)
+                                return (void) log_oom();
                 }
         }
 }
@@ -2480,7 +2499,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;
                 }
 

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 sd_false;
+
+        if (ret_name)
+                *ret_name = good_name;
+
+        if (ret_version)
+                *ret_version = good_version;
+
+        return sd_true;
+}

src/fundamental/bootspec-fundamental.h

@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+#pragma once
+
+#include "types-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);

src/fundamental/macro-fundamental.h

@@ -6,7 +6,7 @@
 #endif
 
 #include <limits.h>
-#include "type.h"
+#include "types-fundamental.h"
 
 #define _align_(x) __attribute__((__aligned__(x)))
 #define _const_ __attribute__((__const__))
@@ -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;                  \

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',
-        'type.h')
+        'string-util-fundamental.h',
+        'types-fundamental.h')
 
 sources = '''
+        bootspec-fundamental.c
         efivars-fundamental.c
         string-util-fundamental.c
         sha256.c

src/fundamental/sha256.h

@@ -6,7 +6,7 @@
 #include <efilib.h>
 #endif
 
-#include "type.h"
+#include "types-fundamental.h"
 
 struct sha256_ctx {
         uint32_t H[8];