Re: [PATCH v3 4/5] eficonfig: use efi_get_next_variable_name_int()

On Fri, Dec 02, 2022 at 01:59:36PM +0900, Masahisa Kojima wrote:

eficonfig command reads all possible UEFI load options from 0x0000 to 0xFFFF to construct the menu. This takes too much time in some environment. This commit uses efi_get_next_variable_name_int() to read all existing UEFI load options to significantlly reduce the count of efi_get_var() call.

Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org

No update since v2

v1->v2:

• totaly change the implemention, remove new Kconfig introduced in v1.
• use efi_get_next_variable_name_int() to read the all existing UEFI variables, then enumerate the "Boot####" variables
• this commit does not provide the common function to enumerate all "Boot####" variables. I think common function does not simplify the implementation, because caller of efi_get_next_variable_name_int() needs to remember original buffer size, new buffer size and buffer address and it is a bit complicated when we consider to create common function.

cmd/eficonfig.c | 141 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 117 insertions(+), 24 deletions(-)

diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 88d507d04c..394ae67cce 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -1683,7 +1683,8 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected) u32 i; u16 *bootorder; efi_status_t ret;

• efi_uintn_t num, size;

• u16 *var_name16 = NULL, *p;
• efi_uintn_t num, size, buf_size; struct efimenu *efi_menu; struct list_head *pos, *n; struct eficonfig_entry *entry;

@@ -1707,14 +1708,43 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected) }

/* list the remaining load option not included in the BootOrder */

• for (i = 0; i <= 0xFFFF; i++) {

• /* If the index is included in the BootOrder, skip it */
  

• if (search_bootorder(bootorder, num, i, NULL))
  

• 	continue;
  

• buf_size = 128;
• var_name16 = malloc(buf_size);
• if (!var_name16)

• return EFI_OUT_OF_RESOURCES;
  
  

• ret = eficonfig_add_boot_selection_entry(efi_menu, i, selected);
  

• if (ret != EFI_SUCCESS)
  

• 	goto out;
  

• var_name16[0] = 0;

Is it worth using calloc instead of malloc and get rid of this?

• for (;;) {

• int index;
  

• efi_guid_t guid;
  

• size = buf_size;
  

• ret = efi_get_next_variable_name_int(&size, var_name16, &guid);
  

• if (ret == EFI_NOT_FOUND)
  

• 	break;
  

• if (ret == EFI_BUFFER_TOO_SMALL) {
  

• 	buf_size = size;
  

• 	p = realloc(var_name16, buf_size);
  

• 	if (!p) {
  

• 		free(var_name16);
  

• 		return EFI_OUT_OF_RESOURCES;
  

• 	}
  

• 	var_name16 = p;
  

• 	ret = efi_get_next_variable_name_int(&size, var_name16, &guid);
  

• }
  

• if (ret != EFI_SUCCESS) {
  

• 	free(var_name16);
  

• 	return ret;
  

• }
  

• if (efi_varname_is_load_option(var_name16, &index)) {
  

• 	/* If the index is included in the BootOrder, skip it */
  

• 	if (search_bootorder(bootorder, num, index, NULL))
  

• 		continue;
  

• 	ret = eficonfig_add_boot_selection_entry(efi_menu, index, selected);
  

• 	if (ret != EFI_SUCCESS)
  

• 		goto out;
  

• }
  
  

  if (efi_menu->count >= EFICONFIG_ENTRY_NUM_MAX - 1) break;

@@ -1732,6 +1762,8 @@ out: } eficonfig_destroy(efi_menu);

• free(var_name16);
• return ret;

}

@@ -1994,6 +2026,8 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi u32 i; char *title; efi_status_t ret;

• u16 *var_name16 = NULL, *p;

• efi_uintn_t size, buf_size;

  /* list the load option in the order of BootOrder variable */ for (i = 0; i < num; i++) {

@@ -2006,17 +2040,45 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi }

/* list the remaining load option not included in the BootOrder */

