Re: [PATCH 2/3] eficonfig: refactor change boot order implementation

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@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

#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?

• 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?

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;
  

  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?

• }
• 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