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

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

[AKASHI Takahiro]

The current GetNextVariableName() is a placeholder.
With this patch, it works well as expected.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@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;
+
+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? */
+	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);
+
+	return EFI_SUCCESS;
+}
+
 /* http://wiki.phoenix.com/wiki/index.php/EFI_RUNTIME_SERVICES#GetNextVariableName.28.29 */
 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]) {
+		/* 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;
+
+			variable = strchr(variable, '\n');
+			if (variable)
+				variable++;
+		}
+
+		free(native_name);
+		if (!(variable && *variable))
+			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 */
+		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);
+
+		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 */
-- 
2.19.1

[U-Boot] [PATCH 2/2] efi_selftest: fix variables test for GetNextVariableName()

[AKASHI Takahiro]

There is a bug in efi variables test.
Fix it with some cosmetic improvements.

Please note that efi variables test still fails at QueryVariableInfo()
and GetVariable(), but this is not due to a change in this patch.
  ==8<==
  Testing EFI API implementation

  Selected test: 'variables'

  Setting up 'variables'
  Setting up 'variables' succeeded

  Executing 'variables'
  .../u-boot/lib/efi_selftest/efi_selftest_variables.c(60):
  TODO: QueryVariableInfo failed
  .../u-boot/lib/efi_selftest/efi_selftest_variables.c(131):
  TODO: GetVariable returned wrong length 7
  .../u-boot/lib/efi_selftest/efi_selftest_variables.c(133):
  TODO: GetVariable returned wrong value
  Executing 'variables' succeeded

  Boot services terminated

  Summary: 0 failures
  ==>8==

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_selftest/efi_selftest_variables.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/lib/efi_selftest/efi_selftest_variables.c b/lib/efi_selftest/efi_selftest_variables.c
index e4c389a872fa..c58434289325 100644
--- a/lib/efi_selftest/efi_selftest_variables.c
+++ b/lib/efi_selftest/efi_selftest_variables.c
@@ -141,19 +141,22 @@ static int execute(void)
 		if (ret == EFI_NOT_FOUND)
 			break;
 		if (ret != EFI_SUCCESS) {
-			efi_st_todo("GetNextVariableName failed\n");
-			break;
+			efi_st_error("GetNextVariableName failed (%u)\n",
+				     (unsigned int)ret);
+			return EFI_ST_FAILURE;
 		}
 		if (!efi_st_memcmp(&guid, &guid_vendor0, sizeof(efi_guid_t)) &&
 		    !efi_st_strcmp_16_8(varname, "efi_st_var0"))
-			flag |= 2;
+			flag |= 1;
 		if (!efi_st_memcmp(&guid, &guid_vendor1, sizeof(efi_guid_t)) &&
 		    !efi_st_strcmp_16_8(varname, "efi_st_var1"))
 			flag |= 2;
 	}
