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