On Sat, Dec 03, 2022 at 09:56:20AM +0900, Masahisa Kojima wrote: > 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.
I think mean the same thing here, but let me make sure. This snippet seems like it could be it's own function, no? 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); } Regards /Ilias > > Thanks, > Masahisa Kojima > > > > > Thanks > > /Ilias