[PATCH v2 1/3] eficonfig: refactor eficonfig_process_common function

[Masahisa Kojima]

Hi Ilias,

On Tue, 27 Dec 2022 at 23:41, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Kojima-san
>
>
> Overall I think the cleanup is nice and easier to maintain in the long
> run.
>
> On Sat, Dec 24, 2022 at 07:57:42AM +0900, Masahisa Kojima wrote:
> > Current change boot order implementation does not call
> > eficonfig_process_common() and call own menu functions
> > for display_statusline, item_data_print and item_choice.
> > Change boot order functionality should call
> > eficonfig_process_common() to improve maintenanceability.
> >
> > This commit is a preparation to remove the change boot
> > order specific implementation. The menu functions
> > (display_statusline, item_data_print and item_choice) are
> > added as argument of eficonfig_process_common().
> > The menu description string displayed at the bottom of
> > the menu is also added as argument.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima at linaro.org>
> > ---
> > Changes in v2:
> > - add const qualifier to eficonfig_menu_desc, change it to pointer
> >
> >  cmd/eficonfig.c       | 69 +++++++++++++++++++++++++++++++++----------
> >  cmd/eficonfig_sbkey.c | 18 +++++++++--
> >  include/efi_config.h  | 13 +++++++-
> >  3 files changed, 81 insertions(+), 19 deletions(-)
> >
> > diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> > index ce7175a566..2fc486dac2 100644
> > --- a/cmd/eficonfig.c
> > +++ b/cmd/eficonfig.c
> > @@ -21,6 +21,8 @@
> >  #include <linux/delay.h>
> >
> >  static struct efi_simple_text_input_protocol *cin;
> > +const char *eficonfig_menu_desc =
> > +     "  Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit";
> >
> >  #define EFICONFIG_DESCRIPTION_MAX 32
> >  #define EFICONFIG_OPTIONAL_DATA_MAX 64
> > @@ -133,7 +135,7 @@ void eficonfig_print_msg(char *msg)
> >   *
> >   * @data:    pointer to the data associated with each menu entry
> >   */
> > -static void eficonfig_print_entry(void *data)
> > +void eficonfig_print_entry(void *data)
> >  {
> >       struct eficonfig_entry *entry = data;
> >       int reverse = (entry->efi_menu->active == entry->num);
> > @@ -160,7 +162,7 @@ static void eficonfig_print_entry(void *data)
> >   *
> >   * @m:       pointer to the menu structure
> >   */
> > -static void eficonfig_display_statusline(struct menu *m)
> > +void eficonfig_display_statusline(struct menu *m)
> >  {
> >       struct eficonfig_entry *entry;
> >
> > @@ -170,10 +172,11 @@ static void eficonfig_display_statusline(struct menu *m)
> >       printf(ANSI_CURSOR_POSITION
> >             "\n%s\n"
> >              ANSI_CURSOR_POSITION ANSI_CLEAR_LINE ANSI_CURSOR_POSITION
> > -            "  Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit"
> > +            "%s"
> >              ANSI_CLEAR_LINE_TO_END ANSI_CURSOR_POSITION ANSI_CLEAR_LINE,
> >              1, 1, entry->efi_menu->menu_header, entry->efi_menu->count + 5, 1,
> > -            entry->efi_menu->count + 6, 1, entry->efi_menu->count + 7, 1);
> > +            entry->efi_menu->count + 6, 1, entry->efi_menu->menu_desc,
> > +            entry->efi_menu->count + 7, 1);
> >  }
> >
> >  /**
> > @@ -182,7 +185,7 @@ static void eficonfig_display_statusline(struct menu *m)
> >   * @data:    pointer to the efimenu structure
> >   * Return:   key string to identify the selected entry
> >   */
> > -static char *eficonfig_choice_entry(void *data)
> > +char *eficonfig_choice_entry(void *data)
> >  {
> >       int esc = 0;
> >       struct list_head *pos, *n;
> > @@ -358,9 +361,17 @@ out:
> >   *
> >   * @efi_menu:                pointer to the efimenu structure
> >   * @menu_header:     pointer to the menu header string
> > + * @menu_desc:               pointer to the menu description
> > + * @display_statusline:      function pointer to draw statusline
> > + * @item_data_print: function pointer to draw the menu item
> > + * @item_choice:     function pointer to handle the key press
> >   * Return:           status code
> >   */
> > -efi_status_t eficonfig_process_common(struct efimenu *efi_menu, char *menu_header)
> > +efi_status_t eficonfig_process_common(struct efimenu *efi_menu,
> > +                                   char *menu_header, const char *menu_desc,
> > +                                   void (*display_statusline)(struct menu *),
> > +                                   void (*item_data_print)(void *),
> > +                                   char *(*item_choice)(void *))
> >  {
> >       struct menu *menu;
> >       void *choice = NULL;
> > @@ -379,10 +390,11 @@ efi_status_t eficonfig_process_common(struct efimenu *efi_menu, char *menu_heade
> >               if (!efi_menu->menu_header)
> >                       return EFI_OUT_OF_RESOURCES;
> >       }
> > +     if (menu_desc)
> > +             efi_menu->menu_desc = menu_desc;
> >
> > -     menu = menu_create(NULL, 0, 1, eficonfig_display_statusline,
> > -                        eficonfig_print_entry, eficonfig_choice_entry,
> > -                        efi_menu);
> > +     menu = menu_create(NULL, 0, 1, display_statusline, item_data_print,
> > +                        item_choice, efi_menu);
>
> menu_create doesn't check any pointers for item_data_print, item_choice, efi_menu
> Should we check any of these here?

This is as I intended.
I think we don't need to check these pointers, U-Boot menu has default behavior
when these pointers are NULL.
For example, if item_data_print is NULL, U-Boot menu prints the "key" string
of each menu entry.

Thanks,
Masahisa Kojima

>
> >       if (!menu)
> >               return EFI_INVALID_PARAMETER;
>
> [...]
>
> Regards
> /Ilias