Merge pull request #12219 from keszybz/bootctl-check-entries

bootctl: check entries when showing them

TODO

@@ -686,9 +686,7 @@ Features:
   - honor timezone efi variables for default timezone selection (if there are any?)
   - change bootctl to be backed by systemd-bootd to control temporary and persistent default boot goal plus efi variables
 * bootctl
-  - verify that the files boot entries point to exist
   - recognize the case when not booted on EFI
-  - specify paths for boot entries
 
 * maybe do not install getty@tty1.service symlink in /etc but in /usr?
 

man/rules/meson.build

@@ -101,6 +101,7 @@ manpages = [
    'SD_ID128_MAKE',
    'SD_ID128_MAKE_STR',
    'SD_ID128_NULL',
+   'SD_ID128_UUID_FORMAT_STR',
    'sd_id128_equal',
    'sd_id128_is_null',
    'sd_id128_t'],

man/sd-id128.xml

@@ -24,6 +24,7 @@
     <refname>SD_ID128_NULL</refname>
     <refname>SD_ID128_CONST_STR</refname>
     <refname>SD_ID128_FORMAT_STR</refname>
+    <refname>SD_ID128_UUID_FORMAT_STR</refname>
     <refname>SD_ID128_FORMAT_VAL</refname>
     <refname>sd_id128_equal</refname>
     <refname>sd_id128_is_null</refname>
@@ -119,6 +120,11 @@ int main(int argc, char **argv) {
         return 0;
 }</programlisting>
 
