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

[Simon Glass]

Hi Heinrich,

On Sun, 28 Jun 2026 at 07:06, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> On 6/25/26 10:54, Simon Glass wrote:
> > 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.
>
> Neither efi_freestanding.c nor efi_app_common.c have one.
> Maybe in future we could add one.
>

OK...we can likely progressively move common things to lib/efi

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

OK

Regards,
Simon