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