+    <para><function>SD_ID128_UUID_FORMAT_STR()</function> is similar to
+    <function>SD_ID128_FORMAT_STR()</function> but includes separating hyphens to conform to the
+    "<ulink url="https://en.wikipedia.org/wiki/Universally_unique_identifier#Format">canonical representation</ulink>".
+    </para>
+
     <para>Use <function>sd_id128_equal()</function> to compare two 128-bit IDs:</para>
 
     <programlisting>int main(int argc, char *argv[]) {

src/boot/bootctl.c

@@ -220,7 +220,7 @@ static int status_binaries(const char *esp_path, sd_id128_t partition) {
 
         printf("          ESP: %s", esp_path);
         if (!sd_id128_is_null(partition))
-                printf(" (/dev/disk/by-partuuid/%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x)", SD_ID128_FORMAT_VAL(partition));
+                printf(" (/dev/disk/by-partuuid/" SD_ID128_UUID_FORMAT_STR ")", SD_ID128_FORMAT_VAL(partition));
         printf("\n");
 
         r = enumerate_binaries(esp_path, "EFI/systemd", NULL);
@@ -262,7 +262,8 @@ static int print_efi_option(uint16_t id, bool in_order) {
         printf("        Title: %s%s%s\n", ansi_highlight(), strna(title), ansi_normal());
         printf("           ID: 0x%04X\n", id);
         printf("       Status: %sactive%s\n", active ? "" : "in", in_order ? ", boot-order" : "");
-        printf("    Partition: /dev/disk/by-partuuid/%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x\n", SD_ID128_FORMAT_VAL(partition));
+        printf("    Partition: /dev/disk/by-partuuid/" SD_ID128_UUID_FORMAT_STR "\n",
+               SD_ID128_FORMAT_VAL(partition));
         printf("         File: %s%s\n", special_glyph(SPECIAL_GLYPH_TREE_RIGHT), path);
         printf("\n");
 
@@ -309,21 +310,57 @@ static int status_variables(void) {
         return 0;
 }
 
+static int boot_entry_file_check(const char *root, const char *p) {
+        _cleanup_free_ char *path;
+
+        path = path_join(root, p);
+        if (!path)
+                return log_oom();
+
+        if (access(path, F_OK) < 0)
+                return -errno;
+
+        return 0;
+}
+
+static void boot_entry_file_list(const char *field, const char *root, const char *p, int *ret_status) {
+        int status = boot_entry_file_check(root, p);
+
+        printf("%13s%s", strempty(field), field ? ":" : " ");
+        if (status < 0) {
+                errno = -status;
+                printf("%s%s%s (%m)\n", ansi_highlight_red(), p, ansi_normal());
+        } else
+                printf("%s\n", p);
+
+        if (*ret_status == 0 && status < 0)
+                *ret_status = status;
+}
+
 static int boot_entry_show(const BootEntry *e, bool show_as_default) {
+        int status = 0;
+
+        /* Returns 0 on success, negative on processing error, and positive if something is wrong with the
+           boot entry itself. */
+
         assert(e);
 
-        printf("        title: %s%s%s%s%s%s\n"
-               "         type: %s\n",
-               ansi_highlight(),
-               boot_entry_title(e),
-               ansi_normal(),
-               ansi_highlight_green(),
-               show_as_default ? " (default)" : "",
-               ansi_normal(),
-               boot_entry_type_to_string(e->type));
+        printf("        title: %s%s%s" "%s%s%s\n",
+               ansi_highlight(), boot_entry_title(e), ansi_normal(),
+               ansi_highlight_green(), show_as_default ? " (default)" : "", ansi_normal());
 
         if (e->id)
                 printf("           id: %s\n", e->id);
+        if (e->path) {
+                _cleanup_free_ char *link = NULL;
+
+                /* Let's urlify the link to make it easy to view in an editor, but only if it is a text
+                 * file. Unified images are binary ELFs, and EFI variables are not pure text either. */
+                if (e->type == BOOT_ENTRY_CONF)
+                        (void) terminal_urlify_path(e->path, NULL, &link);
+
+                printf("       source: %s\n", link ?: e->path);
+        }
         if (e->version)
                 printf("      version: %s\n", e->version);
         if (e->machine_id)
@@ -331,16 +368,14 @@ static int boot_entry_show(const BootEntry *e, bool show_as_default) {
         if (e->architecture)
                 printf(" architecture: %s\n", e->architecture);
         if (e->kernel)
-                printf("        linux: %s\n", e->kernel);
-        if (!strv_isempty(e->initrd)) {
-                _cleanup_free_ char *t;
+                boot_entry_file_list("linux", e->root, e->kernel, &status);
 
-                t = strv_join(e->initrd, " ");
-                if (!t)
-                        return log_oom();
-
-                printf("       initrd: %s\n", t);
-        }
+        char **s;
+        STRV_FOREACH(s, e->initrd)
+                boot_entry_file_list(s == e->initrd ? "initrd" : NULL,
+                                     e->root,
+                                     *s,
+                                     &status);
         if (!strv_isempty(e->options)) {
                 _cleanup_free_ char *t;
 
@@ -351,9 +386,9 @@ static int boot_entry_show(const BootEntry *e, bool show_as_default) {
                 printf("      options: %s\n", t);
         }
         if (e->device_tree)
-                printf("   devicetree: %s\n", e->device_tree);
+                boot_entry_file_list("devicetree", e->root, e->device_tree, &status);
 
-        return 0;
+        return -status;
 }
 
 static int status_entries(
@@ -380,7 +415,8 @@ static int status_entries(
         printf("Boot Loader Entries:\n"
                "        $BOOT: %s", dollar_boot_path);
         if (!sd_id128_is_null(dollar_boot_partition_uuid))
-                printf(" (/dev/disk/by-partuuid/%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x)", SD_ID128_FORMAT_VAL(dollar_boot_partition_uuid));
+                printf(" (/dev/disk/by-partuuid/" SD_ID128_UUID_FORMAT_STR ")",
+                       SD_ID128_FORMAT_VAL(dollar_boot_partition_uuid));
         printf("\n\n");
 
         r = boot_entries_load_config(esp_path, xbootldr_path, &config);
@@ -392,7 +428,11 @@ static int status_entries(
         else {
                 printf("Default Boot Loader Entry:\n");
 
-                boot_entry_show(config.entries + config.default_entry, false);
+                r = boot_entry_show(config.entries + config.default_entry, false);
+                if (r > 0)
+                        /* < 0 is already logged by the function itself, let's just emit an extra warning if
+                           the default entry is broken */
+                        printf("\nWARNING: default boot entry is broken\n");
         }
 
         return 0;
@@ -1150,7 +1190,7 @@ static int verb_status(int argc, char *argv[], void *userdata) {
                 if (stub)
                         printf("         Stub: %s\n", stub);
                 if (!sd_id128_is_null(loader_part_uuid))
-                        printf("          ESP: /dev/disk/by-partuuid/%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x\n",
+                        printf("          ESP: /dev/disk/by-partuuid/" SD_ID128_UUID_FORMAT_STR "\n",
                                SD_ID128_FORMAT_VAL(loader_part_uuid));
                 else
                         printf("          ESP: n/a\n");

src/libsystemd/sd-id128/id128-util.h

@@ -10,9 +10,6 @@
 
 char *id128_to_uuid_string(sd_id128_t id, char s[37]);
 
-/* Like SD_ID128_FORMAT_STR, but formats as UUID, not in plain format */
-#define ID128_UUID_FORMAT_STR "%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x"
-
 bool id128_is_valid(const char *s) _pure_;
 
 typedef enum Id128Format {

src/shared/bootspec.c

@@ -755,7 +755,7 @@ int boot_entries_augment_from_loader(BootConfig *config, bool only_auto) {
         n_allocated = config->n_entries;
 
         STRV_FOREACH(i, found_by_loader) {
-                _cleanup_free_ char *c = NULL, *t = NULL;
+                _cleanup_free_ char *c = NULL, *t = NULL, *p = NULL;
                 char **a, **b;
 
                 if (boot_config_has_entry(config, *i))
@@ -776,6 +776,10 @@ int boot_entries_augment_from_loader(BootConfig *config, bool only_auto) {
                                 break;
                         }
 
+                p = efi_variable_path(EFI_VENDOR_LOADER, "LoaderEntries");
+                if (!p)
+                        return log_oom();
+
                 if (!GREEDY_REALLOC0(config->entries, n_allocated, config->n_entries + 1))
                         return log_oom();
 
@@ -783,6 +787,7 @@ int boot_entries_augment_from_loader(BootConfig *config, bool only_auto) {
                         .type = BOOT_ENTRY_LOADER,
                         .id = TAKE_PTR(c),
                         .title = TAKE_PTR(t),
+                        .path = TAKE_PTR(p),
                 };
         }
 
@@ -1418,11 +1423,3 @@ found:
 
         return 0;
 }
-
-static const char* const boot_entry_type_table[_BOOT_ENTRY_MAX] = {
-        [BOOT_ENTRY_CONF] = "conf",
-        [BOOT_ENTRY_UNIFIED] = "unified",
-        [BOOT_ENTRY_LOADER] = "loader",
-};
-
-DEFINE_STRING_TABLE_LOOKUP(boot_entry_type, BootEntryType);

src/shared/bootspec.h

@@ -79,6 +79,3 @@ static inline const char* boot_entry_title(const BootEntry *entry) {
 
 int find_esp_and_warn(const char *path, bool unprivileged_mode, char **ret_path, uint32_t *ret_part, uint64_t *ret_pstart, uint64_t *ret_psize, sd_id128_t *ret_uuid);
 int find_xbootldr_and_warn(const char *path, bool unprivileged_mode, char **ret_path,sd_id128_t *ret_uuid);
-
-const char* boot_entry_type_to_string(BootEntryType t) _const_;
-BootEntryType boot_entry_type_from_string(const char *s) _pure_;

src/shared/efivars.c

@@ -193,6 +193,17 @@ int efi_set_reboot_to_firmware(bool value) {
         return 0;
 }
 
+char* efi_variable_path(sd_id128_t vendor, const char *name) {
+        char *p;
+
+        if (asprintf(&p,
+                     "/sys/firmware/efi/efivars/%s-" SD_ID128_UUID_FORMAT_STR,
+                     name, SD_ID128_FORMAT_VAL(vendor)) < 0)
+                return NULL;
+
+        return p;
+}
+
 int efi_get_variable(
                 sd_id128_t vendor,
                 const char *name,
@@ -211,9 +222,8 @@ int efi_get_variable(
         assert(value);
         assert(size);
 
-        if (asprintf(&p,
-                     "/sys/firmware/efi/efivars/%s-%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x",
-                     name, SD_ID128_FORMAT_VAL(vendor)) < 0)
+        p = efi_variable_path(vendor, name);
+        if (!p)
                 return -ENOMEM;
 
         fd = open(p, O_RDONLY|O_NOCTTY|O_CLOEXEC);
@@ -293,9 +303,8 @@ int efi_set_variable(
         assert(name);
         assert(value || size == 0);
 
-        if (asprintf(&p,
-                     "/sys/firmware/efi/efivars/%s-%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x",
-                     name, SD_ID128_FORMAT_VAL(vendor)) < 0)
+        p = efi_variable_path(vendor, name);
+        if (!p)
                 return -ENOMEM;
 
         /* Newer efivarfs protects variables that are not in a whitelist with FS_IMMUTABLE_FL by default, to protect
@@ -772,7 +781,7 @@ int efi_loader_get_device_part_uuid(sd_id128_t *u) {
         if (r < 0)
                 return r;
 
-        if (sscanf(p, "%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x",
+        if (sscanf(p, SD_ID128_UUID_FORMAT_STR,
                    &parsed[0], &parsed[1], &parsed[2], &parsed[3],
                    &parsed[4], &parsed[5], &parsed[6], &parsed[7],
                    &parsed[8], &parsed[9], &parsed[10], &parsed[11],

src/shared/efivars.h

@@ -33,6 +33,7 @@ int efi_reboot_to_firmware_supported(void);
 int efi_get_reboot_to_firmware(void);
 int efi_set_reboot_to_firmware(bool value);
 
+char* efi_variable_path(sd_id128_t vendor, const char *name);
 int efi_get_variable(sd_id128_t vendor, const char *name, uint32_t *attribute, void **value, size_t *size);
 int efi_get_variable_string(sd_id128_t vendor, const char *name, char **p);
 int efi_set_variable(sd_id128_t vendor, const char *name, const void *value, size_t size);

src/shared/id128-print.c

@@ -33,7 +33,7 @@ int id128_pretty_print(sd_id128_t id, bool pretty) {
         printf("As string:\n"
                "%s" SD_ID128_FORMAT_STR "%s\n\n"
                "As UUID:\n"
-               "%s%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x%s\n\n"
+               "%s" SD_ID128_UUID_FORMAT_STR "%s\n\n"
                "As %s macro:\n"
                "%s#define MESSAGE_XYZ SD_ID128_MAKE(",
                on, SD_ID128_FORMAT_VAL(id), off,

src/systemd/sd-id128.h

@@ -63,6 +63,9 @@ int sd_id128_get_boot_app_specific(sd_id128_t app_id, sd_id128_t *ret);
 #define SD_ID128_FORMAT_STR "%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x"
 #define SD_ID128_FORMAT_VAL(x) (x).bytes[0], (x).bytes[1], (x).bytes[2], (x).bytes[3], (x).bytes[4], (x).bytes[5], (x).bytes[6], (x).bytes[7], (x).bytes[8], (x).bytes[9], (x).bytes[10], (x).bytes[11], (x).bytes[12], (x).bytes[13], (x).bytes[14], (x).bytes[15]
 
+/* Like SD_ID128_FORMAT_STR, but formats as UUID, not in plain format */
+#define SD_ID128_UUID_FORMAT_STR "%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x"
+
 #define SD_ID128_CONST_STR(x)                                           \
         ((const char[SD_ID128_STRING_MAX]) {                            \
                 ((x).bytes[0] >> 4) >= 10 ? 'a' + ((x).bytes[0] >> 4) - 10 : '0' + ((x).bytes[0] >> 4), \

src/test/test-id128.c

@@ -52,7 +52,7 @@ int main(int argc, char *argv[]) {
         assert_se(streq(q, UUID_WALDI));
 
         b = mfree(b);
-        assert_se(asprintf(&b, ID128_UUID_FORMAT_STR, SD_ID128_FORMAT_VAL(ID128_WALDI)) == 36);
+        assert_se(asprintf(&b, SD_ID128_UUID_FORMAT_STR, SD_ID128_FORMAT_VAL(ID128_WALDI)) == 36);
         printf("waldi4: %s\n", b);
         assert_se(streq(q, b));