| Message ID | 20221202045937.7846-5-masahisa.kojima@linaro.org |
|---|---|
| State | Accepted |
| Commit | 140a8959d48f8ac3734d53b4c8b6b9b5596bc698 |
| Headers | show |
| Series | miscellaneous fixes of eficonfig | expand |
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
On 12/2/22 08:35, Ilias Apalodimas 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.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? It doesn't change the binary code size as var_name16 is already in a register. Best regards Heinrich > >> + 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
On Fri, 2 Dec 2022 at 16:36, Ilias Apalodimas <ilias.apalodimas@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.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? 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
On Sat, Dec 03, 2022 at 09:56:20AM +0900, Masahisa Kojima wrote: > On Fri, 2 Dec 2022 at 16:36, Ilias Apalodimas > <ilias.apalodimas@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.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? > > 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
On Tue, 6 Dec 2022 at 23:12, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > On Sat, Dec 03, 2022 at 09:56:20AM +0900, Masahisa Kojima wrote: > > On Fri, 2 Dec 2022 at 16:36, Ilias Apalodimas > > <ilias.apalodimas@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.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? > > > > 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); > } OK, I will create a common function. This patch was already merged, I will send a new patch. Thanks, Masahisa Kojima > > Regards > /Ilias > > > > Thanks, > > Masahisa Kojima > > > > > > > > Thanks > > > /Ilias
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; + 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);
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(-)