-	if (flag != 3)
-		efi_st_todo(
+	if (flag != 3) {
+		efi_st_error(
 			"GetNextVariableName did not return all variables\n");
+		return EFI_ST_FAILURE;
+	}
 	/* Delete variable 1 */
 	ret = runtime->set_variable(L"efi_st_var1", &guid_vendor1,
 				    0, 0, NULL);
-- 
2.19.1

[U-Boot] [PATCH 2/2] efi_selftest: fix variables test for GetNextVariableName()

[Heinrich Schuchardt]

On 12/14/18 10:47 AM, AKASHI Takahiro wrote:
> There is a bug in efi variables test.
> Fix it with some cosmetic improvements.
> 
> Please note that efi variables test still fails at QueryVariableInfo()
> and GetVariable(), but this is not due to a change in this patch.
>   ==8<==
>   Testing EFI API implementation
> 
>   Selected test: 'variables'
> 
>   Setting up 'variables'
>   Setting up 'variables' succeeded
> 
>   Executing 'variables'
>   .../u-boot/lib/efi_selftest/efi_selftest_variables.c(60):
>   TODO: QueryVariableInfo failed
>   .../u-boot/lib/efi_selftest/efi_selftest_variables.c(131):
>   TODO: GetVariable returned wrong length 7
>   .../u-boot/lib/efi_selftest/efi_selftest_variables.c(133):
>   TODO: GetVariable returned wrong value
>   Executing 'variables' succeeded
> 
>   Boot services terminated
> 
>   Summary: 0 failures
>   ==>8==
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

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

[AKASHI Takahiro]

Alex, Heinrich,

Please review this patch set.

Thanks,
-Takahiro Akashi

On Fri, Dec 14, 2018 at 06:47:11PM +0900, AKASHI Takahiro wrote:
> The current GetNextVariableName() is a placeholder.
> With this patch, it works well as expected.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@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;
> +
> +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? */
> +	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);
> +
> +	return EFI_SUCCESS;
> +}
> +
>  /* http://wiki.phoenix.com/wiki/index.php/EFI_RUNTIME_SERVICES#GetNextVariableName.28.29 */
>  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]) {
> +		/* 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;
> +
> +			variable = strchr(variable, '\n');
> +			if (variable)
> +				variable++;
> +		}
> +
> +		free(native_name);
> +		if (!(variable && *variable))
> +			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 */
> +		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);
> +
> +		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 */
> -- 
> 2.19.1
> 

