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.aka...@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 mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot