* [U-Boot] [PATCH] cmd: env: print a message when setting UEFI variable failed
@ 2019-05-14 4:58 AKASHI Takahiro
2019-05-14 6:21 ` Heinrich Schuchardt
0 siblings, 1 reply; 4+ messages in thread
From: AKASHI Takahiro @ 2019-05-14 4:58 UTC (permalink / raw)
To: u-boot
Error message will alert a user that setting/deleting a variable failed.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@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);
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",
+ ret & ~EFI_ERROR_MASK);
+ ret = CMD_RET_FAILURE;
+ }
out:
free(value);
free(var_name16);
--
2.21.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH] cmd: env: print a message when setting UEFI variable failed
2019-05-14 4:58 [U-Boot] [PATCH] cmd: env: print a message when setting UEFI variable failed AKASHI Takahiro
@ 2019-05-14 6:21 ` Heinrich Schuchardt
2019-05-14 8:08 ` AKASHI Takahiro
0 siblings, 1 reply; 4+ messages in thread
From: Heinrich Schuchardt @ 2019-05-14 6:21 UTC (permalink / raw)
To: u-boot
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@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");
> 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.
Regards
Heinrich
> + ret & ~EFI_ERROR_MASK);
> + ret = CMD_RET_FAILURE;
> + }
> out:
> free(value);
> free(var_name16);
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH] cmd: env: print a message when setting UEFI variable failed
2019-05-14 6:21 ` Heinrich Schuchardt
@ 2019-05-14 8:08 ` AKASHI Takahiro
2019-05-14 10:38 ` Heinrich Schuchardt
0 siblings, 1 reply; 4+ messages in thread
From: AKASHI Takahiro @ 2019-05-14 8:08 UTC (permalink / raw)
To: u-boot
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@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.
> > 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);
> >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH] cmd: env: print a message when setting UEFI variable failed
2019-05-14 8:08 ` AKASHI Takahiro
@ 2019-05-14 10:38 ` Heinrich Schuchardt
0 siblings, 0 replies; 4+ messages in thread
From: Heinrich Schuchardt @ 2019-05-14 10:38 UTC (permalink / raw)
To: u-boot
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@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);
>>>
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-05-14 10:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-05-14 4:58 [U-Boot] [PATCH] cmd: env: print a message when setting UEFI variable failed AKASHI Takahiro
2019-05-14 6:21 ` Heinrich Schuchardt
2019-05-14 8:08 ` AKASHI Takahiro
2019-05-14 10:38 ` Heinrich Schuchardt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.