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

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

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