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

[Simon Glass]

Hi Heinrich,

On 2026-06-21T08:19:04, Heinrich Schuchardt
<heinrich.schuchardt at 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.

> 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.

Regards,
Simon