[PATCH v2 2/3] efi_loader: fix dtbdump output color and format

[Simon Glass]

Hi Adriano,

On Thu, 8 May 2025 at 20:29, Adriano Cordova <adrianox at gmail.com> wrote:
>
> Imitate in dtbdump what initrddump does for color,
> newlines and input handling. The output parsing in
> the CI is strict and with the current output the CI
> is not recongnizing the prompt '=>'.

It would be useful to mention 'ANSI' in the subject, since I'm
assuming it is ANSI colour codes which are being written to the output
here?

>
> Signed-off-by: Adriano Cordova <adriano.cordova at canonical.com>
> Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> ---
>
> (no changes since v1)
>
>  lib/efi_loader/dtbdump.c | 86 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 74 insertions(+), 12 deletions(-)
>
> diff --git a/lib/efi_loader/dtbdump.c b/lib/efi_loader/dtbdump.c
> index a3d59a2fd3b..1e72404ecc1 100644
> --- a/lib/efi_loader/dtbdump.c
> +++ b/lib/efi_loader/dtbdump.c
> @@ -29,6 +29,18 @@ static struct efi_system_table *systable;
>  static const efi_guid_t efi_dt_fixup_protocol_guid = EFI_DT_FIXUP_PROTOCOL_GUID;
>  static const efi_guid_t efi_file_info_guid = EFI_FILE_INFO_GUID;
>  static const efi_guid_t efi_system_partition_guid = PARTITION_SYSTEM_GUID;
> +static bool nocolor;
> +
> +/**
> + * color() - set foreground color
> + *
> + * @color:     foreground color
> + */
> +static void color(u8 color)
> +{
> +       if (!nocolor)
> +               cout->set_attribute(cout, color | EFI_BACKGROUND_BLACK);
> +}

Rather than duplicating this code, could it go in a shared file? Could
be a follow-up patch.

>
>  /**
>   * print() - print string
> @@ -87,6 +99,17 @@ static void printx(unsigned char val)
>         print_hex_digit(val & 0xf);
>  }
>
> +/**
> + * cls() - clear screen
> + */
> +static void cls(void)
> +{
> +       if (nocolor)
> +               print(u"\r\n");
> +       else
> +               cout->clear_screen(cout);
> +}
> +
>  /**
>   * error() - print error string
>   *
> @@ -94,9 +117,17 @@ static void printx(unsigned char val)
>   */
>  static void error(u16 *string)
>  {
> -       cout->set_attribute(cout, EFI_LIGHTRED | EFI_BACKGROUND_BLACK);
> +       color(EFI_LIGHTRED);
>         print(string);
> -       cout->set_attribute(cout, EFI_LIGHTBLUE | EFI_BACKGROUND_BLACK);
> +       color(EFI_LIGHTBLUE);
> +}
> +
> +/**
> + * efi_drain_input() - drain console input
> + */
> +static void efi_drain_input(void)
> +{
> +       cin->reset(cin, true);
>  }
>
>  /**
> @@ -116,8 +147,6 @@ static efi_status_t efi_input_yn(void)
>         efi_uintn_t index;
>         efi_status_t ret;
>
> -       /* Drain the console input */
> -       ret = cin->reset(cin, true);
>         for (;;) {
>                 ret = bs->wait_for_event(1, &cin->wait_for_key, &index);
>                 if (ret != EFI_SUCCESS)
> @@ -158,8 +187,6 @@ static efi_status_t efi_input(u16 *buffer, efi_uintn_t buffer_size)
>         u16 outbuf[2] = u" ";
>         efi_status_t ret;
>
> -       /* Drain the console input */
> -       ret = cin->reset(cin, true);
>         *buffer = 0;
>         for (;;) {
>                 ret = bs->wait_for_event(1, &cin->wait_for_key, &index);
> @@ -262,6 +289,9 @@ static u16 *skip_whitespace(u16 *pos)
>   */

Please update the function comment to indicate its behaviour when NULL is passed

>  static bool starts_with(u16 *string, u16 *keyword)
>  {
> +       if (!string || !keyword)
> +               return false;
> +

[..]

Regards,
Simon