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