[PATCH 2/3] eficonfig: refactor change boot order implementation
Masahisa Kojima
masahisa.kojima at linaro.org
Fri Dec 23 01:43:57 CET 2022
On Thu, 22 Dec 2022 at 20:54, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> On Wed, Dec 21, 2022 at 10:50:37PM +0900, Masahisa Kojima wrote:
> > This commit removes the change boot order specific
> > menu implementation. The change boot order implementation
> > calls eficonfig_process_common() same as other menus.
> >
> > The change boot order menu requires own item_data_print
> > and item_choice implementation, but display_statusline
> > function can be a same function as other menus.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima at linaro.org>
> > ---
> > cmd/eficonfig.c | 236 +++++++++++++++++++++++++++++-------------------
> > 1 file changed, 143 insertions(+), 93 deletions(-)
> >
> > diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> > index 39ee766a7b..c2c6c01c3b 100644
> > --- a/cmd/eficonfig.c
> > +++ b/cmd/eficonfig.c
> > @@ -24,6 +24,11 @@ static struct efi_simple_text_input_protocol *cin;
> > char eficonfig_menu_desc[] =
> > " Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit";
> >
> > +static char eficonfig_change_boot_order_desc[] =
> > + " Press UP/DOWN to move, +/- to change orde\n"
> > + " Press SPACE to activate or deactivate the entry\n"
> > + " Select [Save] to complete, ESC/CTRL+C to quit";
> > +
>
> static const
OK.
>
> > #define EFICONFIG_DESCRIPTION_MAX 32
> > #define EFICONFIG_OPTIONAL_DATA_MAX 64
> >
> > @@ -105,6 +110,17 @@ struct eficonfig_boot_order_data {
> > bool active;
> > };
> >
> > +/**
> > + * struct eficonfig_save_boot_order_data - structure to be used to change boot order
> > + *
> > + * @efi_menu: pointer to efimenu structure
> > + * @selected: flag to indicate user selects "Save" entry
> > + */
> > +struct eficonfig_save_boot_order_data {
> > + struct efimenu *efi_menu;
> > + bool selected;
> > +};
> > +
> > /**
> > * eficonfig_print_msg() - print message
> > *
> > @@ -173,10 +189,9 @@ void eficonfig_display_statusline(struct menu *m)
> > "\n%s\n"
> > ANSI_CURSOR_POSITION ANSI_CLEAR_LINE ANSI_CURSOR_POSITION
> > "%s"
> > - ANSI_CLEAR_LINE_TO_END ANSI_CURSOR_POSITION ANSI_CLEAR_LINE,
> > + ANSI_CLEAR_LINE_TO_END,
> > 1, 1, entry->efi_menu->menu_header, entry->efi_menu->count + 5, 1,
> > - entry->efi_menu->count + 6, 1, entry->efi_menu->menu_desc,
> > - entry->efi_menu->count + 7, 1);
> > + entry->efi_menu->count + 6, 1, entry->efi_menu->menu_desc);
> > }
> >
> > /**
> > @@ -1841,63 +1856,44 @@ out:
> > }
> >
> > /**
> > - * eficonfig_display_change_boot_order() - display the BootOrder list
> > + * eficonfig_print_change_boot_order_entry() - print the boot option entry
> > *
> > - * @efi_menu: pointer to the efimenu structure
> > - * Return: status code
> > + * @data: pointer to the data associated with each menu entry
> > */
> > -static void eficonfig_display_change_boot_order(struct efimenu *efi_menu)
> > +static void eficonfig_print_change_boot_order_entry(void *data)
> > {
> > - bool reverse;
> > - struct list_head *pos, *n;
> > - struct eficonfig_entry *entry;
> > -
> > - printf(ANSI_CLEAR_CONSOLE ANSI_CURSOR_POSITION
> > - "\n ** Change Boot Order **\n"
> > - ANSI_CURSOR_POSITION
> > - " Press UP/DOWN to move, +/- to change order"
> > - ANSI_CURSOR_POSITION
> > - " Press SPACE to activate or deactivate the entry"
> > - ANSI_CURSOR_POSITION
> > - " Select [Save] to complete, ESC/CTRL+C to quit"
> > - ANSI_CURSOR_POSITION ANSI_CLEAR_LINE,
> > - 1, 1, efi_menu->count + 5, 1, efi_menu->count + 6, 1,
> > - efi_menu->count + 7, 1, efi_menu->count + 8, 1);
> > -
> > - /* draw boot option list */
> > - list_for_each_safe(pos, n, &efi_menu->list) {
>
> So this prints one option every time now instead of all the entries?
Yes, it is the same behavior as other menus and U-Boot menu framework expects.
>
> > - entry = list_entry(pos, struct eficonfig_entry, list);
> > - reverse = (entry->num == efi_menu->active);
> > + struct eficonfig_entry *entry = data;
> > + int reverse = (entry->efi_menu->active == entry->num);
> >
> > - printf(ANSI_CURSOR_POSITION, entry->num + 4, 7);
> > + printf(ANSI_CURSOR_POSITION, entry->num + 4, 7);
> >
> > - if (reverse)
> > - puts(ANSI_COLOR_REVERSE);
> > + if (reverse)
> > + puts(ANSI_COLOR_REVERSE);
> >
> > - if (entry->num < efi_menu->count - 2) {
> > - if (((struct eficonfig_boot_order_data *)entry->data)->active)
> > - printf("[*] ");
> > - else
> > - printf("[ ] ");
> > - }
> > + if (entry->num < entry->efi_menu->count - 2) {
> > + if (((struct eficonfig_boot_order_data *)entry->data)->active)
> > + printf("[*] ");
> > + else
> > + printf("[ ] ");
> > + }
> >
> > - printf("%s", entry->title);
> > + printf("%s", entry->title);
> >
> > - if (reverse)
> > - puts(ANSI_COLOR_RESET);
> > - }
> > + if (reverse)
> > + puts(ANSI_COLOR_RESET);
> > }
> >
> > /**
> > - * eficonfig_choice_change_boot_order() - handle the BootOrder update
> > + * eficonfig_choice_change_boot_order() - user key input handler
> > *
> > - * @efi_menu: pointer to the efimenu structure
> > - * Return: status code
> > + * @data: pointer to the menu entry
> > + * Return: key string to identify the selected entry
> > */
> > -static efi_status_t eficonfig_choice_change_boot_order(struct efimenu *efi_menu)
> > +char *eficonfig_choice_change_boot_order(void *data)
> > {
> > int esc = 0;
> > struct list_head *pos, *n;
> > + struct efimenu *efi_menu = data;
> > enum bootmenu_key key = KEY_NONE;
> > struct eficonfig_entry *entry, *tmp;
> >
> > @@ -1922,7 +1918,7 @@ static efi_status_t eficonfig_choice_change_boot_order(struct efimenu *efi_menu)
> > case KEY_UP:
> > if (efi_menu->active > 0)
> > --efi_menu->active;
> > - return EFI_NOT_READY;
> > + return NULL;
> > case KEY_MINUS:
> > if (efi_menu->active < efi_menu->count - 3) {
> > list_for_each_safe(pos, n, &efi_menu->list) {
> > @@ -1938,19 +1934,28 @@ static efi_status_t eficonfig_choice_change_boot_order(struct efimenu *efi_menu)
> >
> > ++efi_menu->active;
> > }
> > - return EFI_NOT_READY;
> > + return NULL;
> > case KEY_DOWN:
> > if (efi_menu->active < efi_menu->count - 1)
> > ++efi_menu->active;
> > - return EFI_NOT_READY;
> > + return NULL;
> > case KEY_SELECT:
> > /* "Save" */
> > - if (efi_menu->active == efi_menu->count - 2)
> > - return EFI_SUCCESS;
> > -
> > + if (efi_menu->active == efi_menu->count - 2) {
> > + list_for_each_prev_safe(pos, n, &efi_menu->list) {
> > + entry = list_entry(pos, struct eficonfig_entry, list);
> > + if (entry->num == efi_menu->active)
> > + break;
> > + }
> > + return entry->key;
> > + }
> > /* "Quit" */
> > - if (efi_menu->active == efi_menu->count - 1)
> > - return EFI_ABORTED;
> > + if (efi_menu->active == efi_menu->count - 1) {
> > + entry = list_last_entry(&efi_menu->list,
> > + struct eficonfig_entry,
> > + list);
> > + return entry->key;
> > + }
> >
> > break;
>
> I think I've asked about this in the past, but don't we have to return
> something here?
Reaching here means the user key press is not valid and no need to
re-draw the menu,
it is the same as 'default' case.
I will add a comment for this case.
>
> > case KEY_SPACE:
> > @@ -1961,13 +1966,15 @@ static efi_status_t eficonfig_choice_change_boot_order(struct efimenu *efi_menu)
> > struct eficonfig_boot_order_data *data = entry->data;
> >
> > data->active = !data->active;
> > - return EFI_NOT_READY;
> > + return NULL;
> > }
> > }
> > }
> > break;
I also add the same comment for this case.
> > case KEY_QUIT:
> > - return EFI_ABORTED;
> > + entry = list_last_entry(&efi_menu->list,
> > + struct eficonfig_entry, list);
> > + return entry->key;
> > default:
> > /* Pressed key is not valid, no need to regenerate the menu */
> > break;
> > @@ -2034,6 +2041,62 @@ out:
> > return ret;
> > }
> >
> > +/**
> > + * eficonfig_process_save_boot_order() - callback function for "Save" entry
> > + *
> > + * @data: pointer to the data
> > + * Return: status code
> > + */
> > +static efi_status_t eficonfig_process_save_boot_order(void *data)
> > +{
> > + u32 count = 0;
> > + efi_status_t ret;
> > + efi_uintn_t size;
> > + struct list_head *pos, *n;
> > + u16 *new_bootorder;
> > + struct efimenu *efi_menu;
> > + struct eficonfig_entry *entry;
> > + struct eficonfig_save_boot_order_data *save_data = data;
> > +
> > + efi_menu = save_data->efi_menu;
> > +
> > + if (!(efi_menu->count - 2)) /* no user defined boot option */
> > + return EFI_SUCCESS;
> > +
> > + new_bootorder = calloc(1, (efi_menu->count - 2) * sizeof(u16));
> > + if (!new_bootorder) {
> > + ret = EFI_OUT_OF_RESOURCES;
> > + goto out;
> > + }
> > +
> > + /* create new BootOrder */
> > + count = 0;
> > + list_for_each_safe(pos, n, &efi_menu->list) {
> > + struct eficonfig_boot_order_data *data;
> > +
> > + entry = list_entry(pos, struct eficonfig_entry, list);
> > + /* exit the loop when iteration reaches "Save" */
> > + if (!strncmp(entry->title, "Save", strlen("Save")))
> > + break;
> > +
> > + data = entry->data;
> > + if (data->active)
> > + new_bootorder[count++] = data->boot_index;
>
> The new_bootorder variable is allocated with the size of 'efi_menu->count - 2'.
> Should we add a check for count < efi_menu->count - 2 or are we always
> safe?
At the beginning of this function, there is a check.
if (!(efi_menu->count - 2)) /* no user defined boot option */
return EFI_SUCCESS;
The change boot order menu always has at least "Save" and "Quit" menu entries,
count < efi_menu->count - 2 is always safe here.
I will add a comment on this.
Thanks,
Masahisa Kojima
>
> > + }
> > +
> > + size = count * sizeof(u16);
> > + ret = efi_set_variable_int(u"BootOrder", &efi_global_variable_guid,
> > + EFI_VARIABLE_NON_VOLATILE |
> > + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > + EFI_VARIABLE_RUNTIME_ACCESS,
> > + size, new_bootorder, false);
> > +
> > + save_data->selected = true;
> > +out:
> > + free(new_bootorder);
> > +
> > + return ret;
> > +}
>
> [...]
>
> Thanks
> /Ilias
More information about the U-Boot
mailing list