[U-Boot] [PATCH 1/2] efi_loader: implement GetNextVariableName()
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#GetNextVariableN... */ 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.2... */
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);
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
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#GetNextVariableN... */ 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.2... */
2.19.1
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#GetNextVariableN... */
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.2... */
Thanks a lot for filling this gap in our EFI implementation.
Best regards
Heinrich
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#GetNextVariableN... */
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.2... */
Thanks a lot for filling this gap in our EFI implementation.
Best regards
Heinrich
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#GetNextVariableN... */
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.2... */
Thanks a lot for filling this gap in our EFI implementation.
Best regards
Heinrich
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#GetNextVariableN... */
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.2... */
Thanks a lot for filling this gap in our EFI implementation.
Best regards
Heinrich
participants (2)
-
AKASHI Takahiro -
Heinrich Schuchardt