* [U-Boot] [PATCH 1/2] efi_loader: implement GetNextVariableName()
@ 2018-12-14 9:47 AKASHI Takahiro
2018-12-14 9:47 ` [U-Boot] [PATCH 2/2] efi_selftest: fix variables test for GetNextVariableName() AKASHI Takahiro
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: AKASHI Takahiro @ 2018-12-14 9:47 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH 2/2] efi_selftest: fix variables test for GetNextVariableName()
2018-12-14 9:47 [U-Boot] [PATCH 1/2] efi_loader: implement GetNextVariableName() AKASHI Takahiro
@ 2018-12-14 9:47 ` AKASHI Takahiro
2019-01-16 17:54 ` Heinrich Schuchardt
2019-01-16 7:08 ` [U-Boot] [PATCH 1/2] efi_loader: implement GetNextVariableName() AKASHI Takahiro
2019-01-16 18:43 ` Heinrich Schuchardt
2 siblings, 1 reply; 8+ messages in thread
From: AKASHI Takahiro @ 2018-12-14 9:47 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH 1/2] efi_loader: implement GetNextVariableName()
2018-12-14 9:47 [U-Boot] [PATCH 1/2] efi_loader: implement GetNextVariableName() AKASHI Takahiro
2018-12-14 9:47 ` [U-Boot] [PATCH 2/2] efi_selftest: fix variables test for GetNextVariableName() AKASHI Takahiro
@ 2019-01-16 7:08 ` AKASHI Takahiro
2019-01-16 18:43 ` Heinrich Schuchardt
2 siblings, 0 replies; 8+ messages in thread
From: AKASHI Takahiro @ 2019-01-16 7:08 UTC (permalink / raw)
To: u-boot
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH 2/2] efi_selftest: fix variables test for GetNextVariableName()
2018-12-14 9:47 ` [U-Boot] [PATCH 2/2] efi_selftest: fix variables test for GetNextVariableName() AKASHI Takahiro
@ 2019-01-16 17:54 ` Heinrich Schuchardt
0 siblings, 0 replies; 8+ messages in thread
From: Heinrich Schuchardt @ 2019-01-16 17:54 UTC (permalink / raw)
To: u-boot
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>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH 1/2] efi_loader: implement GetNextVariableName()
2018-12-14 9:47 [U-Boot] [PATCH 1/2] efi_loader: implement GetNextVariableName() AKASHI Takahiro
2018-12-14 9:47 ` [U-Boot] [PATCH 2/2] efi_selftest: fix variables test for GetNextVariableName() AKASHI Takahiro
2019-01-16 7:08 ` [U-Boot] [PATCH 1/2] efi_loader: implement GetNextVariableName() AKASHI Takahiro
@ 2019-01-16 18:43 ` Heinrich Schuchardt
2019-01-17 2:14 ` AKASHI Takahiro
2 siblings, 1 reply; 8+ messages in thread
From: Heinrich Schuchardt @ 2019-01-16 18:43 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH 1/2] efi_loader: implement GetNextVariableName()
2019-01-16 18:43 ` Heinrich Schuchardt
@ 2019-01-17 2:14 ` AKASHI Takahiro
2019-01-17 7:09 ` Heinrich Schuchardt
0 siblings, 1 reply; 8+ messages in thread
From: AKASHI Takahiro @ 2019-01-17 2:14 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH 1/2] efi_loader: implement GetNextVariableName()
2019-01-17 2:14 ` AKASHI Takahiro
@ 2019-01-17 7:09 ` Heinrich Schuchardt
2019-01-17 8:15 ` AKASHI Takahiro
0 siblings, 1 reply; 8+ messages in thread
From: Heinrich Schuchardt @ 2019-01-17 7:09 UTC (permalink / raw)
To: u-boot
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH 1/2] efi_loader: implement GetNextVariableName()
2019-01-17 7:09 ` Heinrich Schuchardt
@ 2019-01-17 8:15 ` AKASHI Takahiro
0 siblings, 0 replies; 8+ messages in thread
From: AKASHI Takahiro @ 2019-01-17 8:15 UTC (permalink / raw)
To: u-boot
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
> >
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-01-17 8:15 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-14 9:47 [U-Boot] [PATCH 1/2] efi_loader: implement GetNextVariableName() AKASHI Takahiro
2018-12-14 9:47 ` [U-Boot] [PATCH 2/2] efi_selftest: fix variables test for GetNextVariableName() AKASHI Takahiro
2019-01-16 17:54 ` Heinrich Schuchardt
2019-01-16 7:08 ` [U-Boot] [PATCH 1/2] efi_loader: implement GetNextVariableName() AKASHI Takahiro
2019-01-16 18:43 ` Heinrich Schuchardt
2019-01-17 2:14 ` AKASHI Takahiro
2019-01-17 7:09 ` Heinrich Schuchardt
2019-01-17 8:15 ` AKASHI Takahiro
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.