[U-Boot] [PATCH] cmd: env: print a message when setting UEFI variable failed

[Heinrich Schuchardt]

On 5/14/19 10:08 AM, AKASHI Takahiro wrote:
> On Tue, May 14, 2019 at 08:21:55AM +0200, Heinrich Schuchardt wrote:
>> On 5/14/19 6:58 AM, AKASHI Takahiro wrote:
>>> Error message will alert a user that setting/deleting a variable failed.
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
>>> ---
>>>   cmd/nvedit_efi.c | 10 +++++++++-
>>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/cmd/nvedit_efi.c b/cmd/nvedit_efi.c
>>> index 2805e8182b41..e0d8f578ac33 100644
>>> --- a/cmd/nvedit_efi.c
>>> +++ b/cmd/nvedit_efi.c
>>> @@ -373,6 +373,7 @@ int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>
>>>   		for ( ; argc > 0; argc--, argv++)
>>>   			if (append_value(&value, &size, argv[0]) < 0) {
>>> +				printf("## Failed to process arguments\n");
>>>   				ret = CMD_RET_FAILURE;
>>>   				goto out;
>>>   			}
>>> @@ -381,6 +382,7 @@ int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>   	len = utf8_utf16_strnlen(var_name, strlen(var_name));
>>>   	var_name16 = malloc((len + 1) * 2);
>>>   	if (!var_name16) {
>>> +		printf("## Can't malloc %ld bytes\n", (len + 1) * 2);
>>
>> That message is much too technical. As a user I would not care about
>> malloc(), calloc() or any other memory allocation call failing.
>>
>> In other parts of our coding we use
>>
>> printf("ERROR: Out of memory\n");
>
> See _do_env_set() in cmd/nvedit.c.
> My message above is consistent with that function.

Then that printf() should be changed too.

Best regards

Heinrich

>
>
>>>   		ret = CMD_RET_FAILURE;
>>>   		goto out;
>>>   	}
>>> @@ -392,7 +394,13 @@ int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>   					EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>>   					EFI_VARIABLE_RUNTIME_ACCESS,
>>>   					size, value));
>>> -	ret = (ret == EFI_SUCCESS ? CMD_RET_SUCCESS : CMD_RET_FAILURE);
>>> +	if (ret == EFI_SUCCESS) {
>>> +		ret = CMD_RET_SUCCESS;
>>> +	} else {
>>> +		printf("## Failed to set EFI variable (%ld)\n",
>>
>> efi_set_variable() is called from different places in U-Boot (e.g.
>> efi_setup.c). On the other hand we want to make efi_set_variable()
>> available at runtime in funture.
>>
>> So may be we should replace all our EFI_CALL(efi_set_variable()) by an
>> internal function which provides error output.
>
> I don't think so.
> Whether such a message be printed should deter to a caller (or
> application).
> What message be appropriate will also depend on a context of caller.
>
> -Takahiro Akashi
>
>
>> Regards
>>
>> Heinrich
>>
>>> +		       ret & ~EFI_ERROR_MASK);
>>> +		ret = CMD_RET_FAILURE;
>>> +	}
>>>   out:
>>>   	free(value);
>>>   	free(var_name16);
>>>
>>
>