| Message ID | 20190524065948.21566-1-takahiro.akashi@linaro.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | [U-Boot,v2] cmd: env: print a message when setting UEFI variable failed | expand |
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@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. > 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? Best regards Heinrich > + ret & ~EFI_ERROR_MASK); > + ret = CMD_RET_FAILURE; > + } > out: > free(value); > free(var_name16); >
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@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. > > 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. Thanks, -Takahiro Akashi > Best regards > > Heinrich > > > + ret & ~EFI_ERROR_MASK); > > + ret = CMD_RET_FAILURE; > > + } > > out: > > free(value); > > free(var_name16); > > >
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@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); >>> >> >
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); 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 & ~EFI_ERROR_MASK); + ret = CMD_RET_FAILURE; + } out: free(value); free(var_name16);
Error message will alert a user that setting/deleting a variable failed. Signed-off-by: AKASHI Takahiro <takahiro.akashi@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(-)