[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@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

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

[AKASHI Takahiro]

Heinrich,

Thank you for your quick review.

On Wed, Jan 16, 2019 at 07:43:47PM +0100, Heinrich Schuchardt wrote:
> 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@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.

I think that the function name describes itself clearly, but OK.

> Describe every parameter
> 
> Clearly state set variable_name_size is in bytes (cf.
> EmuGetNextVariableName() in EDK2)

Right.

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

Which part of code do you mean? I don't think so.

> > +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.

OK

> > +	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.

Right.

> > +
> > +	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

OK, but not for efi_get/set_variable() as I didn't touch anything there.

> >  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.

Which part of code do you mean? I don't see any duplication.

> > +		/* 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.

No, "native_name" already contains a given guid.

> Consider the case that the GUID changes between two calls.

UEFI specification, section 8.2, clearly describes;
   When VariableName is a pointer to a Null character, VendorGuid is ignored.
   etNextVariableName() cannot be used as a filter to return variable names
   with a specific GUID. Instead, the entire list of variables must be
   retrieved, and the caller may act as a filter if it chooses.


> > +			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.

Well, this "if" clause corresponds with a termination condition of
the previous "for" clause and checks whether a for loop has been exhausted.
So my expression would be better IMO.

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

ditto

> 
> > +			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.

OK

> > +		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.

No. Please see UEFI specification quoted above.

Thanks,
-Takahiro Akashi

> 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

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

[Heinrich Schuchardt]

On 1/17/19 3:14 AM, AKASHI Takahiro wrote:
> Heinrich,
> 
> Thank you for your quick review.
> 
> On Wed, Jan 16, 2019 at 07:43:47PM +0100, Heinrich Schuchardt wrote:
>> 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@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.
> 
> I think that the function name describes itself clearly, but OK.
> 
>> Describe every parameter
>>
>> Clearly state set variable_name_size is in bytes (cf.
>> EmuGetNextVariableName() in EDK2)
> 
> Right.
> 
>> This function duplicates some of the code in efi_get_variable. So,
>> please, use it in efi_get_variable too.
> 
> Which part of code do you mean? I don't think so.

Checking buffer size.
Comparing vendor GUID.
Extracting an EFI variable from a U-Boot variable.

> 
>>> +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.
> 
> OK
> 
>>> +	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.
> 
> Right.
> 
>>> +
>>> +	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
> 
> OK, but not for efi_get/set_variable() as I didn't touch anything there.

For the other functions we should do it in a separate patch.

> 
>>>  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.
> 
> Which part of code do you mean? I don't see any duplication.
> 
>>> +		/* 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.
> 
> No, "native_name" already contains a given guid.
> 
>> Consider the case that the GUID changes between two calls.
> 
> UEFI specification, section 8.2, clearly describes;
>    When VariableName is a pointer to a Null character, VendorGuid is ignored.
>    etNextVariableName() cannot be used as a filter to return variable names
>    with a specific GUID. Instead, the entire list of variables must be
>    retrieved, and the caller may act as a filter if it chooses.
> 

Look at the list of error codes. EFI_INVALID_PARAMETER has to be
returned if name or GUID does not match an existing variable.

Regards

Heinrich

> 
>>> +			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.
> 
> Well, this "if" clause corresponds with a termination condition of
> the previous "for" clause and checks whether a for loop has been exhausted.
> So my expression would be better IMO.
> 
>> Please, consider the case that the variable is not on the list because
>> the variable has already been deleted.
> 
> ditto
> 
>>
>>> +			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.
> 
> OK
> 
>>> +		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.
> 
> No. Please see UEFI specification quoted above.
> 
> Thanks,
> -Takahiro Akashi
> 
>> 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
> 

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

[AKASHI Takahiro]

On Thu, Jan 17, 2019 at 08:09:57AM +0100, Heinrich Schuchardt wrote:
> On 1/17/19 3:14 AM, AKASHI Takahiro wrote:
> > Heinrich,
> > 
> > Thank you for your quick review.
> > 
> > On Wed, Jan 16, 2019 at 07:43:47PM +0100, Heinrich Schuchardt wrote:
> >> 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@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.
> > 
> > I think that the function name describes itself clearly, but OK.
> > 
> >> Describe every parameter
> >>
> >> Clearly state set variable_name_size is in bytes (cf.
> >> EmuGetNextVariableName() in EDK2)
> > 
> > Right.
> > 
> >> This function duplicates some of the code in efi_get_variable. So,
> >> please, use it in efi_get_variable too.
> > 
> > Which part of code do you mean? I don't think so.
> 
> Checking buffer size.
> Comparing vendor GUID.
> Extracting an EFI variable from a U-Boot variable.

Please specify exact line numbers on both side.
Otherwise, I don't get you.

> > 
> >>> +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.
> > 
> > OK
> > 
> >>> +	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.
> > 
> > Right.
> > 
> >>> +
> >>> +	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
> > 
> > OK, but not for efi_get/set_variable() as I didn't touch anything there.
> 
> For the other functions we should do it in a separate patch.

Not me.

> > 
> >>>  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.
> > 
> > Which part of code do you mean? I don't see any duplication.

What about this?

> > 
> >>> +		/* 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.
> > 
> > No, "native_name" already contains a given guid.
> > 
> >> Consider the case that the GUID changes between two calls.
> > 
> > UEFI specification, section 8.2, clearly describes;
> >    When VariableName is a pointer to a Null character, VendorGuid is ignored.
> >    etNextVariableName() cannot be used as a filter to return variable names
> >    with a specific GUID. Instead, the entire list of variables must be
> >    retrieved, and the caller may act as a filter if it chooses.
> > 
> 
> Look at the list of error codes. EFI_INVALID_PARAMETER has to be
> returned if name or GUID does not match an existing variable.

The code below behaves exactly as you mention:
	/* search for the last-returned variable */
	ret = efi_to_native(&native_name, variable_name, vendor);

	name_len = strlen(native_name);

	for (variable = efi_variables_list; variable && *variable;) {
		...
	}
	free(native_name);
	if (!(variable && *variable))
		return EFI_EXIT(EFI_INVALID_PARAMETER);

-Takahiro Akashi

> Regards
> 
> Heinrich
> 
> > 
> >>> +			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.
> > 
> > Well, this "if" clause corresponds with a termination condition of
> > the previous "for" clause and checks whether a for loop has been exhausted.
> > So my expression would be better IMO.
> > 
> >> Please, consider the case that the variable is not on the list because
> >> the variable has already been deleted.
> > 
> > ditto
> > 
> >>
> >>> +			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.
> > 
> > OK
> > 
> >>> +		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.
> > 
> > No. Please see UEFI specification quoted above.
> > 
> > Thanks,
> > -Takahiro Akashi
> > 
> >> 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
> > 
>