Re: [PATCH v2 1/2] eficonfig: carve out efi_get_next_variable_name_int calls

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

On Mon, Dec 19, 2022 at 11:33:12AM +0900, Masahisa Kojima wrote:
> To retrieve the EFI variable name by efi_get_next_variable_name_int(),
> the sequence of alloc -> efi_get_next_variable_name_int ->
> realloc -> efi_get_next_variable_name_int is required.
> In current code, this sequence repeatedly appears in
> the several functions. It should be curved out a common function.
>
> This commit also fixes the missing free() of var_name16
> in eficonfig_delete_invalid_boot_option().
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> Changes in v2:
> - fix typo in the commit message
> - rename efi_get_variable_name to efi_next_variable_name
>
>  cmd/eficonfig.c             | 62 +++++++++----------------------------
>  include/efi_loader.h        |  2 ++
>  lib/efi_loader/efi_helper.c | 34 ++++++++++++++++++++
>  3 files changed, 51 insertions(+), 47 deletions(-)
>
> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> index 394ae67cce..0b07dfc958 100644
> --- a/cmd/eficonfig.c
> +++ b/cmd/eficonfig.c
> @@ -1683,7 +1683,7 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected)
>  	u32 i;
>  	u16 *bootorder;
>  	efi_status_t ret;
> -	u16 *var_name16 = NULL, *p;
> +	u16 *var_name16 = NULL;
>  	efi_uintn_t num, size, buf_size;
>  	struct efimenu *efi_menu;
>  	struct list_head *pos, *n;
> @@ -1718,24 +1718,12 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected)
>  		int index;
>  		efi_guid_t guid;
>
> -		size = buf_size;
> -		ret = efi_get_next_variable_name_int(&size, var_name16, &guid);
> +		ret = efi_next_variable_name(&buf_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 (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))
> @@ -2026,7 +2014,7 @@ 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;
> +	u16 *var_name16 = NULL;
>  	efi_uintn_t size, buf_size;
>
>  	/* list the load option in the order of BootOrder variable */
> @@ -2054,19 +2042,9 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi
>  			break;
>
>  		size = buf_size;
> -		ret = efi_get_next_variable_name_int(&size, var_name16, &guid);
> +		ret = efi_next_variable_name(&buf_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;
>
> @@ -2336,10 +2314,10 @@ efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_op
>  	efi_uintn_t size;
>  	void *load_option;
>  	struct efi_load_option lo;
> -	u16 *var_name16 = NULL, *p;
> +	u16 *var_name16 = NULL;
>  	u16 varname[] = u"Boot####";
>  	efi_status_t ret = EFI_SUCCESS;
> -	efi_uintn_t varname_size, buf_size;
> +	efi_uintn_t buf_size;
>
>  	buf_size = 128;
>  	var_name16 = malloc(buf_size);
> @@ -2352,24 +2330,12 @@ efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_op
>  		efi_guid_t guid;
>  		efi_uintn_t tmp;
>
> -		varname_size = buf_size;
> -		ret = efi_get_next_variable_name_int(&varname_size, var_name16, &guid);
> +		ret = efi_next_variable_name(&buf_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 (ret != EFI_SUCCESS)
> +			goto out;
> +
>  		if (!efi_varname_is_load_option(var_name16, &index))
>  			continue;
>
> @@ -2407,6 +2373,8 @@ next:
>  	}
>
>  out:
> +	free(var_name16);
> +
>  	return ret;
>  }
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 0899e293e5..699176872d 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -708,6 +708,8 @@ int algo_to_len(const char *algo);
>  int efi_link_dev(efi_handle_t handle, struct udevice *dev);
>  int efi_unlink_dev(efi_handle_t handle);
>  bool efi_varname_is_load_option(u16 *var_name16, int *index);
> +efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
> +				    efi_guid_t *guid);
>
>  /**
>   * efi_size_in_pages() - convert size in bytes to size in pages
> diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> index 788cb9faec..1f4ab2b419 100644
> --- a/lib/efi_loader/efi_helper.c
> +++ b/lib/efi_loader/efi_helper.c
> @@ -223,3 +223,37 @@ bool efi_varname_is_load_option(u16 *var_name16, int *index)
>
>  	return false;
>  }
> +
> +/**
> + * efi_next_variable_name() - get next variable name
> + *
> + * This function is a wrapper of efi_get_next_variable_name_int().
> + * If efi_get_next_variable_name_int() returns EFI_BUFFER_TOO_SMALL,
> + * @size and @buf are updated by new buffer size and realloced buffer.
> + *
> + * @size:	pointer to the buffer size
> + * @buf:	pointer to the buffer
> + * @guid:	pointer to the guid
> + * Return:	status code
> + */
> +efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf, efi_guid_t *guid)
> +{
> +	u16 *p;
> +	efi_status_t ret;
> +	efi_uintn_t buf_size = *size;
> +
> +	ret = efi_get_next_variable_name_int(&buf_size, *buf, guid);
> +	if (ret == EFI_NOT_FOUND)
> +		return ret;
> +	if (ret == EFI_BUFFER_TOO_SMALL) {
> +		p = realloc(*buf, buf_size);
> +		if (!p)
> +			return EFI_OUT_OF_RESOURCES;
> +
> +		*buf = p;
> +		*size = buf_size;
> +		ret = efi_get_next_variable_name_int(&buf_size, *buf, guid);
> +	}
> +
> +	return ret;
> +}
> --
> 2.17.1
>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>