From mboxrd@z Thu Jan  1 00:00:00 1970
From: AKASHI Takahiro <takahiro.akashi@linaro.org>
Date: Tue, 14 May 2019 17:08:30 +0900
Subject: [U-Boot] [PATCH] cmd: env: print a message when setting UEFI
 variable failed
In-Reply-To: <43c3f7e1-fb0f-e4d2-4aa4-07269850376d@gmx.de>
References: <20190514045853.10166-1-takahiro.akashi@linaro.org>
 <43c3f7e1-fb0f-e4d2-4aa4-07269850376d@gmx.de>
Message-ID: <20190514080829.GP11160@linaro.org>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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);
> >
>