[U-Boot] [PATCH 1/2] efi_loader: implement GetNextVariableName()

[Heinrich Schuchardt]

On 12/14/18 10:47 AM, AKASHI Takahiro wrote:
> The current GetNextVariableName() is a placeholder.
> With this patch, it works well as expected.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> ---
>  lib/efi_loader/efi_variable.c | 116 +++++++++++++++++++++++++++++++++-
>  1 file changed, 115 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index 19d9cb865f25..dac2f49aa1cc 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -8,6 +8,9 @@
>  #include <malloc.h>
>  #include <charset.h>
>  #include <efi_loader.h>
> +#include <environment.h>
> +#include <search.h>
> +#include <uuid.h>
>  
>  #define READ_ONLY BIT(31)
>  
> @@ -241,14 +244,125 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name, efi_guid_t *vendor,
>  	return EFI_EXIT(EFI_SUCCESS);
>  }
>  
> +static char *efi_variables_list;
> +static char *efi_cur_variable;
> +

Please, provide a comment describing what this function is meant to do.

Describe every parameter

Clearly state set variable_name_size is in bytes (cf.
EmuGetNextVariableName() in EDK2)

This function duplicates some of the code in efi_get_variable. So,
please, use it in efi_get_variable too.

> +static efi_status_t parse_uboot_variable(char *variable,
> +					 efi_uintn_t *variable_name_size,
> +					 u16 *variable_name,
> +					 efi_guid_t *vendor,
> +					 u32 *attribute)
> +{



> +	char *guid, *name, *end, c;
> +	unsigned long name_size;
> +	u16 *p;
> +
> +	guid = strchr(variable, '_');
> +	if (!guid)
> +		return EFI_NOT_FOUND;
> +	guid++;
> +	name = strchr(guid, '_');
> +	if (!name)
> +		return EFI_NOT_FOUND;
> +	name++;
> +	end = strchr(name, '=');
> +	if (!end)
> +		return EFI_NOT_FOUND;
> +
> +	/* FIXME: size is in byte or u16? */

It is in bytes. See comment above.

> +	name_size = end - name;
> +	if (*variable_name_size < (name_size + 1)) {
> +		*variable_name_size = name_size + 1;
> +		return EFI_BUFFER_TOO_SMALL;
> +	}
> +	end++; /* point to value */
> +
> +	p = variable_name;
> +	utf8_utf16_strncpy(&p, name, name_size);
> +	variable_name[name_size] = 0;
> +
> +	c = *(name - 1);
> +	*(name - 1) = '\0'; /* guid need be null-terminated here */
> +	uuid_str_to_bin(guid, (unsigned char *)vendor, UUID_STR_FORMAT_GUID);
> +	*(name - 1) = c;
> +
> +	parse_attr(end, attribute);

You have to update variable_name_size.

> +
> +	return EFI_SUCCESS;
> +}
> +
>  /* http://wiki.phoenix.com/wiki/index.php/EFI_RUNTIME_SERVICES#GetNextVariableName.28.29 */

Please add a description of the function here like we have it in
efi_bootefi.c

>  efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
>  					       u16 *variable_name,
>  					       efi_guid_t *vendor)
>  {
> +	char *native_name, *variable;
> +	ssize_t name_len, list_len;
> +	char regex[256];
> +	char * const regexlist[] = {regex};
> +	u32 attribute;
> +	int i;
> +	efi_status_t ret;
> +
>  	EFI_ENTRY("%p \"%ls\" %pUl", variable_name_size, variable_name, vendor);
>  
> -	return EFI_EXIT(EFI_DEVICE_ERROR);
> +	if (!variable_name_size || !variable_name || !vendor)
> +		EFI_EXIT(EFI_INVALID_PARAMETER);
> +
> +	if (variable_name[0]) {

This code partially duplicates code in in efi_get_variable. Please,
carve out a common function.

> +		/* check null-terminated string */
> +		for (i = 0; i < *variable_name_size; i++)
> +			if (!variable_name[i])
> +				break;
> +		if (i >= *variable_name_size)
> +			return EFI_EXIT(EFI_INVALID_PARAMETER);
> +
> +		/* search for the last-returned variable */
> +		ret = efi_to_native(&native_name, variable_name, vendor);
> +		if (ret)
> +			return EFI_EXIT(ret);
> +
> +		name_len = strlen(native_name);
> +		for (variable = efi_variables_list; variable && *variable;) {
> +			if (!strncmp(variable, native_name, name_len) &&
> +			    variable[name_len] == '=')
> +				break;
> +

You miss to compare the GUID.

Consider the case that the GUID changes between two calls.

> +			variable = strchr(variable, '\n');
> +			if (variable)
> +				variable++;
> +		}
> +
> +		free(native_name);
> +		if (!(variable && *variable))

With less parentheses I can read the logic more easily:

if (!variable || !*variable)

But that is just a question of taste.

Please, consider the case that the variable is not on the list because
the variable has already been deleted.


> +			return EFI_EXIT(EFI_INVALID_PARAMETER);
> +
> +		/* next variable */
> +		variable = strchr(variable, '\n');
> +		if (variable)
> +			variable++;
> +		if (!(variable && *variable))
> +			return EFI_EXIT(EFI_NOT_FOUND);
> +	} else {
> +		/* new search */

Please, put a comment here explaining that the list of the preceding
search is freed here.

> +		free(efi_variables_list);
> +		efi_variables_list = NULL;
> +		efi_cur_variable = NULL;
> +
> +		snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_.*");
> +		list_len = hexport_r(&env_htab, '\n',
> +				     H_MATCH_REGEX | H_MATCH_KEY,
> +				     &efi_variables_list, 0, 1, regexlist);
> +		if (list_len <= 0)
> +			return EFI_EXIT(EFI_NOT_FOUND);

You miss to compare the vendor GUIDs.

Please, assume that variables are deleted or inserted while the caller
loops over the variables.
> +
> +		variable = efi_variables_list;
> +	}
> +
> +	ret = parse_uboot_variable(variable, variable_name_size, variable_name,
> +				   vendor, &attribute);
> +
> +	return EFI_EXIT(ret);
>  }
>  
>  /* http://wiki.phoenix.com/wiki/index.php/EFI_RUNTIME_SERVICES#SetVariable.28.29 */
> 

Thanks a lot for filling this gap in our EFI implementation.

Best regards

Heinrich