Re: [PATCH v2 09/11] efi_loader: move efi_main and command loop to efi_app_common

[Heinrich Schuchardt]

On 6/25/26 10:54, Simon Glass wrote:
Hi Heinrich,
On 2026-06-21T08:19:04, Heinrich Schuchardt
<heinrich.schucha...@canonical.com> wrote:
efi_loader: move efi_main and command loop to efi_app_common

Add the remaining shared code to efi_app_common to complete the
refactoring:

* efi_main() - common entry point; calls efi_app_init(), prints
   app_banner, calls command_loop(), then clears the screen on exit.
* struct efi_app_cmd - table entry holding the keyword, help text,
   and a handler of type void (*fn)(u16 *args).
* command_loop() - iterates app_cmds[], matches keywords with
   starts_with(), and falls back to efi_app_help() on no match.
* efi_app_help() - prints each table entry's help line plus the
   built-in 'exit' line.

Each dump app is updated accordingly:
* efi_main is removed; command_loop becomes non-static.
* app_banner[] is defined at file scope with the application's
   title string.
* Per-app duplicate state (bs, handle, systable) is removed; apps
   now use the common module's state.
[...]

lib/efi_loader/dbginfodump.c    |  95 +++++-------------------------
  lib/efi_loader/dtbdump.c        | 109 +++++++---------------------------
  lib/efi_loader/efi_app_common.c | 111 ++++++++++++++++++++++++++++++++++-
  lib/efi_loader/efi_app_common.h |  31 +++++-----
  lib/efi_loader/initrddump.c     |  91 +++++------------------------
  lib/efi_loader/smbiosdump.c     | 126 ++++++++--------------------------------
  6 files changed, 205 insertions(+), 358 deletions(-)

diff --git a/lib/efi_loader/efi_app_common.c b/lib/efi_loader/efi_app_common.c
@@ -304,3 +317,99 @@ efi_status_t save_file(u16 *filename, void *buf, 
efi_uintn_t size)
+extern u16 app_banner[];
+extern struct efi_app_cmd app_cmds[];

Please move these extern declarations into efi_app_common.h next to
struct efi_app_cmd - inline externs in the .c file hide the contract
that every consumer must define both symbols; putting them in the
header makes the interface explicit and lets the compiler catch type
mismatches.

diff --git a/lib/efi_loader/dbginfodump.c b/lib/efi_loader/dbginfodump.c
@@ -118,20 +99,27 @@ static void print_info(struct efi_loaded_image *info)
-static efi_status_t do_dump(void)
+static void do_dump(u16 * __maybe_unused args)
  {
       struct dbg_info_header *dbg;
+
+     if (!device_path_to_text)
+             bs->locate_protocol(&guid_device_path_to_text_protocol, NULL,
+                                  (void **)&device_path_to_text);
+
       u32 count;

Declarations should be at the top of the block; u32 count lands after
the locate_protocol() call. Also, the locate_protocol() return value
is now ignored, so a permanent failure silently leaves
device_path_to_text NULL with no diagnostic; the previous code printed
'No device path to text protocol' on error. Please restore that or at
least check the return.

diff --git a/lib/efi_loader/dbginfodump.c b/lib/efi_loader/dbginfodump.c
@@ -164,60 +152,9 @@ static efi_status_t do_dump(void)
               print(u'\r\n');
       }

-     return EFI_SUCCESS;
  }

Stray blank line before the closing brace.

diff --git a/lib/efi_loader/smbiosdump.c b/lib/efi_loader/smbiosdump.c
@@ -156,16 +143,13 @@ static efi_status_t do_save(u16 *filename)
-     ret = do_check();
-     if (ret != EFI_SUCCESS)
-             return ret;
-
       smbios3_anchor = get_config_table(&smbios3_guid);

Behaviour change not mentioned in the commit message: previously
'save' refused to write a malformed table because it ran do_check()
first; now it happily writes whatever is there. If intentional, please
call it out; otherwise restore the check.

diff --git a/lib/efi_loader/efi_app_common.c b/lib/efi_loader/efi_app_common.c
@@ -304,3 +317,99 @@ efi_status_t save_file(u16 *filename, void *buf, 
efi_uintn_t size)
+                     for (cmd = app_cmds; cmd->fn; ++cmd) {
+                             if (starts_with(pos, (u16 *)cmd->cmd)) {
+                                     efi_uintn_t len;
+
+                                     for (len = 0; cmd->cmd[len]; ++len)
+                                             ;

Isn't there a u16 strlen-style helper that exists here? Also,
cmd->cmd[len - 1] below assumes len > 0; an empty keyword would
underflow. Worth an assertion or comment documenting the invariant.

Neither efi_freestanding.c nor efi_app_common.c have one.
Maybe in future we could add one.

Let's check len below this line.


diff --git a/lib/efi_loader/dbginfodump.c b/lib/efi_loader/dbginfodump.c
@@ -0,0 +0,0 @@
+struct efi_app_cmd app_cmds[] = {
+     { u'dump', u"dump - print debug info table\r\n", do_dump },
+     { }
+};

The original help text column-aligned the dash ('dump       - print
...') across all apps. With the new table the dash sits right after
the keyword, so 'load' and 'save <initrd>' no longer line up in
initrddump, and likewise for dtbdump/smbiosdump. Easy to restore by
padding the help strings.

Maintaining such an alignment in the app definition does not seem right.
If we want to restore the alignment, we can do that in a future patch 
for the help() function.

Best Heinrich