[U-Boot] [PATCH v2 03/11] cmd: efidebug: rework "boot dump" sub-command using GetNextVariableName()

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 4/24/19 8:30 AM, AKASHI Takahiro wrote:
> Efidebug command should be implemented using well-defined EFI interfaces,
> rather than using internal functions/data. This change will be needed in
> a later patch where UEFI variables are re-implemented.

I had trouble to get the point. In the next version I suggest to write:

Currently in do_efi_boot_dump() we directly read EFI variables from the
related environment variables. To accomodate alternative storage
backends we should switch to using the UEFI API instead.

>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   cmd/efidebug.c | 92 ++++++++++++++++++++++++++++++++++++--------------
>   1 file changed, 66 insertions(+), 26 deletions(-)
>
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index a40c4f4be286..8890dd7268f1 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -509,7 +509,7 @@ static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag,
>   	if (argc < 6 || argc > 7)
>   		return CMD_RET_USAGE;
>
> -	id = (int)simple_strtoul(argv[1], &endp, 16);
> +	id = simple_strtoul(argv[1], &endp, 16);

This change is correct but unrelated. Please, put it into a separate
patch. Or at least mention it in the commit message.

>   	if (*endp != '\0' || id > 0xffff)
>   		return CMD_RET_USAGE;
>
> @@ -595,7 +595,7 @@ static int do_efi_boot_rm(cmd_tbl_t *cmdtp, int flag,
>
>   	guid = efi_global_variable_guid;
>   	for (i = 1; i < argc; i++, argv++) {
> -		id = (int)simple_strtoul(argv[1], &endp, 16);
> +		id = simple_strtoul(argv[1], &endp, 16);

Same here.

>   		if (*endp != '\0' || id > 0xffff)
>   			return CMD_RET_FAILURE;
>
> @@ -693,6 +693,27 @@ static void show_efi_boot_opt(int id)
>   	free(data);
>   }
>
> +static bool u16_isxdigit(u16 c)
> +{
> +	if (c & 0xff00)
> +		return false;
> +
> +	return isxdigit((u8)c);
> +}
> +
> +static int u16_tohex(u16 c)
> +{
> +	if (c >= '0' && c < '9')
> +		return c - '0';
> +	if (c >= 'A' && c < 'F')
> +		return c - 'A' + 10;
> +	if (c >= 'a' && c < 'f')
> +		return c - 'a' + 10;

Does the UEFI spec really allow lower case hexadecimal numbers here?
I only found an example with capital numbers. Would this imply that I
could have variables Boot00a0 and Boot00A0 side by side? Which one would
be selected by boot option 00a0?

Or should SetVariable() make a case insensitive search for variable names?

In EDK2 function FindVariableEx() in
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
uses CompareMem() to compare variable names.

Function BmCharToUint() is used to check the digits of boot options and
has this comment:

"It assumes the input Char is in the scope of L'0' ~ L'9'
and L'A' ~ L'F'"

So let's us assume that variable names are case sensitive and Boot
entries can only have capital hexadecimal digits.

So u16_isxdigit() is incorrect.

> +
> +	/* dummy */
> +	return -1;

As we do not check bounds here the function could be reduced to:

return c > '9' ? c - ('A' - 0xa) : c - '9';

But I would prefer one library function instead of the two static
functions which returns -1 for a non-digit and 0 - 15 for a digit.
And a unit test in test/unicoode_ut.c

> +}
> +
>   /**
>    * show_efi_boot_dump() - dump all UEFI load options
>    *
> @@ -709,38 +730,57 @@ static void show_efi_boot_opt(int id)
>   static int do_efi_boot_dump(cmd_tbl_t *cmdtp, int flag,
>   			    int argc, char * const argv[])
>   {
> -	char regex[256];
> -	char * const regexlist[] = {regex};
> -	char *variables = NULL, *boot, *value;
> -	int len;
> -	int id;
> +	u16 *var_name16, *p;
> +	efi_uintn_t buf_size, size;
> +	efi_guid_t guid;
> +	int id, i;
> +	efi_status_t ret;
>
>   	if (argc > 1)
>   		return CMD_RET_USAGE;
>
> -	snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_Boot[0-9A-F]+");
> -
> -	/* TODO: use GetNextVariableName? */
> -	len = hexport_r(&env_htab, '\n', H_MATCH_REGEX | H_MATCH_KEY,
> -			&variables, 0, 1, regexlist);
> +	buf_size = 128;
> +	var_name16 = malloc(buf_size);
> +	if (!var_name16)
> +		return CMD_RET_FAILURE;
>
> -	if (!len)
> -		return CMD_RET_SUCCESS;
> +	var_name16[0] = 0;
> +	for (;;) {
> +		size = buf_size;
> +		ret = EFI_CALL(efi_get_next_variable_name(&size, var_name16,
> +							  &guid));
> +		if (ret == EFI_NOT_FOUND)
> +			break;
> +		if (ret == EFI_BUFFER_TOO_SMALL) {
> +			buf_size = size;
> +			p = realloc(var_name16, buf_size);
> +			if (!p) {
> +				free(var_name16);
> +				return CMD_RET_FAILURE;
> +			}
> +			var_name16 = p;
> +			ret = EFI_CALL(efi_get_next_variable_name(&size,
> +								  var_name16,
> +								  &guid));
> +		}
> +		if (ret != EFI_SUCCESS) {
> +			free(var_name16);
> +			return CMD_RET_FAILURE;
> +		}
>
> -	if (len < 0)
> -		return CMD_RET_FAILURE;
> +		if (u16_strncmp(var_name16, L"Boot", 4) || var_name16[8] ||

We can use memcmp() here. This avoids introducing u16_strncmp().

> +		    !u16_isxdigit(var_name16[4]) ||
> +		    !u16_isxdigit(var_name16[5]) ||
> +		    !u16_isxdigit(var_name16[6]) ||
> +		    !u16_isxdigit(var_name16[7]))
> +			continue;
>
> -	boot = variables;
> -	while (*boot) {
> -		value = strstr(boot, "Boot") + 4;
> -		id = (int)simple_strtoul(value, NULL, 16);
> +		for (id = 0, i = 0; i < 4; i++)
> +			id = (id << 4) + u16_tohex(var_name16[4 + i]);

This all can be simplified if we unify u16_isxdigit() and u16_tohex().
Just one function returning -1 if not a hexadecimal and 0 - 15 otherwise.

>   		show_efi_boot_opt(id);
> -		boot = strchr(boot, '\n');
> -		if (!*boot)
> -			break;
> -		boot++;
>   	}
> -	free(variables);
> +
> +	free(var_name16);
>
>   	return CMD_RET_SUCCESS;
>   }
> @@ -914,7 +954,7 @@ static int do_efi_boot_order(cmd_tbl_t *cmdtp, int flag,
>   		return CMD_RET_FAILURE;
>
>   	for (i = 0; i < argc; i++) {
> -		id = (int)simple_strtoul(argv[i], &endp, 16);
> +		id = simple_strtoul(argv[i], &endp, 16);

This change is correct but unrelated. Please, put it into a separate
patch. Or at least mention it in the commit message.

Best regards

Heinrich

>   		if (*endp != '\0' || id > 0xffff) {
>   			printf("invalid value: %s\n", argv[i]);
>   			ret = CMD_RET_FAILURE;
>