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)

diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c
index 68fb94cbdce7f622d4444b0d0af5f60e627cfd78..89888c3aa9a508babf56346f26337f850901e347 100644 (file)
--- a/src/boot/efi/boot.c
+++ b/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"
@@ -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) {
+                        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();
                 }
         }
 }

diff --git a/src/fundamental/bootspec-fundamental.c b/src/fundamental/bootspec-fundamental.c
new file mode 100644 (file)
index 0000000..5107839
--- /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 (file)
index 0000000..0a1fe5c
--- /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 3c9f07b191d021d562df5cb8c3cc3c0d84a0ab25..26859653e60ce1a4603aebac03f82886e13ae15d 100644 (file)
--- 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