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

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