• for (i = 0; i < 0xFFFF; i++) {

• buf_size = 128;
• var_name16 = malloc(buf_size);
• if (!var_name16)

• return EFI_OUT_OF_RESOURCES;
  

• var_name16[0] = 0;
• for (;;) {

• int index;
  

• efi_guid_t guid;
  

• if (efi_menu->count >= EFICONFIG_ENTRY_NUM_MAX - 2) break;

• /* If the index is included in the BootOrder, skip it */
  

• if (search_bootorder(bootorder, num, i, NULL))
  

• 	continue;
  

• ret = eficonfig_add_change_boot_order_entry(efi_menu, i, false);
  

• size = buf_size;
  

• ret = efi_get_next_variable_name_int(&size, var_name16, &guid);
  

• if (ret == EFI_NOT_FOUND)
  

• 	break;
  

• if (ret == EFI_BUFFER_TOO_SMALL) {
  

• 	buf_size = size;
  

• 	p = realloc(var_name16, buf_size);
  

• 	if (!p) {
  

• 		ret = EFI_OUT_OF_RESOURCES;
  

• 		goto out;
  

• 	}
  

• 	var_name16 = p;
  

• 	ret = efi_get_next_variable_name_int(&size, var_name16, &guid);
  

• }
  

  if (ret != EFI_SUCCESS) goto out;

• if (efi_varname_is_load_option(var_name16, &index)) {
  

• 	/* If the index is included in the BootOrder, skip it */
  

• 	if (search_bootorder(bootorder, num, index, NULL))
  

• 		continue;
  

• 	ret = eficonfig_add_change_boot_order_entry(efi_menu, index, false);
  

• 	if (ret != EFI_SUCCESS)
  

• 		goto out;
  

• }
  

  }

  /* add "Save" and "Quit" entries */

@@ -2035,9 +2097,9 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi goto out;

efi_menu->active = 0;

• return EFI_SUCCESS;

out:

• free(var_name16);
• return ret;

}

@@ -2270,17 +2332,48 @@ out: efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_option *opt, efi_status_t count) {

• u32 i, j;

• u32 i; efi_uintn_t size; void *load_option; struct efi_load_option lo;
• u16 *var_name16 = NULL, *p; u16 varname[] = u"Boot####"; efi_status_t ret = EFI_SUCCESS;
• efi_uintn_t varname_size, buf_size;

• for (i = 0; i <= 0xFFFF; i++) {

• buf_size = 128;
• var_name16 = malloc(buf_size);
• if (!var_name16)

• return EFI_OUT_OF_RESOURCES;
  

• var_name16[0] = 0;
• for (;;) {

• int index;
  

• efi_guid_t guid;
  

  efi_uintn_t tmp;

• efi_create_indexed_name(varname, sizeof(varname), "Boot", i);
  

• varname_size = buf_size;
  

• ret = efi_get_next_variable_name_int(&varname_size, var_name16, &guid);
  

• if (ret == EFI_NOT_FOUND)
  

• 	break;
  

• if (ret == EFI_BUFFER_TOO_SMALL) {
  

• 	buf_size = varname_size;
  

• 	p = realloc(var_name16, buf_size);
  

• 	if (!p) {
  

• 		free(var_name16);
  

• 		return EFI_OUT_OF_RESOURCES;
  

• 	}
  

• 	var_name16 = p;
  

• 	ret = efi_get_next_variable_name_int(&varname_size, var_name16, &guid);
  

• }
  

• if (ret != EFI_SUCCESS) {
  

• 	free(var_name16);
  

• 	return ret;
  

• }
  

• if (!efi_varname_is_load_option(var_name16, &index))
  

• 	continue;
  

• efi_create_indexed_name(varname, sizeof(varname), "Boot", index);
  

  load_option = efi_get_var(varname, &efi_global_variable_guid, &size); if (!load_option) continue;

@@ -2292,15 +2385,15 @@ efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_op

if (size >= sizeof(efi_guid_bootmenu_auto_generated)) {
	if (guidcmp(lo.optional_data, &efi_guid_bootmenu_auto_generated) == 0) {

• 		for (j = 0; j < count; j++) {
  

• 			if (opt[j].size == tmp &&
  

• 			    memcmp(opt[j].lo, load_option, tmp) == 0) {
  

• 				opt[j].exist = true;
  

• 		for (i = 0; i < count; i++) {
  

• 			if (opt[i].size == tmp &&
  

• 			    memcmp(opt[i].lo, load_option, tmp) == 0) {
  

• 				opt[i].exist = true;
  			break;
  		}
  	}
  
  

• 		if (j == count) {
  

• 		if (i == count) {
  		ret = delete_boot_option(i);
  		if (ret != EFI_SUCCESS) {
  			free(load_option);
  

-- 2.17.1

Overall this looks correct and a good idea. The sequence of alloc -> efi_get_next_variable_name_int -> realloc -> efi_get_next_variable_name_int seems common and repeated though. Can we factor that out on a common function?

Thanks /Ilias