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

[Heinrich Schuchardt]

On 5/27/19 2:46 AM, AKASHI Takahiro wrote:
> On Fri, May 24, 2019 at 08:38:41PM +0200, Heinrich Schuchardt wrote:
>> On 5/24/19 8:59 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>
>>> ---
>>> Changes in v2 (May 24, 2019)
>>> * change a message to "Out of memory"
>>> * add more error messages
>>> ---
>>>   cmd/nvedit_efi.c | 11 ++++++++++-
>>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/cmd/nvedit_efi.c b/cmd/nvedit_efi.c
>>> index 2805e8182b41..88d6ca1cd7f2 100644
>>> --- a/cmd/nvedit_efi.c
>>> +++ b/cmd/nvedit_efi.c
>>> @@ -373,6 +373,8 @@ 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 an argument, %s\n",
>>> +				       argv[0]);
>>>   				ret = CMD_RET_FAILURE;
>>>   				goto out;
>>>   			}
>>> @@ -381,6 +383,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("## Out of memory (%ld bytes)\n", (len + 1) * 2);
>>
>> cmd/nvedit_efi.c: In function ‘do_env_set_efi’:
>> cmd/nvedit_efi.c:386:31: warning: format ‘%ld’ expects argument of type
>> ‘long int’, but argument 2 has type ‘size_t’ {aka ‘unsigned int’}
>> [-Wformat=]
>>     printf("## Out of memory (%ld bytes)\n", (len + 1) * 2);
>>                               ~~^            ~~~~~~~~~~~~~
>>                               %d
>>
>> If you want to print the number of bytes use %zu. But why should the
>> user care if it is 10 bytes or 100 bytes that could not be assigned? If
>> he is out of memory here anyway the system is messed up.
>
> Can you please clarify what you want me to do?
> Otherwise, I cannot submit a next version.

I would suggest to leave the size information away.

>
>>>   		ret = CMD_RET_FAILURE;
>>>   		goto out;
>>>   	}
>>> @@ -392,7 +395,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",
>>
>> ret has type size_t so this has to %zu.
>>
>> Think of a non-developer user who bought a device with pre-installedö
>> U-Boot. What would he do with the error code?
>
> Ditto.

I would suggest to leave the return code away.

Best regards

Heinrich

>
> Thanks,
> -Takahiro Akashi
>
>> Best regards
>>
>> Heinrich
>>
>>> +		       ret & ~EFI_ERROR_MASK);
>>> +		ret = CMD_RET_FAILURE;
>>> +	}
>>>   out:
>>>   	free(value);
>>>   	free(var_name16);
>>>
>>
>