On Fri, 2 Dec 2022 at 16:36, Ilias Apalodimas <ilias.apalodi...@linaro.org> wrote: > > 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.koj...@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?
I tried to factor out these sequences into a common function, but I could not find proper common function interface. Even if we factor out some of these sequences, the caller still should take care the cases of ret == EFI_NOT_FOUND and ret != EFI_SUCCESS. We can factor out the EFI_BUFFER_TOO_SMALL and realloc sequence, but I think it will not make the caller simpler. Thanks, Masahisa Kojima > > Thanks > /Ilias