[PATCH v2 05/11] efi_loader: add efi_app_common skeleton

[Simon Glass]

Hi Heinrich,

On 2026-06-21T08:19:04, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
> efi_loader: add efi_app_common skeleton
>
> The EFI dump applications (acpidump, dbginfodump, dtbdump, initrddump,
> smbiosdump) each contain duplicate copies of the same output and
> input helpers, and each has its own command loop. This patch series
> deduplicates that code by introducing a shared module.
>
> Create lib/efi_loader/efi_app_common.c and efi_app_common.h to hold
> utilities shared across the EFI dump applications.
>
> This first patch adds the module skeleton:
>
> * State variables (app_handle, cout, bs, nocolor, systable) owned by
>   the common module.
> * efi_app_init() to initialize them from sys_table and image_handle,
>   including detecting the 'nocolor' load option via get_load_options()
>   and starts_with().
> * Output helpers: print(), printx(), color(), cls(), error().
> * starts_with() - test whether a UTF-16 string begins with a keyword.
> * get_load_options() - retrieve the load options of the loaded image.
> [...]
>
> lib/efi_loader/Makefile         |  13 +++-
>  lib/efi_loader/dbginfodump.c    |  43 +------------
>  lib/efi_loader/dtbdump.c        | 133 ++--------------------------------------
>  lib/efi_loader/efi_app_common.c | 106 ++++++++++++++++++++++++++++++++
>  lib/efi_loader/efi_app_common.h |  67 ++++++++++++++++++++
>  lib/efi_loader/initrddump.c     | 126 +------------------------------------
>  lib/efi_loader/smbiosdump.c     | 100 +-----------------------------
>  7 files changed, 196 insertions(+), 392 deletions(-)

Great to see this!

> diff --git a/lib/efi_loader/efi_app_common.c b/lib/efi_loader/efi_app_common.c
> new file mode 100644
> index 00000000000..e9e9334acc7
> --- /dev/null
> +++ b/lib/efi_loader/efi_app_common.c
> @@ -0,0 +1,106 @@
> +static efi_handle_t app_handle;
> +static struct efi_simple_text_output_protocol *cout;
> +struct efi_boot_services *bs;
> +static bool nocolor;
> +static struct efi_system_table *systable;

Is it intended to have bs as a global? If so, please add a comment and
put it out on its own.

> diff --git a/lib/efi_loader/dtbdump.c b/lib/efi_loader/dtbdump.c
> @@ -639,7 +548,7 @@ static void print_property(const unsigned char *val, u32 len)
>               for (int i = 0; i < len; ++i) {
>                       if (i)
>                               print(u" ");
> -                     printx(val[i]);
> +                     printx(val[i], 2);

dtbdump's local printx() took an unsigned char and always printed two
digits; the common helper takes (u64, u32 prec) and the call sites are
silently re-targeted. Behaviour is preserved, but please call the
signature change out in the commit message so a future bisect reader
can spot it.

> diff --git a/lib/efi_loader/dbginfodump.c b/lib/efi_loader/dbginfodump.c
> @@ -310,9 +272,10 @@ efi_status_t EFIAPI efi_main(efi_handle_t image_handle,
>  {
>       efi_status_t ret;
>
> +     efi_app_init(systab, image_handle);
> +
>       handle = image_handle;
>       systable = systab;
> -     cerr = systable->std_err;
>       cout = systable->con_out;
>       cin = systable->con_in;
>       bs = systable->boottime;

After efi_app_init() runs, every app still re-derives
cout/cin/bs/handle into its own static variabless, it is worth noting
in the commit message that updating the tools use use these comes
later...also that dbginfodump previously had no nocolor handling at
all, so its error() now respects nocolor.

Regards,
Simon