[PATCH 0/5] improve eficonfig usability

[PATCH 0/5] improve eficonfig usability

[Masahisa Kojima]

This series improves the eficonfig usability, enhances the error
message and fixes the issue reported by coverity.

Masahisa Kojima (5):
  menu: remove CTRL+C to quit
  eficonfig: CTRL+S to save the boot order
  eficonfig: set EFICONFIG_ENTRY_NUM_MAX to INT_MAX - 1
  eficonfig: include EFI_STATUS string in error message
  eficonfig: add error message of SetVariable

 cmd/bootmenu.c               |   2 +-
 cmd/eficonfig.c              | 127 ++++++++++++++++++++++++++++++-----
 cmd/eficonfig_sbkey.c        |  16 +++--
 common/menu.c                |   4 +-
 doc/usage/cmd/bootmenu.rst   |   2 +-
 include/efi_config.h         |   4 +-
 include/menu.h               |   1 +
 lib/efi_loader/efi_console.c |   2 +-
 8 files changed, 127 insertions(+), 31 deletions(-)

-- 
2.17.1

[PATCH 1/5] menu: remove CTRL+C to quit

[Masahisa Kojima]

On the sandbox called without "--terminal raw" CTRL+C leaves U-Boot,
"ESC/CTRL+C to quit" is misleading.

Let's remove CTRL+C to quit key handling from bootmenu and eficonfig menu.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 cmd/bootmenu.c               | 2 +-
 cmd/eficonfig.c              | 6 +++---
 common/menu.c                | 1 -
 doc/usage/cmd/bootmenu.rst   | 2 +-
 lib/efi_loader/efi_console.c | 2 +-
 5 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
index 3236ca5d79..8dc133c236 100644
--- a/cmd/bootmenu.c
+++ b/cmd/bootmenu.c
@@ -437,7 +437,7 @@ static void menu_display_statusline(struct menu *m)
 	printf(ANSI_CURSOR_POSITION, menu->count + 5, 1);
 	puts(ANSI_CLEAR_LINE);
 	printf(ANSI_CURSOR_POSITION, menu->count + 6, 3);
-	puts("Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit");
+	puts("Press UP/DOWN to move, ENTER to select, ESC to quit");
 	puts(ANSI_CLEAR_LINE_TO_END);
 	printf(ANSI_CURSOR_POSITION, menu->count + 7, 1);
 	puts(ANSI_CLEAR_LINE);
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
index 47c04cf536..f365a988d4 100644
--- a/cmd/eficonfig.c
+++ b/cmd/eficonfig.c
@@ -23,12 +23,12 @@
 
 static struct efi_simple_text_input_protocol *cin;
 const char *eficonfig_menu_desc =
-	"  Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit";
+	"  Press UP/DOWN to move, ENTER to select, ESC to quit";
 
 static const char *eficonfig_change_boot_order_desc =
 	"  Press UP/DOWN to move, +/- to change orde\n"
 	"  Press SPACE to activate or deactivate the entry\n"
-	"  Select [Save] to complete, ESC/CTRL+C to quit";
+	"  Select [Save] to complete, ESC to quit";
 
 static struct efi_simple_text_output_protocol *cout;
 static int avail_row;
@@ -927,7 +927,7 @@ static efi_status_t handle_user_input(u16 *buf, int buf_size,
 	       ANSI_CURSOR_POSITION
 	       "%s"
 	       ANSI_CURSOR_POSITION
-	       "  Press ENTER to complete, ESC/CTRL+C to quit",
+	       "  Press ENTER to complete, ESC to quit",
 	       0, 1, msg, 8, 1);
 
 	/* tmp is used to accept user cancel */
diff --git a/common/menu.c b/common/menu.c
index cdcdbb2a18..56401695de 100644
--- a/common/menu.c
+++ b/common/menu.c
@@ -492,7 +492,6 @@ enum bootmenu_key bootmenu_conv_key(int ichar)
 		/* enter key was pressed */
 		key = BKEY_SELECT;
 		break;
-	case CTL_CH('c'):
 	case '\e':
 		/* ^C was pressed */
 		key = BKEY_QUIT;
diff --git a/doc/usage/cmd/bootmenu.rst b/doc/usage/cmd/bootmenu.rst
index cb3c8d2f93..684a18d8e1 100644
--- a/doc/usage/cmd/bootmenu.rst
+++ b/doc/usage/cmd/bootmenu.rst
@@ -122,7 +122,7 @@ Example bootmenu is as below::
 Default behavior when user exits from the bootmenu
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 User can exit from bootmenu by selecting the last entry
-"U-Boot console"/"Quit" or ESC/CTRL+C key.
+"U-Boot console"/"Quit" or ESC key.
 
 When the CONFIG_BOOTMENU_DISABLE_UBOOT_CONSOLE is disabled,
 user exits from the bootmenu and returns to the U-Boot console.
diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
index 1ed8c7aa36..2c7536107a 100644
--- a/lib/efi_loader/efi_console.c
+++ b/lib/efi_loader/efi_console.c
@@ -1395,7 +1395,7 @@ efi_status_t efi_console_get_u16_string(struct efi_simple_text_input_protocol *c
 		} else if (key.unicode_char == u'\r') {
 			buf[len] = u'\0';
 			return EFI_SUCCESS;
-		} else if (key.unicode_char == 0x3 || key.scan_code == 23) {
+		} else if (key.scan_code == 23) {
 			return EFI_ABORTED;
 		} else if (key.unicode_char < 0x20) {
 			/* ignore control codes other than Ctrl+C, '\r' and '\b' */
-- 
2.17.1

[PATCH 2/5] eficonfig: CTRL+S to save the boot order

[Masahisa Kojima]

The change boot order menu in eficonfig can have at most INT_MAX lines
and it is troublesome to scroll down to the "Save" entry.

This commit assigns CTRL+S to save the boot order.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 cmd/eficonfig.c | 6 +++++-
 common/menu.c   | 3 +++
 include/menu.h  | 1 +
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
index f365a988d4..0a17b8cf34 100644
--- a/cmd/eficonfig.c
+++ b/cmd/eficonfig.c
@@ -28,7 +28,7 @@ const char *eficonfig_menu_desc =
 static const char *eficonfig_change_boot_order_desc =
 	"  Press UP/DOWN to move, +/- to change orde\n"
 	"  Press SPACE to activate or deactivate the entry\n"
-	"  Select [Save] to complete, ESC to quit";
+	"  CTRL+S to save, ESC to quit";
 
 static struct efi_simple_text_output_protocol *cout;
 static int avail_row;
@@ -1983,6 +1983,10 @@ char *eficonfig_choice_change_boot_order(void *data)
 				eficonfig_menu_down(efi_menu);
 
 			return NULL;
+		case BKEY_SAVE:
+			/* force to select "Save" entry */
+			efi_menu->active = efi_menu->count - 2;
+			fallthrough;
 		case BKEY_SELECT:
 			/* "Save" */
 			if (efi_menu->active == efi_menu->count - 2) {
diff --git a/common/menu.c b/common/menu.c
index 56401695de..da08f17747 100644
--- a/common/menu.c
+++ b/common/menu.c
@@ -502,6 +502,9 @@ enum bootmenu_key bootmenu_conv_key(int ichar)
 	case CTL_CH('n'):
 		key = BKEY_DOWN;
 		break;
+	case CTL_CH('s'):
+		key = BKEY_SAVE;
+		break;
 	case '+':
 		key = BKEY_PLUS;
 		break;
diff --git a/include/menu.h b/include/menu.h
index 1e88141d6b..64ce89b7d2 100644
--- a/include/menu.h
+++ b/include/menu.h
@@ -53,6 +53,7 @@ enum bootmenu_key {
 	BKEY_PLUS,
 	BKEY_MINUS,
 	BKEY_SPACE,
+	BKEY_SAVE,
 
 	BKEY_COUNT,
 };
-- 
2.17.1

[PATCH 3/5] eficonfig: set EFICONFIG_ENTRY_NUM_MAX to INT_MAX - 1

[Masahisa Kojima]

eficonfig_append_menu_entryi() accepts the number of entries
less than or equal to EFICONFIG_ENTRY_NUM_MAX.
EFICONFIG_ENTRY_NUM_MAX is currently set as INT_MAX, so
the invalid menu count check(efi_menu->count > EFICONFIG_ENTRY_NUM_MAX)
in eficonfig_process_common() is always false.

This commit sets EFICONFIG_ENTRY_NUM_MAX to (INT_MAX - 1).

Reported-by: Coverity (CID 435659)
Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 include/efi_config.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/efi_config.h b/include/efi_config.h
index e5edbb5e09..01ce9b2b06 100644
--- a/include/efi_config.h
+++ b/include/efi_config.h
@@ -11,7 +11,7 @@
 #include <efi_loader.h>
 #include <menu.h>
 
-#define EFICONFIG_ENTRY_NUM_MAX INT_MAX
+#define EFICONFIG_ENTRY_NUM_MAX (INT_MAX - 1)
 #define EFICONFIG_VOLUME_PATH_MAX 512
 #define EFICONFIG_FILE_PATH_MAX 512
 #define EFICONFIG_FILE_PATH_BUF_SIZE (EFICONFIG_FILE_PATH_MAX * sizeof(u16))
-- 
2.17.1

[PATCH 4/5] eficonfig: include EFI_STATUS string in error message

[Masahisa Kojima]

Current eficonfig_print_msg() does not show the return
value of EFI Boot/Runtime Services when the service call fails.
With this commit, user can know EFI_STATUS in the error message.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 cmd/eficonfig.c       | 95 +++++++++++++++++++++++++++++++++++++------
 cmd/eficonfig_sbkey.c | 16 ++++----
 include/efi_config.h  |  2 +-
 3 files changed, 93 insertions(+), 20 deletions(-)

diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
index 0a17b8cf34..b0c8637676 100644
--- a/cmd/eficonfig.c
+++ b/cmd/eficonfig.c
@@ -151,19 +151,90 @@ static void eficonfig_menu_adjust(struct efimenu *efi_menu, bool add)
 #define eficonfig_menu_up(_a) eficonfig_menu_adjust(_a, false)
 #define eficonfig_menu_down(_a) eficonfig_menu_adjust(_a, true)
 
+struct efi_status_str {
+	efi_status_t status;
+	char *str;
+};
+
+static const struct efi_status_str status_str_table[] = {
+	{EFI_LOAD_ERROR,		"Load Error"},
+	{EFI_INVALID_PARAMETER,		"Invalid Parameter"},
+	{EFI_UNSUPPORTED,		"Unsupported"},
+	{EFI_BAD_BUFFER_SIZE,		"Bad Buffer Size"},
+	{EFI_BUFFER_TOO_SMALL,		"Buffer Too Small"},
+	{EFI_NOT_READY,			"Not Ready"},
+	{EFI_DEVICE_ERROR,		"Device Error"},
+	{EFI_WRITE_PROTECTED,		"Write Protected"},
+	{EFI_OUT_OF_RESOURCES,		"Out of Resources"},
+	{EFI_VOLUME_CORRUPTED,		"Volume Corrupted"},
+	{EFI_VOLUME_FULL,		"Volume Full"},
+	{EFI_NO_MEDIA,			"No Media"},
+	{EFI_MEDIA_CHANGED,		"Media Changed"},
+	{EFI_NOT_FOUND,			"Not Found"},
+	{EFI_ACCESS_DENIED,		"Access Denied"},
+	{EFI_NO_RESPONSE,		"No Response"},
+	{EFI_NO_MAPPING,		"No Mapping"},
+	{EFI_TIMEOUT,			"Timeout"},
+	{EFI_NOT_STARTED,		"Not Started"},
+	{EFI_ALREADY_STARTED,		"Already Started"},
+	{EFI_ABORTED,			"Aborted"},
+	{EFI_ICMP_ERROR,		"ICMP Error"},
+	{EFI_TFTP_ERROR,		"TFTP Error"},
+	{EFI_PROTOCOL_ERROR,		"Protocol Error"},
+	{EFI_INCOMPATIBLE_VERSION,	"Incompatible Version"},
+	{EFI_SECURITY_VIOLATION,	"Security Violation"},
+	{EFI_CRC_ERROR,			"CRC Error"},
+	{EFI_END_OF_MEDIA,		"End of Media"},
+	{EFI_END_OF_FILE,		"End of File"},
+	{EFI_INVALID_LANGUAGE,		"Invalid Language"},
+	{EFI_COMPROMISED_DATA,		"Compromised Data"},
+	{EFI_IP_ADDRESS_CONFLICT,	"IP Address Conflict"},
+	{EFI_HTTP_ERROR,		"HTTP Error"},
+	{EFI_WARN_UNKNOWN_GLYPH,	"Warn Unknown Glyph"},
+	{EFI_WARN_DELETE_FAILURE,	"Warn Delete Failure"},
+	{EFI_WARN_WRITE_FAILURE,	"Warn Write Failure"},
+	{EFI_WARN_BUFFER_TOO_SMALL,	"Warn Buffer Too Small"},
+	{EFI_WARN_STALE_DATA,		"Warn Stale Data"},
+	{EFI_WARN_FILE_SYSTEM,		"Warn File System"},
+	{EFI_WARN_RESET_REQUIRED,	"Warn Reset Required"},
+	{0, ""},
+};
+
+/**
+ * struct get_status_str - get status string
+ *
+ * @status:	efi_status_t value to covert to string
+ * Return:	pointer to the string
+ */
+static char *get_error_str(efi_status_t status)
+{
+	u32 i;
+
+	for (i = 0; status_str_table[i].status != 0; i++) {
+		if (status == status_str_table[i].status)
+			return status_str_table[i].str;
+	}
+	return status_str_table[i].str;
+}
+
 /**
  * eficonfig_print_msg() - print message
  *
  * display the message to the user, user proceeds the screen
  * with any key press.
  *
- * @items:		pointer to the structure of each menu entry
- * @count:		the number of menu entry
- * @menu_header:	pointer to the menu header string
- * Return:	status code
+ * @msg:	pointer to the error message
+ * @status:	efi status code, set 0 if no status string output
  */
-void eficonfig_print_msg(char *msg)
+void eficonfig_print_msg(const char *msg, efi_status_t status)
 {
+	char str[128];
+
+	if (status == 0)
+		snprintf(str, sizeof(str), "%s", msg);
+	else
+		snprintf(str, sizeof(str), "%s (%s)", msg, get_error_str(status));
+
 	/* Flush input */
 	while (tstc())
 		getchar();
@@ -171,7 +242,7 @@ void eficonfig_print_msg(char *msg)
 	printf(ANSI_CURSOR_HIDE
 	       ANSI_CLEAR_CONSOLE
 	       ANSI_CURSOR_POSITION
-	       "%s\n\n  Press any key to continue", 3, 4, msg);
+	       "%s\n\n  Press any key to continue", 3, 4, str);
 
 	getchar();
 }
@@ -580,7 +651,7 @@ static efi_status_t eficonfig_file_selected(void *data)
 		new_len = u16_strlen(info->file_info->current_path) +
 				     strlen(info->file_name);
 		if (new_len >= EFICONFIG_FILE_PATH_MAX) {
-			eficonfig_print_msg("File path is too long!");
+			eficonfig_print_msg("File path is too long!", 0);
 			return EFI_INVALID_PARAMETER;
 		}
 		tmp = &info->file_info->current_path[u16_strlen(info->file_info->current_path)];
@@ -626,7 +697,7 @@ static efi_status_t eficonfig_select_volume(struct eficonfig_select_file_info *f
 	ret = efi_locate_handle_buffer_int(BY_PROTOCOL, &efi_simple_file_system_protocol_guid,
 					   NULL, &count, (efi_handle_t **)&volume_handles);
 	if (ret != EFI_SUCCESS) {
-		eficonfig_print_msg("No block device found!");
+		eficonfig_print_msg("No block device found!", ret);
 		return ret;
 	}
 
@@ -850,7 +921,7 @@ static efi_status_t eficonfig_show_file_selection(struct eficonfig_select_file_i
 		ret = EFI_CALL(root->open(root, &f, file_info->current_path,
 					  EFI_FILE_MODE_READ, 0));
 		if (ret != EFI_SUCCESS) {
-			eficonfig_print_msg("Reading volume failed!");
+			eficonfig_print_msg("Reading volume failed!", ret);
 			free(efi_menu);
 			ret = EFI_ABORTED;
 			goto out;
@@ -990,12 +1061,12 @@ static efi_status_t eficonfig_boot_edit_save(void *data)
 	struct eficonfig_boot_option *bo = data;
 
 	if (u16_strlen(bo->description) == 0) {
-		eficonfig_print_msg("Boot Description is empty!");
+		eficonfig_print_msg("Boot Description is empty!", 0);
 		bo->edit_completed = false;
 		return EFI_NOT_READY;
 	}
 	if (u16_strlen(bo->file_info.current_path) == 0) {
-		eficonfig_print_msg("File is not selected!");
+		eficonfig_print_msg("File is not selected!", 0);
 		bo->edit_completed = false;
 		return EFI_NOT_READY;
 	}
@@ -2658,7 +2729,7 @@ static efi_status_t eficonfig_init(void)
 		avail_row = rows - (EFICONFIG_MENU_HEADER_ROW_NUM +
 				    EFICONFIG_MENU_DESC_ROW_NUM);
 		if (avail_row <= 0) {
-			eficonfig_print_msg("Console size is too small!");
+			eficonfig_print_msg("Console size is too small!", 0);
 			return EFI_INVALID_PARAMETER;
 		}
 		/* TODO: Should we check the minimum column size? */
diff --git a/cmd/eficonfig_sbkey.c b/cmd/eficonfig_sbkey.c
index caca27495e..7ae1953567 100644
--- a/cmd/eficonfig_sbkey.c
+++ b/cmd/eficonfig_sbkey.c
@@ -150,7 +150,7 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
 	free(buf);
 
 	if (!size) {
-		eficonfig_print_msg("ERROR! File is empty.");
+		eficonfig_print_msg("ERROR! File is empty.", 0);
 		ret = EFI_INVALID_PARAMETER;
 		goto out;
 	}
@@ -163,11 +163,13 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
 
 	ret = EFI_CALL(f->read(f, &size, buf));
 	if (ret != EFI_SUCCESS) {
-		eficonfig_print_msg("ERROR! Failed to read file.");
+		eficonfig_print_msg("ERROR! Failed to read file.", ret);
 		goto out;
 	}
 	if (!file_have_auth_header(buf, size)) {
-		eficonfig_print_msg("ERROR! Invalid file format. Only .auth variables is allowed.");
+		eficonfig_print_msg(
+			"ERROR! Invalid file format. Only .auth variables is allowed.",
+			0);
 		ret = EFI_INVALID_PARAMETER;
 		goto out;
 	}
@@ -175,7 +177,7 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
 	ret = file_is_null_key((struct efi_variable_authentication_2 *)buf,
 			       size, &null_key);
 	if (ret != EFI_SUCCESS) {
-		eficonfig_print_msg("ERROR! Invalid file format.");
+		eficonfig_print_msg("ERROR! Invalid file format.", ret);
 		goto out;
 	}
 
@@ -202,7 +204,7 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
 	ret = efi_set_variable_int((u16 *)data, efi_auth_var_get_guid((u16 *)data),
 				   attr, size, buf, false);
 	if (ret != EFI_SUCCESS)
-		eficonfig_print_msg("ERROR! Failed to update signature database");
+		eficonfig_print_msg("ERROR! Failed to update signature database", ret);
 
 out:
 	free(file_info.current_path);
@@ -283,7 +285,7 @@ static efi_status_t eficonfig_process_show_siglist(void *data)
 				break;
 			}
 			default:
-				eficonfig_print_msg("ERROR! Unsupported format.");
+				eficonfig_print_msg("ERROR! Unsupported format.", 0);
 				return EFI_INVALID_PARAMETER;
 			}
 		}
@@ -394,7 +396,7 @@ static efi_status_t enumerate_and_show_signature_database(void *varname)
 
 	db = efi_get_var(varname, efi_auth_var_get_guid(varname), &db_size);
 	if (!db) {
-		eficonfig_print_msg("There is no entry in the signature database.");
+		eficonfig_print_msg("There is no entry in the signature database.", 0);
 		return EFI_NOT_FOUND;
 	}
 
diff --git a/include/efi_config.h b/include/efi_config.h
index 01ce9b2b06..19b1686907 100644
--- a/include/efi_config.h
+++ b/include/efi_config.h
@@ -93,7 +93,7 @@ struct eficonfig_select_file_info {
 	bool file_selected;
 };
 
-void eficonfig_print_msg(char *msg);
+void eficonfig_print_msg(const char *msg, efi_status_t status);
 void eficonfig_print_entry(void *data);
 void eficonfig_display_statusline(struct menu *m);
 char *eficonfig_choice_entry(void *data);
-- 
2.17.1

[PATCH 5/5] eficonfig: add error message of SetVariable

[Masahisa Kojima]

This commits add the error message when EFI Runtime Service
SetVariable() failed.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 cmd/eficonfig.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
index b0c8637676..c5cbf27631 100644
--- a/cmd/eficonfig.c
+++ b/cmd/eficonfig.c
@@ -30,6 +30,8 @@ static const char *eficonfig_change_boot_order_desc =
 	"  Press SPACE to activate or deactivate the entry\n"
 	"  CTRL+S to save, ESC to quit";
 
+static const char *set_variable_fail_str = "SetVariable failed!";
+
 static struct efi_simple_text_output_protocol *cout;
 static int avail_row;
 
@@ -1274,6 +1276,9 @@ static efi_status_t eficonfig_set_boot_option(u16 *varname, struct efi_device_pa
 				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
 				   EFI_VARIABLE_RUNTIME_ACCESS,
 				   size, p, false);
+	if (ret != EFI_SUCCESS)
+		eficonfig_print_msg(set_variable_fail_str, ret);
+
 	free(p);
 
 	return ret;
@@ -1309,8 +1314,10 @@ efi_status_t eficonfig_append_bootorder(u16 index)
 				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
 				   EFI_VARIABLE_RUNTIME_ACCESS,
 				   new_size, new_bootorder, false);
-	if (ret != EFI_SUCCESS)
+	if (ret != EFI_SUCCESS) {
+		eficonfig_print_msg(set_variable_fail_str, ret);
 		goto out;
+	}
 
 out:
 	free(bootorder);
@@ -2155,6 +2162,8 @@ static efi_status_t eficonfig_process_save_boot_order(void *data)
 				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
 				   EFI_VARIABLE_RUNTIME_ACCESS,
 				   size, new_bootorder, false);
+	if (ret != EFI_SUCCESS)
+		eficonfig_print_msg(set_variable_fail_str, ret);
 
 	save_data->selected = true;
 out:
@@ -2394,7 +2403,7 @@ static efi_status_t delete_boot_option(u16 boot_index)
 	ret = efi_set_variable_int(varname, &efi_global_variable_guid,
 				   0, 0, NULL, false);
 	if (ret != EFI_SUCCESS) {
-		log_err("delete boot option(%ls) failed\n", varname);
+		eficonfig_print_msg("Delete boot option(%ls) failed!", ret);
 		return ret;
 	}
 
@@ -2415,6 +2424,8 @@ static efi_status_t delete_boot_option(u16 boot_index)
 				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
 				   EFI_VARIABLE_RUNTIME_ACCESS,
 				   size, bootorder, false);
+	if (ret != EFI_SUCCESS)
+		eficonfig_print_msg(set_variable_fail_str, ret);
 
 	return ret;
 }
@@ -2672,13 +2683,18 @@ efi_status_t eficonfig_generate_media_device_boot_option(void)
 						   EFI_VARIABLE_BOOTSERVICE_ACCESS |
 						   EFI_VARIABLE_RUNTIME_ACCESS,
 						   opt[i].size, opt[i].lo, false);
-			if (ret != EFI_SUCCESS)
+			if (ret != EFI_SUCCESS) {
+				eficonfig_print_msg(set_variable_fail_str, ret);
 				goto out;
+			}
 
 			ret = eficonfig_append_bootorder(boot_index);
 			if (ret != EFI_SUCCESS) {
 				efi_set_variable_int(var_name, &efi_global_variable_guid,
 						     0, 0, NULL, false);
+				if (ret != EFI_SUCCESS)
+					eficonfig_print_msg(set_variable_fail_str, ret);
+
 				goto out;
 			}
 		}
-- 
2.17.1

Re: [PATCH 1/5] menu: remove CTRL+C to quit

[Heinrich Schuchardt]

On 2/2/23 10:24, Masahisa Kojima wrote:
> On the sandbox called without "--terminal raw" CTRL+C leaves U-Boot,
> "ESC/CTRL+C to quit" is misleading.
>
> Let's remove CTRL+C to quit key handling from bootmenu and eficonfig menu.

I guess my review was misleading.

CTRL+C on the sandbox leaves the u-boot program. Therefore advising its
use in the GUI is not a good idea. Reacting on the CTRL+C on other
systems does no harm.

I will just take the text changes and leave the code as is.

Best regards

Heinrich

>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
>   cmd/bootmenu.c               | 2 +-
>   cmd/eficonfig.c              | 6 +++---
>   common/menu.c                | 1 -
>   doc/usage/cmd/bootmenu.rst   | 2 +-
>   lib/efi_loader/efi_console.c | 2 +-
>   5 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
> index 3236ca5d79..8dc133c236 100644
> --- a/cmd/bootmenu.c
> +++ b/cmd/bootmenu.c
> @@ -437,7 +437,7 @@ static void menu_display_statusline(struct menu *m)
>   	printf(ANSI_CURSOR_POSITION, menu->count + 5, 1);
>   	puts(ANSI_CLEAR_LINE);
>   	printf(ANSI_CURSOR_POSITION, menu->count + 6, 3);
> -	puts("Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit");
> +	puts("Press UP/DOWN to move, ENTER to select, ESC to quit");
>   	puts(ANSI_CLEAR_LINE_TO_END);
>   	printf(ANSI_CURSOR_POSITION, menu->count + 7, 1);
>   	puts(ANSI_CLEAR_LINE);
> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> index 47c04cf536..f365a988d4 100644
> --- a/cmd/eficonfig.c
> +++ b/cmd/eficonfig.c
> @@ -23,12 +23,12 @@
>
>   static struct efi_simple_text_input_protocol *cin;
>   const char *eficonfig_menu_desc =
> -	"  Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit";
> +	"  Press UP/DOWN to move, ENTER to select, ESC to quit";
>
>   static const char *eficonfig_change_boot_order_desc =
>   	"  Press UP/DOWN to move, +/- to change orde\n"
>   	"  Press SPACE to activate or deactivate the entry\n"
> -	"  Select [Save] to complete, ESC/CTRL+C to quit";
> +	"  Select [Save] to complete, ESC to quit";
>
>   static struct efi_simple_text_output_protocol *cout;
>   static int avail_row;
> @@ -927,7 +927,7 @@ static efi_status_t handle_user_input(u16 *buf, int buf_size,
>   	       ANSI_CURSOR_POSITION
>   	       "%s"
>   	       ANSI_CURSOR_POSITION
> -	       "  Press ENTER to complete, ESC/CTRL+C to quit",
> +	       "  Press ENTER to complete, ESC to quit",
>   	       0, 1, msg, 8, 1);
>
>   	/* tmp is used to accept user cancel */
> diff --git a/common/menu.c b/common/menu.c
> index cdcdbb2a18..56401695de 100644
> --- a/common/menu.c
> +++ b/common/menu.c
> @@ -492,7 +492,6 @@ enum bootmenu_key bootmenu_conv_key(int ichar)
>   		/* enter key was pressed */
>   		key = BKEY_SELECT;
>   		break;
> -	case CTL_CH('c'):
>   	case '\e':
>   		/* ^C was pressed */
>   		key = BKEY_QUIT;
> diff --git a/doc/usage/cmd/bootmenu.rst b/doc/usage/cmd/bootmenu.rst
> index cb3c8d2f93..684a18d8e1 100644
> --- a/doc/usage/cmd/bootmenu.rst
> +++ b/doc/usage/cmd/bootmenu.rst
> @@ -122,7 +122,7 @@ Example bootmenu is as below::
>   Default behavior when user exits from the bootmenu
>   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   User can exit from bootmenu by selecting the last entry
> -"U-Boot console"/"Quit" or ESC/CTRL+C key.
> +"U-Boot console"/"Quit" or ESC key.
>
>   When the CONFIG_BOOTMENU_DISABLE_UBOOT_CONSOLE is disabled,
>   user exits from the bootmenu and returns to the U-Boot console.
> diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
> index 1ed8c7aa36..2c7536107a 100644
> --- a/lib/efi_loader/efi_console.c
> +++ b/lib/efi_loader/efi_console.c
> @@ -1395,7 +1395,7 @@ efi_status_t efi_console_get_u16_string(struct efi_simple_text_input_protocol *c
>   		} else if (key.unicode_char == u'\r') {
>   			buf[len] = u'\0';
>   			return EFI_SUCCESS;
> -		} else if (key.unicode_char == 0x3 || key.scan_code == 23) {
> +		} else if (key.scan_code == 23) {
>   			return EFI_ABORTED;
>   		} else if (key.unicode_char < 0x20) {
>   			/* ignore control codes other than Ctrl+C, '\r' and '\b' */

Re: [PATCH 4/5] eficonfig: include EFI_STATUS string in error message

[Heinrich Schuchardt]

On 2/2/23 10:24, Masahisa Kojima wrote:
> Current eficonfig_print_msg() does not show the return
> value of EFI Boot/Runtime Services when the service call fails.
> With this commit, user can know EFI_STATUS in the error message.

Why do we need function eficonfig_print_msg()?

I cannot see why the printing only parameter msg with log_err() should
not be good enough.

Best regards

Heinrich

>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
>   cmd/eficonfig.c       | 95 +++++++++++++++++++++++++++++++++++++------
>   cmd/eficonfig_sbkey.c | 16 ++++----
>   include/efi_config.h  |  2 +-
>   3 files changed, 93 insertions(+), 20 deletions(-)
>
> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> index 0a17b8cf34..b0c8637676 100644
> --- a/cmd/eficonfig.c
> +++ b/cmd/eficonfig.c
> @@ -151,19 +151,90 @@ static void eficonfig_menu_adjust(struct efimenu *efi_menu, bool add)
>   #define eficonfig_menu_up(_a) eficonfig_menu_adjust(_a, false)
>   #define eficonfig_menu_down(_a) eficonfig_menu_adjust(_a, true)
>
> +struct efi_status_str {
> +	efi_status_t status;
> +	char *str;
> +};
> +
> +static const struct efi_status_str status_str_table[] = {
> +	{EFI_LOAD_ERROR,		"Load Error"},
> +	{EFI_INVALID_PARAMETER,		"Invalid Parameter"},
> +	{EFI_UNSUPPORTED,		"Unsupported"},
> +	{EFI_BAD_BUFFER_SIZE,		"Bad Buffer Size"},
> +	{EFI_BUFFER_TOO_SMALL,		"Buffer Too Small"},
> +	{EFI_NOT_READY,			"Not Ready"},
> +	{EFI_DEVICE_ERROR,		"Device Error"},
> +	{EFI_WRITE_PROTECTED,		"Write Protected"},
> +	{EFI_OUT_OF_RESOURCES,		"Out of Resources"},
> +	{EFI_VOLUME_CORRUPTED,		"Volume Corrupted"},
> +	{EFI_VOLUME_FULL,		"Volume Full"},
> +	{EFI_NO_MEDIA,			"No Media"},
> +	{EFI_MEDIA_CHANGED,		"Media Changed"},
> +	{EFI_NOT_FOUND,			"Not Found"},
> +	{EFI_ACCESS_DENIED,		"Access Denied"},
> +	{EFI_NO_RESPONSE,		"No Response"},
> +	{EFI_NO_MAPPING,		"No Mapping"},
> +	{EFI_TIMEOUT,			"Timeout"},
> +	{EFI_NOT_STARTED,		"Not Started"},
> +	{EFI_ALREADY_STARTED,		"Already Started"},
> +	{EFI_ABORTED,			"Aborted"},
> +	{EFI_ICMP_ERROR,		"ICMP Error"},
> +	{EFI_TFTP_ERROR,		"TFTP Error"},
> +	{EFI_PROTOCOL_ERROR,		"Protocol Error"},
> +	{EFI_INCOMPATIBLE_VERSION,	"Incompatible Version"},
> +	{EFI_SECURITY_VIOLATION,	"Security Violation"},
> +	{EFI_CRC_ERROR,			"CRC Error"},
> +	{EFI_END_OF_MEDIA,		"End of Media"},
> +	{EFI_END_OF_FILE,		"End of File"},
> +	{EFI_INVALID_LANGUAGE,		"Invalid Language"},
> +	{EFI_COMPROMISED_DATA,		"Compromised Data"},
> +	{EFI_IP_ADDRESS_CONFLICT,	"IP Address Conflict"},
> +	{EFI_HTTP_ERROR,		"HTTP Error"},
> +	{EFI_WARN_UNKNOWN_GLYPH,	"Warn Unknown Glyph"},
> +	{EFI_WARN_DELETE_FAILURE,	"Warn Delete Failure"},
> +	{EFI_WARN_WRITE_FAILURE,	"Warn Write Failure"},
> +	{EFI_WARN_BUFFER_TOO_SMALL,	"Warn Buffer Too Small"},
> +	{EFI_WARN_STALE_DATA,		"Warn Stale Data"},
> +	{EFI_WARN_FILE_SYSTEM,		"Warn File System"},
> +	{EFI_WARN_RESET_REQUIRED,	"Warn Reset Required"},
> +	{0, ""},
> +};
> +
> +/**
> + * struct get_status_str - get status string
> + *
> + * @status:	efi_status_t value to covert to string
> + * Return:	pointer to the string
> + */
> +static char *get_error_str(efi_status_t status)
> +{
> +	u32 i;
> +
> +	for (i = 0; status_str_table[i].status != 0; i++) {
> +		if (status == status_str_table[i].status)
> +			return status_str_table[i].str;
> +	}
> +	return status_str_table[i].str;
> +}
> +
>   /**
>    * eficonfig_print_msg() - print message
>    *
>    * display the message to the user, user proceeds the screen
>    * with any key press.
>    *
> - * @items:		pointer to the structure of each menu entry
> - * @count:		the number of menu entry
> - * @menu_header:	pointer to the menu header string
> - * Return:	status code
> + * @msg:	pointer to the error message
> + * @status:	efi status code, set 0 if no status string output
>    */
> -void eficonfig_print_msg(char *msg)
> +void eficonfig_print_msg(const char *msg, efi_status_t status)
>   {
> +	char str[128];
> +
> +	if (status == 0)
> +		snprintf(str, sizeof(str), "%s", msg);
> +	else
> +		snprintf(str, sizeof(str), "%s (%s)", msg, get_error_str(status));
> +
>   	/* Flush input */
>   	while (tstc())
>   		getchar();
> @@ -171,7 +242,7 @@ void eficonfig_print_msg(char *msg)
>   	printf(ANSI_CURSOR_HIDE
>   	       ANSI_CLEAR_CONSOLE
>   	       ANSI_CURSOR_POSITION
> -	       "%s\n\n  Press any key to continue", 3, 4, msg);
> +	       "%s\n\n  Press any key to continue", 3, 4, str);
>
>   	getchar();
>   }
> @@ -580,7 +651,7 @@ static efi_status_t eficonfig_file_selected(void *data)
>   		new_len = u16_strlen(info->file_info->current_path) +
>   				     strlen(info->file_name);
>   		if (new_len >= EFICONFIG_FILE_PATH_MAX) {
> -			eficonfig_print_msg("File path is too long!");
> +			eficonfig_print_msg("File path is too long!", 0);
>   			return EFI_INVALID_PARAMETER;
>   		}
>   		tmp = &info->file_info->current_path[u16_strlen(info->file_info->current_path)];
> @@ -626,7 +697,7 @@ static efi_status_t eficonfig_select_volume(struct eficonfig_select_file_info *f
>   	ret = efi_locate_handle_buffer_int(BY_PROTOCOL, &efi_simple_file_system_protocol_guid,
>   					   NULL, &count, (efi_handle_t **)&volume_handles);
>   	if (ret != EFI_SUCCESS) {
> -		eficonfig_print_msg("No block device found!");
> +		eficonfig_print_msg("No block device found!", ret);
>   		return ret;
>   	}
>
> @@ -850,7 +921,7 @@ static efi_status_t eficonfig_show_file_selection(struct eficonfig_select_file_i
>   		ret = EFI_CALL(root->open(root, &f, file_info->current_path,
>   					  EFI_FILE_MODE_READ, 0));
>   		if (ret != EFI_SUCCESS) {
> -			eficonfig_print_msg("Reading volume failed!");
> +			eficonfig_print_msg("Reading volume failed!", ret);
>   			free(efi_menu);
>   			ret = EFI_ABORTED;
>   			goto out;
> @@ -990,12 +1061,12 @@ static efi_status_t eficonfig_boot_edit_save(void *data)
>   	struct eficonfig_boot_option *bo = data;
>
>   	if (u16_strlen(bo->description) == 0) {
> -		eficonfig_print_msg("Boot Description is empty!");
> +		eficonfig_print_msg("Boot Description is empty!", 0);
>   		bo->edit_completed = false;
>   		return EFI_NOT_READY;
>   	}
>   	if (u16_strlen(bo->file_info.current_path) == 0) {
> -		eficonfig_print_msg("File is not selected!");
> +		eficonfig_print_msg("File is not selected!", 0);
>   		bo->edit_completed = false;
>   		return EFI_NOT_READY;
>   	}
> @@ -2658,7 +2729,7 @@ static efi_status_t eficonfig_init(void)
>   		avail_row = rows - (EFICONFIG_MENU_HEADER_ROW_NUM +
>   				    EFICONFIG_MENU_DESC_ROW_NUM);
>   		if (avail_row <= 0) {
> -			eficonfig_print_msg("Console size is too small!");
> +			eficonfig_print_msg("Console size is too small!", 0);
>   			return EFI_INVALID_PARAMETER;
>   		}
>   		/* TODO: Should we check the minimum column size? */
> diff --git a/cmd/eficonfig_sbkey.c b/cmd/eficonfig_sbkey.c
> index caca27495e..7ae1953567 100644
> --- a/cmd/eficonfig_sbkey.c
> +++ b/cmd/eficonfig_sbkey.c
> @@ -150,7 +150,7 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
>   	free(buf);
>
>   	if (!size) {
> -		eficonfig_print_msg("ERROR! File is empty.");
> +		eficonfig_print_msg("ERROR! File is empty.", 0);
>   		ret = EFI_INVALID_PARAMETER;
>   		goto out;
>   	}
> @@ -163,11 +163,13 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
>
>   	ret = EFI_CALL(f->read(f, &size, buf));
>   	if (ret != EFI_SUCCESS) {
> -		eficonfig_print_msg("ERROR! Failed to read file.");
> +		eficonfig_print_msg("ERROR! Failed to read file.", ret);
>   		goto out;
>   	}
>   	if (!file_have_auth_header(buf, size)) {
> -		eficonfig_print_msg("ERROR! Invalid file format. Only .auth variables is allowed.");
> +		eficonfig_print_msg(
> +			"ERROR! Invalid file format. Only .auth variables is allowed.",
> +			0);
>   		ret = EFI_INVALID_PARAMETER;
>   		goto out;
>   	}
> @@ -175,7 +177,7 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
>   	ret = file_is_null_key((struct efi_variable_authentication_2 *)buf,
>   			       size, &null_key);
>   	if (ret != EFI_SUCCESS) {
> -		eficonfig_print_msg("ERROR! Invalid file format.");
> +		eficonfig_print_msg("ERROR! Invalid file format.", ret);
>   		goto out;
>   	}
>
> @@ -202,7 +204,7 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
>   	ret = efi_set_variable_int((u16 *)data, efi_auth_var_get_guid((u16 *)data),
>   				   attr, size, buf, false);
>   	if (ret != EFI_SUCCESS)
> -		eficonfig_print_msg("ERROR! Failed to update signature database");
> +		eficonfig_print_msg("ERROR! Failed to update signature database", ret);
>
>   out:
>   	free(file_info.current_path);
> @@ -283,7 +285,7 @@ static efi_status_t eficonfig_process_show_siglist(void *data)
>   				break;
>   			}
>   			default:
> -				eficonfig_print_msg("ERROR! Unsupported format.");
> +				eficonfig_print_msg("ERROR! Unsupported format.", 0);
>   				return EFI_INVALID_PARAMETER;
>   			}
>   		}
> @@ -394,7 +396,7 @@ static efi_status_t enumerate_and_show_signature_database(void *varname)
>
>   	db = efi_get_var(varname, efi_auth_var_get_guid(varname), &db_size);
>   	if (!db) {
> -		eficonfig_print_msg("There is no entry in the signature database.");
> +		eficonfig_print_msg("There is no entry in the signature database.", 0);
>   		return EFI_NOT_FOUND;
>   	}
>
> diff --git a/include/efi_config.h b/include/efi_config.h
> index 01ce9b2b06..19b1686907 100644
> --- a/include/efi_config.h
> +++ b/include/efi_config.h
> @@ -93,7 +93,7 @@ struct eficonfig_select_file_info {
>   	bool file_selected;
>   };
>
> -void eficonfig_print_msg(char *msg);
> +void eficonfig_print_msg(const char *msg, efi_status_t status);
>   void eficonfig_print_entry(void *data);
>   void eficonfig_display_statusline(struct menu *m);
>   char *eficonfig_choice_entry(void *data);

Re: [PATCH 1/5] menu: remove CTRL+C to quit

[Masahisa Kojima]

On Fri, 10 Feb 2023 at 20:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 2/2/23 10:24, Masahisa Kojima wrote:
> > On the sandbox called without "--terminal raw" CTRL+C leaves U-Boot,
> > "ESC/CTRL+C to quit" is misleading.
> >
> > Let's remove CTRL+C to quit key handling from bootmenu and eficonfig menu.
>
> I guess my review was misleading.
>
> CTRL+C on the sandbox leaves the u-boot program. Therefore advising its
> use in the GUI is not a good idea. Reacting on the CTRL+C on other
> systems does no harm.
>
> I will just take the text changes and leave the code as is.

I misunderstand your review comment.
Thank you for the update.

Regards,
Masahisa Kojima

>
> Best regards
>
> Heinrich
>
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> >   cmd/bootmenu.c               | 2 +-
> >   cmd/eficonfig.c              | 6 +++---
> >   common/menu.c                | 1 -
> >   doc/usage/cmd/bootmenu.rst   | 2 +-
> >   lib/efi_loader/efi_console.c | 2 +-
> >   5 files changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
> > index 3236ca5d79..8dc133c236 100644
> > --- a/cmd/bootmenu.c
> > +++ b/cmd/bootmenu.c
> > @@ -437,7 +437,7 @@ static void menu_display_statusline(struct menu *m)
> >       printf(ANSI_CURSOR_POSITION, menu->count + 5, 1);
> >       puts(ANSI_CLEAR_LINE);
> >       printf(ANSI_CURSOR_POSITION, menu->count + 6, 3);
> > -     puts("Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit");
> > +     puts("Press UP/DOWN to move, ENTER to select, ESC to quit");
> >       puts(ANSI_CLEAR_LINE_TO_END);
> >       printf(ANSI_CURSOR_POSITION, menu->count + 7, 1);
> >       puts(ANSI_CLEAR_LINE);
> > diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> > index 47c04cf536..f365a988d4 100644
> > --- a/cmd/eficonfig.c
> > +++ b/cmd/eficonfig.c
> > @@ -23,12 +23,12 @@
> >
> >   static struct efi_simple_text_input_protocol *cin;
> >   const char *eficonfig_menu_desc =
> > -     "  Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit";
> > +     "  Press UP/DOWN to move, ENTER to select, ESC to quit";
> >
> >   static const char *eficonfig_change_boot_order_desc =
> >       "  Press UP/DOWN to move, +/- to change orde\n"
> >       "  Press SPACE to activate or deactivate the entry\n"
> > -     "  Select [Save] to complete, ESC/CTRL+C to quit";
> > +     "  Select [Save] to complete, ESC to quit";
> >
> >   static struct efi_simple_text_output_protocol *cout;
> >   static int avail_row;
> > @@ -927,7 +927,7 @@ static efi_status_t handle_user_input(u16 *buf, int buf_size,
> >              ANSI_CURSOR_POSITION
> >              "%s"
> >              ANSI_CURSOR_POSITION
> > -            "  Press ENTER to complete, ESC/CTRL+C to quit",
> > +            "  Press ENTER to complete, ESC to quit",
> >              0, 1, msg, 8, 1);
> >
> >       /* tmp is used to accept user cancel */
> > diff --git a/common/menu.c b/common/menu.c
> > index cdcdbb2a18..56401695de 100644
> > --- a/common/menu.c
> > +++ b/common/menu.c
> > @@ -492,7 +492,6 @@ enum bootmenu_key bootmenu_conv_key(int ichar)
> >               /* enter key was pressed */
> >               key = BKEY_SELECT;
> >               break;
> > -     case CTL_CH('c'):
> >       case '\e':
> >               /* ^C was pressed */
> >               key = BKEY_QUIT;
> > diff --git a/doc/usage/cmd/bootmenu.rst b/doc/usage/cmd/bootmenu.rst
> > index cb3c8d2f93..684a18d8e1 100644
> > --- a/doc/usage/cmd/bootmenu.rst
> > +++ b/doc/usage/cmd/bootmenu.rst
> > @@ -122,7 +122,7 @@ Example bootmenu is as below::
> >   Default behavior when user exits from the bootmenu
> >   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >   User can exit from bootmenu by selecting the last entry
> > -"U-Boot console"/"Quit" or ESC/CTRL+C key.
> > +"U-Boot console"/"Quit" or ESC key.
> >
> >   When the CONFIG_BOOTMENU_DISABLE_UBOOT_CONSOLE is disabled,
> >   user exits from the bootmenu and returns to the U-Boot console.
> > diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
> > index 1ed8c7aa36..2c7536107a 100644
> > --- a/lib/efi_loader/efi_console.c
> > +++ b/lib/efi_loader/efi_console.c
> > @@ -1395,7 +1395,7 @@ efi_status_t efi_console_get_u16_string(struct efi_simple_text_input_protocol *c
> >               } else if (key.unicode_char == u'\r') {
> >                       buf[len] = u'\0';
> >                       return EFI_SUCCESS;
> > -             } else if (key.unicode_char == 0x3 || key.scan_code == 23) {
> > +             } else if (key.scan_code == 23) {
> >                       return EFI_ABORTED;
> >               } else if (key.unicode_char < 0x20) {
> >                       /* ignore control codes other than Ctrl+C, '\r' and '\b' */

Re: [PATCH 4/5] eficonfig: include EFI_STATUS string in error message

[Masahisa Kojima]

On Fri, 10 Feb 2023 at 20:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 2/2/23 10:24, Masahisa Kojima wrote:
> > Current eficonfig_print_msg() does not show the return
> > value of EFI Boot/Runtime Services when the service call fails.
> > With this commit, user can know EFI_STATUS in the error message.
>
> Why do we need function eficonfig_print_msg()?
>
> I cannot see why the printing only parameter msg with log_err() should
> not be good enough.

ANSI_CLEAR_CONSOLE is sent before drawing the menu. I think it is
difficult for user
to know some error occurs by the user operation, user needs scroll up
to see the error message
when we use log_err().

Regards,
Masahisa Kojima

>
> Best regards
>
> Heinrich
>
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> >   cmd/eficonfig.c       | 95 +++++++++++++++++++++++++++++++++++++------
> >   cmd/eficonfig_sbkey.c | 16 ++++----
> >   include/efi_config.h  |  2 +-
> >   3 files changed, 93 insertions(+), 20 deletions(-)
> >
> > diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> > index 0a17b8cf34..b0c8637676 100644
> > --- a/cmd/eficonfig.c
> > +++ b/cmd/eficonfig.c
> > @@ -151,19 +151,90 @@ static void eficonfig_menu_adjust(struct efimenu *efi_menu, bool add)
> >   #define eficonfig_menu_up(_a) eficonfig_menu_adjust(_a, false)
> >   #define eficonfig_menu_down(_a) eficonfig_menu_adjust(_a, true)
> >
> > +struct efi_status_str {
> > +     efi_status_t status;
> > +     char *str;
> > +};
> > +
> > +static const struct efi_status_str status_str_table[] = {
> > +     {EFI_LOAD_ERROR,                "Load Error"},
> > +     {EFI_INVALID_PARAMETER,         "Invalid Parameter"},
> > +     {EFI_UNSUPPORTED,               "Unsupported"},
> > +     {EFI_BAD_BUFFER_SIZE,           "Bad Buffer Size"},
> > +     {EFI_BUFFER_TOO_SMALL,          "Buffer Too Small"},
> > +     {EFI_NOT_READY,                 "Not Ready"},
> > +     {EFI_DEVICE_ERROR,              "Device Error"},
> > +     {EFI_WRITE_PROTECTED,           "Write Protected"},
> > +     {EFI_OUT_OF_RESOURCES,          "Out of Resources"},
> > +     {EFI_VOLUME_CORRUPTED,          "Volume Corrupted"},
> > +     {EFI_VOLUME_FULL,               "Volume Full"},
> > +     {EFI_NO_MEDIA,                  "No Media"},
> > +     {EFI_MEDIA_CHANGED,             "Media Changed"},
> > +     {EFI_NOT_FOUND,                 "Not Found"},
> > +     {EFI_ACCESS_DENIED,             "Access Denied"},
> > +     {EFI_NO_RESPONSE,               "No Response"},
> > +     {EFI_NO_MAPPING,                "No Mapping"},
> > +     {EFI_TIMEOUT,                   "Timeout"},
> > +     {EFI_NOT_STARTED,               "Not Started"},
> > +     {EFI_ALREADY_STARTED,           "Already Started"},
> > +     {EFI_ABORTED,                   "Aborted"},
> > +     {EFI_ICMP_ERROR,                "ICMP Error"},
> > +     {EFI_TFTP_ERROR,                "TFTP Error"},
> > +     {EFI_PROTOCOL_ERROR,            "Protocol Error"},
> > +     {EFI_INCOMPATIBLE_VERSION,      "Incompatible Version"},
> > +     {EFI_SECURITY_VIOLATION,        "Security Violation"},
> > +     {EFI_CRC_ERROR,                 "CRC Error"},
> > +     {EFI_END_OF_MEDIA,              "End of Media"},
> > +     {EFI_END_OF_FILE,               "End of File"},
> > +     {EFI_INVALID_LANGUAGE,          "Invalid Language"},
> > +     {EFI_COMPROMISED_DATA,          "Compromised Data"},
> > +     {EFI_IP_ADDRESS_CONFLICT,       "IP Address Conflict"},
> > +     {EFI_HTTP_ERROR,                "HTTP Error"},
> > +     {EFI_WARN_UNKNOWN_GLYPH,        "Warn Unknown Glyph"},
> > +     {EFI_WARN_DELETE_FAILURE,       "Warn Delete Failure"},
> > +     {EFI_WARN_WRITE_FAILURE,        "Warn Write Failure"},
> > +     {EFI_WARN_BUFFER_TOO_SMALL,     "Warn Buffer Too Small"},
> > +     {EFI_WARN_STALE_DATA,           "Warn Stale Data"},
> > +     {EFI_WARN_FILE_SYSTEM,          "Warn File System"},
> > +     {EFI_WARN_RESET_REQUIRED,       "Warn Reset Required"},
> > +     {0, ""},
> > +};
> > +
> > +/**
> > + * struct get_status_str - get status string
> > + *
> > + * @status:  efi_status_t value to covert to string
> > + * Return:   pointer to the string
> > + */
> > +static char *get_error_str(efi_status_t status)
> > +{
> > +     u32 i;
> > +
> > +     for (i = 0; status_str_table[i].status != 0; i++) {
> > +             if (status == status_str_table[i].status)
> > +                     return status_str_table[i].str;
> > +     }
> > +     return status_str_table[i].str;
> > +}
> > +
> >   /**
> >    * eficonfig_print_msg() - print message
> >    *
> >    * display the message to the user, user proceeds the screen
> >    * with any key press.
> >    *
> > - * @items:           pointer to the structure of each menu entry
> > - * @count:           the number of menu entry
> > - * @menu_header:     pointer to the menu header string
> > - * Return:   status code
> > + * @msg:     pointer to the error message
> > + * @status:  efi status code, set 0 if no status string output
> >    */
> > -void eficonfig_print_msg(char *msg)
> > +void eficonfig_print_msg(const char *msg, efi_status_t status)
> >   {
> > +     char str[128];
> > +
> > +     if (status == 0)
> > +             snprintf(str, sizeof(str), "%s", msg);
> > +     else
> > +             snprintf(str, sizeof(str), "%s (%s)", msg, get_error_str(status));
> > +
> >       /* Flush input */
> >       while (tstc())
> >               getchar();
> > @@ -171,7 +242,7 @@ void eficonfig_print_msg(char *msg)
> >       printf(ANSI_CURSOR_HIDE
> >              ANSI_CLEAR_CONSOLE
> >              ANSI_CURSOR_POSITION
> > -            "%s\n\n  Press any key to continue", 3, 4, msg);
> > +            "%s\n\n  Press any key to continue", 3, 4, str);
> >
> >       getchar();
> >   }
> > @@ -580,7 +651,7 @@ static efi_status_t eficonfig_file_selected(void *data)
> >               new_len = u16_strlen(info->file_info->current_path) +
> >                                    strlen(info->file_name);
> >               if (new_len >= EFICONFIG_FILE_PATH_MAX) {
> > -                     eficonfig_print_msg("File path is too long!");
> > +                     eficonfig_print_msg("File path is too long!", 0);
> >                       return EFI_INVALID_PARAMETER;
> >               }
> >               tmp = &info->file_info->current_path[u16_strlen(info->file_info->current_path)];
> > @@ -626,7 +697,7 @@ static efi_status_t eficonfig_select_volume(struct eficonfig_select_file_info *f
> >       ret = efi_locate_handle_buffer_int(BY_PROTOCOL, &efi_simple_file_system_protocol_guid,
> >                                          NULL, &count, (efi_handle_t **)&volume_handles);
> >       if (ret != EFI_SUCCESS) {
> > -             eficonfig_print_msg("No block device found!");
> > +             eficonfig_print_msg("No block device found!", ret);
> >               return ret;
> >       }
> >
> > @@ -850,7 +921,7 @@ static efi_status_t eficonfig_show_file_selection(struct eficonfig_select_file_i
> >               ret = EFI_CALL(root->open(root, &f, file_info->current_path,
> >                                         EFI_FILE_MODE_READ, 0));
> >               if (ret != EFI_SUCCESS) {
> > -                     eficonfig_print_msg("Reading volume failed!");
> > +                     eficonfig_print_msg("Reading volume failed!", ret);
> >                       free(efi_menu);
> >                       ret = EFI_ABORTED;
> >                       goto out;
> > @@ -990,12 +1061,12 @@ static efi_status_t eficonfig_boot_edit_save(void *data)
> >       struct eficonfig_boot_option *bo = data;
> >
> >       if (u16_strlen(bo->description) == 0) {
> > -             eficonfig_print_msg("Boot Description is empty!");
> > +             eficonfig_print_msg("Boot Description is empty!", 0);
> >               bo->edit_completed = false;
> >               return EFI_NOT_READY;
> >       }
> >       if (u16_strlen(bo->file_info.current_path) == 0) {
> > -             eficonfig_print_msg("File is not selected!");
> > +             eficonfig_print_msg("File is not selected!", 0);
> >               bo->edit_completed = false;
> >               return EFI_NOT_READY;
> >       }
> > @@ -2658,7 +2729,7 @@ static efi_status_t eficonfig_init(void)
> >               avail_row = rows - (EFICONFIG_MENU_HEADER_ROW_NUM +
> >                                   EFICONFIG_MENU_DESC_ROW_NUM);
> >               if (avail_row <= 0) {
> > -                     eficonfig_print_msg("Console size is too small!");
> > +                     eficonfig_print_msg("Console size is too small!", 0);
> >                       return EFI_INVALID_PARAMETER;
> >               }
> >               /* TODO: Should we check the minimum column size? */
> > diff --git a/cmd/eficonfig_sbkey.c b/cmd/eficonfig_sbkey.c
> > index caca27495e..7ae1953567 100644
> > --- a/cmd/eficonfig_sbkey.c
> > +++ b/cmd/eficonfig_sbkey.c
> > @@ -150,7 +150,7 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
> >       free(buf);
> >
> >       if (!size) {
> > -             eficonfig_print_msg("ERROR! File is empty.");
> > +             eficonfig_print_msg("ERROR! File is empty.", 0);
> >               ret = EFI_INVALID_PARAMETER;
> >               goto out;
> >       }
> > @@ -163,11 +163,13 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
> >
> >       ret = EFI_CALL(f->read(f, &size, buf));
> >       if (ret != EFI_SUCCESS) {
> > -             eficonfig_print_msg("ERROR! Failed to read file.");
> > +             eficonfig_print_msg("ERROR! Failed to read file.", ret);
> >               goto out;
> >       }
> >       if (!file_have_auth_header(buf, size)) {
> > -             eficonfig_print_msg("ERROR! Invalid file format. Only .auth variables is allowed.");
> > +             eficonfig_print_msg(
> > +                     "ERROR! Invalid file format. Only .auth variables is allowed.",
> > +                     0);
> >               ret = EFI_INVALID_PARAMETER;
> >               goto out;
> >       }
> > @@ -175,7 +177,7 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
> >       ret = file_is_null_key((struct efi_variable_authentication_2 *)buf,
> >                              size, &null_key);
> >       if (ret != EFI_SUCCESS) {
> > -             eficonfig_print_msg("ERROR! Invalid file format.");
> > +             eficonfig_print_msg("ERROR! Invalid file format.", ret);
> >               goto out;
> >       }
> >
> > @@ -202,7 +204,7 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
> >       ret = efi_set_variable_int((u16 *)data, efi_auth_var_get_guid((u16 *)data),
> >                                  attr, size, buf, false);
> >       if (ret != EFI_SUCCESS)
> > -             eficonfig_print_msg("ERROR! Failed to update signature database");
> > +             eficonfig_print_msg("ERROR! Failed to update signature database", ret);
> >
> >   out:
> >       free(file_info.current_path);
> > @@ -283,7 +285,7 @@ static efi_status_t eficonfig_process_show_siglist(void *data)
> >                               break;
> >                       }
> >                       default:
> > -                             eficonfig_print_msg("ERROR! Unsupported format.");
> > +                             eficonfig_print_msg("ERROR! Unsupported format.", 0);
> >                               return EFI_INVALID_PARAMETER;
> >                       }
> >               }
> > @@ -394,7 +396,7 @@ static efi_status_t enumerate_and_show_signature_database(void *varname)
> >
> >       db = efi_get_var(varname, efi_auth_var_get_guid(varname), &db_size);
> >       if (!db) {
> > -             eficonfig_print_msg("There is no entry in the signature database.");
> > +             eficonfig_print_msg("There is no entry in the signature database.", 0);
> >               return EFI_NOT_FOUND;
> >       }
> >
> > diff --git a/include/efi_config.h b/include/efi_config.h
> > index 01ce9b2b06..19b1686907 100644
> > --- a/include/efi_config.h
> > +++ b/include/efi_config.h
> > @@ -93,7 +93,7 @@ struct eficonfig_select_file_info {
> >       bool file_selected;
> >   };
> >
> > -void eficonfig_print_msg(char *msg);
> > +void eficonfig_print_msg(const char *msg, efi_status_t status);
> >   void eficonfig_print_entry(void *data);
> >   void eficonfig_display_statusline(struct menu *m);
> >   char *eficonfig_choice_entry(void *data);
>

Re: [PATCH 4/5] eficonfig: include EFI_STATUS string in error message

[Heinrich Schuchardt]

On 2/13/23 06:50, Masahisa Kojima wrote:
> On Fri, 10 Feb 2023 at 20:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 2/2/23 10:24, Masahisa Kojima wrote:
>>> Current eficonfig_print_msg() does not show the return
>>> value of EFI Boot/Runtime Services when the service call fails.
>>> With this commit, user can know EFI_STATUS in the error message.
>>
>> Why do we need function eficonfig_print_msg()?
>>
>> I cannot see why the printing only parameter msg with log_err() should
>> not be good enough.
>
> ANSI_CLEAR_CONSOLE is sent before drawing the menu. I think it is
> difficult for user
> to know some error occurs by the user operation, user needs scroll up
> to see the error message
> when we use log_err(
Understood. But why do we need the status value (or with this patch the
long text for the status value)?

Best regards

Heinrich

>
> Regards,
> Masahisa Kojima
>
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>>> ---
>>>    cmd/eficonfig.c       | 95 +++++++++++++++++++++++++++++++++++++------
>>>    cmd/eficonfig_sbkey.c | 16 ++++----
>>>    include/efi_config.h  |  2 +-
>>>    3 files changed, 93 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
>>> index 0a17b8cf34..b0c8637676 100644
>>> --- a/cmd/eficonfig.c
>>> +++ b/cmd/eficonfig.c
>>> @@ -151,19 +151,90 @@ static void eficonfig_menu_adjust(struct efimenu *efi_menu, bool add)
>>>    #define eficonfig_menu_up(_a) eficonfig_menu_adjust(_a, false)
>>>    #define eficonfig_menu_down(_a) eficonfig_menu_adjust(_a, true)
>>>
>>> +struct efi_status_str {
>>> +     efi_status_t status;
>>> +     char *str;
>>> +};
>>> +
>>> +static const struct efi_status_str status_str_table[] = {
>>> +     {EFI_LOAD_ERROR,                "Load Error"},
>>> +     {EFI_INVALID_PARAMETER,         "Invalid Parameter"},
>>> +     {EFI_UNSUPPORTED,               "Unsupported"},
>>> +     {EFI_BAD_BUFFER_SIZE,           "Bad Buffer Size"},
>>> +     {EFI_BUFFER_TOO_SMALL,          "Buffer Too Small"},
>>> +     {EFI_NOT_READY,                 "Not Ready"},
>>> +     {EFI_DEVICE_ERROR,              "Device Error"},
>>> +     {EFI_WRITE_PROTECTED,           "Write Protected"},
>>> +     {EFI_OUT_OF_RESOURCES,          "Out of Resources"},
>>> +     {EFI_VOLUME_CORRUPTED,          "Volume Corrupted"},
>>> +     {EFI_VOLUME_FULL,               "Volume Full"},
>>> +     {EFI_NO_MEDIA,                  "No Media"},
>>> +     {EFI_MEDIA_CHANGED,             "Media Changed"},
>>> +     {EFI_NOT_FOUND,                 "Not Found"},
>>> +     {EFI_ACCESS_DENIED,             "Access Denied"},
>>> +     {EFI_NO_RESPONSE,               "No Response"},
>>> +     {EFI_NO_MAPPING,                "No Mapping"},
>>> +     {EFI_TIMEOUT,                   "Timeout"},
>>> +     {EFI_NOT_STARTED,               "Not Started"},
>>> +     {EFI_ALREADY_STARTED,           "Already Started"},
>>> +     {EFI_ABORTED,                   "Aborted"},
>>> +     {EFI_ICMP_ERROR,                "ICMP Error"},
>>> +     {EFI_TFTP_ERROR,                "TFTP Error"},
>>> +     {EFI_PROTOCOL_ERROR,            "Protocol Error"},
>>> +     {EFI_INCOMPATIBLE_VERSION,      "Incompatible Version"},
>>> +     {EFI_SECURITY_VIOLATION,        "Security Violation"},
>>> +     {EFI_CRC_ERROR,                 "CRC Error"},
>>> +     {EFI_END_OF_MEDIA,              "End of Media"},
>>> +     {EFI_END_OF_FILE,               "End of File"},
>>> +     {EFI_INVALID_LANGUAGE,          "Invalid Language"},
>>> +     {EFI_COMPROMISED_DATA,          "Compromised Data"},
>>> +     {EFI_IP_ADDRESS_CONFLICT,       "IP Address Conflict"},
>>> +     {EFI_HTTP_ERROR,                "HTTP Error"},
>>> +     {EFI_WARN_UNKNOWN_GLYPH,        "Warn Unknown Glyph"},
>>> +     {EFI_WARN_DELETE_FAILURE,       "Warn Delete Failure"},
>>> +     {EFI_WARN_WRITE_FAILURE,        "Warn Write Failure"},
>>> +     {EFI_WARN_BUFFER_TOO_SMALL,     "Warn Buffer Too Small"},
>>> +     {EFI_WARN_STALE_DATA,           "Warn Stale Data"},
>>> +     {EFI_WARN_FILE_SYSTEM,          "Warn File System"},
>>> +     {EFI_WARN_RESET_REQUIRED,       "Warn Reset Required"},
>>> +     {0, ""},
>>> +};
>>> +
>>> +/**
>>> + * struct get_status_str - get status string
>>> + *
>>> + * @status:  efi_status_t value to covert to string
>>> + * Return:   pointer to the string
>>> + */
>>> +static char *get_error_str(efi_status_t status)
>>> +{
>>> +     u32 i;
>>> +
>>> +     for (i = 0; status_str_table[i].status != 0; i++) {
>>> +             if (status == status_str_table[i].status)
>>> +                     return status_str_table[i].str;
>>> +     }
>>> +     return status_str_table[i].str;
>>> +}
>>> +
>>>    /**
>>>     * eficonfig_print_msg() - print message
>>>     *
>>>     * display the message to the user, user proceeds the screen
>>>     * with any key press.
>>>     *
>>> - * @items:           pointer to the structure of each menu entry
>>> - * @count:           the number of menu entry
>>> - * @menu_header:     pointer to the menu header string
>>> - * Return:   status code
>>> + * @msg:     pointer to the error message
>>> + * @status:  efi status code, set 0 if no status string output
>>>     */
>>> -void eficonfig_print_msg(char *msg)
>>> +void eficonfig_print_msg(const char *msg, efi_status_t status)
>>>    {
>>> +     char str[128];
>>> +
>>> +     if (status == 0)
>>> +             snprintf(str, sizeof(str), "%s", msg);
>>> +     else
>>> +             snprintf(str, sizeof(str), "%s (%s)", msg, get_error_str(status));
>>> +
>>>        /* Flush input */
>>>        while (tstc())
>>>                getchar();
>>> @@ -171,7 +242,7 @@ void eficonfig_print_msg(char *msg)
>>>        printf(ANSI_CURSOR_HIDE
>>>               ANSI_CLEAR_CONSOLE
>>>               ANSI_CURSOR_POSITION
>>> -            "%s\n\n  Press any key to continue", 3, 4, msg);
>>> +            "%s\n\n  Press any key to continue", 3, 4, str);
>>>
>>>        getchar();
>>>    }
>>> @@ -580,7 +651,7 @@ static efi_status_t eficonfig_file_selected(void *data)
>>>                new_len = u16_strlen(info->file_info->current_path) +
>>>                                     strlen(info->file_name);
>>>                if (new_len >= EFICONFIG_FILE_PATH_MAX) {
>>> -                     eficonfig_print_msg("File path is too long!");
>>> +                     eficonfig_print_msg("File path is too long!", 0);
>>>                        return EFI_INVALID_PARAMETER;
>>>                }
>>>                tmp = &info->file_info->current_path[u16_strlen(info->file_info->current_path)];
>>> @@ -626,7 +697,7 @@ static efi_status_t eficonfig_select_volume(struct eficonfig_select_file_info *f
>>>        ret = efi_locate_handle_buffer_int(BY_PROTOCOL, &efi_simple_file_system_protocol_guid,
>>>                                           NULL, &count, (efi_handle_t **)&volume_handles);
>>>        if (ret != EFI_SUCCESS) {
>>> -             eficonfig_print_msg("No block device found!");
>>> +             eficonfig_print_msg("No block device found!", ret);
>>>                return ret;
>>>        }
>>>
>>> @@ -850,7 +921,7 @@ static efi_status_t eficonfig_show_file_selection(struct eficonfig_select_file_i
>>>                ret = EFI_CALL(root->open(root, &f, file_info->current_path,
>>>                                          EFI_FILE_MODE_READ, 0));
>>>                if (ret != EFI_SUCCESS) {
>>> -                     eficonfig_print_msg("Reading volume failed!");
>>> +                     eficonfig_print_msg("Reading volume failed!", ret);
>>>                        free(efi_menu);
>>>                        ret = EFI_ABORTED;
>>>                        goto out;
>>> @@ -990,12 +1061,12 @@ static efi_status_t eficonfig_boot_edit_save(void *data)
>>>        struct eficonfig_boot_option *bo = data;
>>>
>>>        if (u16_strlen(bo->description) == 0) {
>>> -             eficonfig_print_msg("Boot Description is empty!");
>>> +             eficonfig_print_msg("Boot Description is empty!", 0);
>>>                bo->edit_completed = false;
>>>                return EFI_NOT_READY;
>>>        }
>>>        if (u16_strlen(bo->file_info.current_path) == 0) {
>>> -             eficonfig_print_msg("File is not selected!");
>>> +             eficonfig_print_msg("File is not selected!", 0);
>>>                bo->edit_completed = false;
>>>                return EFI_NOT_READY;
>>>        }
>>> @@ -2658,7 +2729,7 @@ static efi_status_t eficonfig_init(void)
>>>                avail_row = rows - (EFICONFIG_MENU_HEADER_ROW_NUM +
>>>                                    EFICONFIG_MENU_DESC_ROW_NUM);
>>>                if (avail_row <= 0) {
>>> -                     eficonfig_print_msg("Console size is too small!");
>>> +                     eficonfig_print_msg("Console size is too small!", 0);
>>>                        return EFI_INVALID_PARAMETER;
>>>                }
>>>                /* TODO: Should we check the minimum column size? */
>>> diff --git a/cmd/eficonfig_sbkey.c b/cmd/eficonfig_sbkey.c
>>> index caca27495e..7ae1953567 100644
>>> --- a/cmd/eficonfig_sbkey.c
>>> +++ b/cmd/eficonfig_sbkey.c
>>> @@ -150,7 +150,7 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
>>>        free(buf);
>>>
>>>        if (!size) {
>>> -             eficonfig_print_msg("ERROR! File is empty.");
>>> +             eficonfig_print_msg("ERROR! File is empty.", 0);
>>>                ret = EFI_INVALID_PARAMETER;
>>>                goto out;
>>>        }
>>> @@ -163,11 +163,13 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
>>>
>>>        ret = EFI_CALL(f->read(f, &size, buf));
>>>        if (ret != EFI_SUCCESS) {
>>> -             eficonfig_print_msg("ERROR! Failed to read file.");
>>> +             eficonfig_print_msg("ERROR! Failed to read file.", ret);
>>>                goto out;
>>>        }
>>>        if (!file_have_auth_header(buf, size)) {
>>> -             eficonfig_print_msg("ERROR! Invalid file format. Only .auth variables is allowed.");
>>> +             eficonfig_print_msg(
>>> +                     "ERROR! Invalid file format. Only .auth variables is allowed.",
>>> +                     0);
>>>                ret = EFI_INVALID_PARAMETER;
>>>                goto out;
>>>        }
>>> @@ -175,7 +177,7 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
>>>        ret = file_is_null_key((struct efi_variable_authentication_2 *)buf,
>>>                               size, &null_key);
>>>        if (ret != EFI_SUCCESS) {
>>> -             eficonfig_print_msg("ERROR! Invalid file format.");
>>> +             eficonfig_print_msg("ERROR! Invalid file format.", ret);
>>>                goto out;
>>>        }
>>>
>>> @@ -202,7 +204,7 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
>>>        ret = efi_set_variable_int((u16 *)data, efi_auth_var_get_guid((u16 *)data),
>>>                                   attr, size, buf, false);
>>>        if (ret != EFI_SUCCESS)
>>> -             eficonfig_print_msg("ERROR! Failed to update signature database");
>>> +             eficonfig_print_msg("ERROR! Failed to update signature database", ret);
>>>
>>>    out:
>>>        free(file_info.current_path);
>>> @@ -283,7 +285,7 @@ static efi_status_t eficonfig_process_show_siglist(void *data)
>>>                                break;
>>>                        }
>>>                        default:
>>> -                             eficonfig_print_msg("ERROR! Unsupported format.");
>>> +                             eficonfig_print_msg("ERROR! Unsupported format.", 0);
>>>                                return EFI_INVALID_PARAMETER;
>>>                        }
>>>                }
>>> @@ -394,7 +396,7 @@ static efi_status_t enumerate_and_show_signature_database(void *varname)
>>>
>>>        db = efi_get_var(varname, efi_auth_var_get_guid(varname), &db_size);
>>>        if (!db) {
>>> -             eficonfig_print_msg("There is no entry in the signature database.");
>>> +             eficonfig_print_msg("There is no entry in the signature database.", 0);
>>>                return EFI_NOT_FOUND;
>>>        }
>>>
>>> diff --git a/include/efi_config.h b/include/efi_config.h
>>> index 01ce9b2b06..19b1686907 100644
>>> --- a/include/efi_config.h
>>> +++ b/include/efi_config.h
>>> @@ -93,7 +93,7 @@ struct eficonfig_select_file_info {
>>>        bool file_selected;
>>>    };
>>>
>>> -void eficonfig_print_msg(char *msg);
>>> +void eficonfig_print_msg(const char *msg, efi_status_t status);
>>>    void eficonfig_print_entry(void *data);
>>>    void eficonfig_display_statusline(struct menu *m);
>>>    char *eficonfig_choice_entry(void *data);
>>

Re: [PATCH 4/5] eficonfig: include EFI_STATUS string in error message

[Masahisa Kojima]

Hi Heinrich,

On Mon, 13 Feb 2023 at 16:44, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 2/13/23 06:50, Masahisa Kojima wrote:
> > On Fri, 10 Feb 2023 at 20:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 2/2/23 10:24, Masahisa Kojima wrote:
> >>> Current eficonfig_print_msg() does not show the return
> >>> value of EFI Boot/Runtime Services when the service call fails.
> >>> With this commit, user can know EFI_STATUS in the error message.
> >>
> >> Why do we need function eficonfig_print_msg()?
> >>
> >> I cannot see why the printing only parameter msg with log_err() should
> >> not be good enough.
> >
> > ANSI_CLEAR_CONSOLE is sent before drawing the menu. I think it is
> > difficult for user
> > to know some error occurs by the user operation, user needs scroll up
> > to see the error message
> > when we use log_err(
> Understood. But why do we need the status value (or with this patch the
> long text for the status value)?

At first, I planned to add additional error messages specific to some
status value, but it will increase the eficonfig_print_msg() calls.
Instead of adding eficonfig_print_msg() calls,
I think printing the status value(or text for the status value) will reduce the
code size eventually.
But printing the status code will not much help user to understand
what the error cause is.

Thanks,
Masahisa Kojima

>
> Best regards
>
> Heinrich
>
> >
> > Regards,
> > Masahisa Kojima
> >
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> >>> ---
> >>>    cmd/eficonfig.c       | 95 +++++++++++++++++++++++++++++++++++++------
> >>>    cmd/eficonfig_sbkey.c | 16 ++++----
> >>>    include/efi_config.h  |  2 +-
> >>>    3 files changed, 93 insertions(+), 20 deletions(-)
> >>>
> >>> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> >>> index 0a17b8cf34..b0c8637676 100644
> >>> --- a/cmd/eficonfig.c
> >>> +++ b/cmd/eficonfig.c
> >>> @@ -151,19 +151,90 @@ static void eficonfig_menu_adjust(struct efimenu *efi_menu, bool add)
> >>>    #define eficonfig_menu_up(_a) eficonfig_menu_adjust(_a, false)
> >>>    #define eficonfig_menu_down(_a) eficonfig_menu_adjust(_a, true)
> >>>
> >>> +struct efi_status_str {
> >>> +     efi_status_t status;
> >>> +     char *str;
> >>> +};
> >>> +
> >>> +static const struct efi_status_str status_str_table[] = {
> >>> +     {EFI_LOAD_ERROR,                "Load Error"},
> >>> +     {EFI_INVALID_PARAMETER,         "Invalid Parameter"},
> >>> +     {EFI_UNSUPPORTED,               "Unsupported"},
> >>> +     {EFI_BAD_BUFFER_SIZE,           "Bad Buffer Size"},
> >>> +     {EFI_BUFFER_TOO_SMALL,          "Buffer Too Small"},
> >>> +     {EFI_NOT_READY,                 "Not Ready"},
> >>> +     {EFI_DEVICE_ERROR,              "Device Error"},
> >>> +     {EFI_WRITE_PROTECTED,           "Write Protected"},
> >>> +     {EFI_OUT_OF_RESOURCES,          "Out of Resources"},
> >>> +     {EFI_VOLUME_CORRUPTED,          "Volume Corrupted"},
> >>> +     {EFI_VOLUME_FULL,               "Volume Full"},
> >>> +     {EFI_NO_MEDIA,                  "No Media"},
> >>> +     {EFI_MEDIA_CHANGED,             "Media Changed"},
> >>> +     {EFI_NOT_FOUND,                 "Not Found"},
> >>> +     {EFI_ACCESS_DENIED,             "Access Denied"},
> >>> +     {EFI_NO_RESPONSE,               "No Response"},
> >>> +     {EFI_NO_MAPPING,                "No Mapping"},
> >>> +     {EFI_TIMEOUT,                   "Timeout"},
> >>> +     {EFI_NOT_STARTED,               "Not Started"},
> >>> +     {EFI_ALREADY_STARTED,           "Already Started"},
> >>> +     {EFI_ABORTED,                   "Aborted"},
> >>> +     {EFI_ICMP_ERROR,                "ICMP Error"},
> >>> +     {EFI_TFTP_ERROR,                "TFTP Error"},
> >>> +     {EFI_PROTOCOL_ERROR,            "Protocol Error"},
> >>> +     {EFI_INCOMPATIBLE_VERSION,      "Incompatible Version"},
> >>> +     {EFI_SECURITY_VIOLATION,        "Security Violation"},
> >>> +     {EFI_CRC_ERROR,                 "CRC Error"},
> >>> +     {EFI_END_OF_MEDIA,              "End of Media"},
> >>> +     {EFI_END_OF_FILE,               "End of File"},
> >>> +     {EFI_INVALID_LANGUAGE,          "Invalid Language"},
> >>> +     {EFI_COMPROMISED_DATA,          "Compromised Data"},
> >>> +     {EFI_IP_ADDRESS_CONFLICT,       "IP Address Conflict"},
> >>> +     {EFI_HTTP_ERROR,                "HTTP Error"},
> >>> +     {EFI_WARN_UNKNOWN_GLYPH,        "Warn Unknown Glyph"},
> >>> +     {EFI_WARN_DELETE_FAILURE,       "Warn Delete Failure"},
> >>> +     {EFI_WARN_WRITE_FAILURE,        "Warn Write Failure"},
> >>> +     {EFI_WARN_BUFFER_TOO_SMALL,     "Warn Buffer Too Small"},
> >>> +     {EFI_WARN_STALE_DATA,           "Warn Stale Data"},
> >>> +     {EFI_WARN_FILE_SYSTEM,          "Warn File System"},
> >>> +     {EFI_WARN_RESET_REQUIRED,       "Warn Reset Required"},
> >>> +     {0, ""},
> >>> +};
> >>> +
> >>> +/**
> >>> + * struct get_status_str - get status string
> >>> + *
> >>> + * @status:  efi_status_t value to covert to string
> >>> + * Return:   pointer to the string
> >>> + */
> >>> +static char *get_error_str(efi_status_t status)
> >>> +{
> >>> +     u32 i;
> >>> +
> >>> +     for (i = 0; status_str_table[i].status != 0; i++) {
> >>> +             if (status == status_str_table[i].status)
> >>> +                     return status_str_table[i].str;
> >>> +     }
> >>> +     return status_str_table[i].str;
> >>> +}
> >>> +
> >>>    /**
> >>>     * eficonfig_print_msg() - print message
> >>>     *
> >>>     * display the message to the user, user proceeds the screen
> >>>     * with any key press.
> >>>     *
> >>> - * @items:           pointer to the structure of each menu entry
> >>> - * @count:           the number of menu entry
> >>> - * @menu_header:     pointer to the menu header string
> >>> - * Return:   status code
> >>> + * @msg:     pointer to the error message
> >>> + * @status:  efi status code, set 0 if no status string output
> >>>     */
> >>> -void eficonfig_print_msg(char *msg)
> >>> +void eficonfig_print_msg(const char *msg, efi_status_t status)
> >>>    {
> >>> +     char str[128];
> >>> +
> >>> +     if (status == 0)
> >>> +             snprintf(str, sizeof(str), "%s", msg);
> >>> +     else
> >>> +             snprintf(str, sizeof(str), "%s (%s)", msg, get_error_str(status));
> >>> +
> >>>        /* Flush input */
> >>>        while (tstc())
> >>>                getchar();
> >>> @@ -171,7 +242,7 @@ void eficonfig_print_msg(char *msg)
> >>>        printf(ANSI_CURSOR_HIDE
> >>>               ANSI_CLEAR_CONSOLE
> >>>               ANSI_CURSOR_POSITION
> >>> -            "%s\n\n  Press any key to continue", 3, 4, msg);
> >>> +            "%s\n\n  Press any key to continue", 3, 4, str);
> >>>
> >>>        getchar();
> >>>    }
> >>> @@ -580,7 +651,7 @@ static efi_status_t eficonfig_file_selected(void *data)
> >>>                new_len = u16_strlen(info->file_info->current_path) +
> >>>                                     strlen(info->file_name);
> >>>                if (new_len >= EFICONFIG_FILE_PATH_MAX) {
> >>> -                     eficonfig_print_msg("File path is too long!");
> >>> +                     eficonfig_print_msg("File path is too long!", 0);
> >>>                        return EFI_INVALID_PARAMETER;
> >>>                }
> >>>                tmp = &info->file_info->current_path[u16_strlen(info->file_info->current_path)];
> >>> @@ -626,7 +697,7 @@ static efi_status_t eficonfig_select_volume(struct eficonfig_select_file_info *f
> >>>        ret = efi_locate_handle_buffer_int(BY_PROTOCOL, &efi_simple_file_system_protocol_guid,
> >>>                                           NULL, &count, (efi_handle_t **)&volume_handles);
> >>>        if (ret != EFI_SUCCESS) {
> >>> -             eficonfig_print_msg("No block device found!");
> >>> +             eficonfig_print_msg("No block device found!", ret);
> >>>                return ret;
> >>>        }
> >>>
> >>> @@ -850,7 +921,7 @@ static efi_status_t eficonfig_show_file_selection(struct eficonfig_select_file_i
> >>>                ret = EFI_CALL(root->open(root, &f, file_info->current_path,
> >>>                                          EFI_FILE_MODE_READ, 0));
> >>>                if (ret != EFI_SUCCESS) {
> >>> -                     eficonfig_print_msg("Reading volume failed!");
> >>> +                     eficonfig_print_msg("Reading volume failed!", ret);
> >>>                        free(efi_menu);
> >>>                        ret = EFI_ABORTED;
> >>>                        goto out;
> >>> @@ -990,12 +1061,12 @@ static efi_status_t eficonfig_boot_edit_save(void *data)
> >>>        struct eficonfig_boot_option *bo = data;
> >>>
> >>>        if (u16_strlen(bo->description) == 0) {
> >>> -             eficonfig_print_msg("Boot Description is empty!");
> >>> +             eficonfig_print_msg("Boot Description is empty!", 0);
> >>>                bo->edit_completed = false;
> >>>                return EFI_NOT_READY;
> >>>        }
> >>>        if (u16_strlen(bo->file_info.current_path) == 0) {
> >>> -             eficonfig_print_msg("File is not selected!");
> >>> +             eficonfig_print_msg("File is not selected!", 0);
> >>>                bo->edit_completed = false;
> >>>                return EFI_NOT_READY;
> >>>        }
> >>> @@ -2658,7 +2729,7 @@ static efi_status_t eficonfig_init(void)
> >>>                avail_row = rows - (EFICONFIG_MENU_HEADER_ROW_NUM +
> >>>                                    EFICONFIG_MENU_DESC_ROW_NUM);
> >>>                if (avail_row <= 0) {
> >>> -                     eficonfig_print_msg("Console size is too small!");
> >>> +                     eficonfig_print_msg("Console size is too small!", 0);
> >>>                        return EFI_INVALID_PARAMETER;
> >>>                }
> >>>                /* TODO: Should we check the minimum column size? */
> >>> diff --git a/cmd/eficonfig_sbkey.c b/cmd/eficonfig_sbkey.c
> >>> index caca27495e..7ae1953567 100644
> >>> --- a/cmd/eficonfig_sbkey.c
> >>> +++ b/cmd/eficonfig_sbkey.c
> >>> @@ -150,7 +150,7 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
> >>>        free(buf);
> >>>
> >>>        if (!size) {
> >>> -             eficonfig_print_msg("ERROR! File is empty.");
> >>> +             eficonfig_print_msg("ERROR! File is empty.", 0);
> >>>                ret = EFI_INVALID_PARAMETER;
> >>>                goto out;
> >>>        }
> >>> @@ -163,11 +163,13 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
> >>>
> >>>        ret = EFI_CALL(f->read(f, &size, buf));
> >>>        if (ret != EFI_SUCCESS) {
> >>> -             eficonfig_print_msg("ERROR! Failed to read file.");
> >>> +             eficonfig_print_msg("ERROR! Failed to read file.", ret);
> >>>                goto out;
> >>>        }
> >>>        if (!file_have_auth_header(buf, size)) {
> >>> -             eficonfig_print_msg("ERROR! Invalid file format. Only .auth variables is allowed.");
> >>> +             eficonfig_print_msg(
> >>> +                     "ERROR! Invalid file format. Only .auth variables is allowed.",
> >>> +                     0);
> >>>                ret = EFI_INVALID_PARAMETER;
> >>>                goto out;
> >>>        }
> >>> @@ -175,7 +177,7 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
> >>>        ret = file_is_null_key((struct efi_variable_authentication_2 *)buf,
> >>>                               size, &null_key);
> >>>        if (ret != EFI_SUCCESS) {
> >>> -             eficonfig_print_msg("ERROR! Invalid file format.");
> >>> +             eficonfig_print_msg("ERROR! Invalid file format.", ret);
> >>>                goto out;
> >>>        }
> >>>
> >>> @@ -202,7 +204,7 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
> >>>        ret = efi_set_variable_int((u16 *)data, efi_auth_var_get_guid((u16 *)data),
> >>>                                   attr, size, buf, false);
> >>>        if (ret != EFI_SUCCESS)
> >>> -             eficonfig_print_msg("ERROR! Failed to update signature database");
> >>> +             eficonfig_print_msg("ERROR! Failed to update signature database", ret);
> >>>
> >>>    out:
> >>>        free(file_info.current_path);
> >>> @@ -283,7 +285,7 @@ static efi_status_t eficonfig_process_show_siglist(void *data)
> >>>                                break;
> >>>                        }
> >>>                        default:
> >>> -                             eficonfig_print_msg("ERROR! Unsupported format.");
> >>> +                             eficonfig_print_msg("ERROR! Unsupported format.", 0);
> >>>                                return EFI_INVALID_PARAMETER;
> >>>                        }
> >>>                }
> >>> @@ -394,7 +396,7 @@ static efi_status_t enumerate_and_show_signature_database(void *varname)
> >>>
> >>>        db = efi_get_var(varname, efi_auth_var_get_guid(varname), &db_size);
> >>>        if (!db) {
> >>> -             eficonfig_print_msg("There is no entry in the signature database.");
> >>> +             eficonfig_print_msg("There is no entry in the signature database.", 0);
> >>>                return EFI_NOT_FOUND;
> >>>        }
> >>>
> >>> diff --git a/include/efi_config.h b/include/efi_config.h
> >>> index 01ce9b2b06..19b1686907 100644
> >>> --- a/include/efi_config.h
> >>> +++ b/include/efi_config.h
> >>> @@ -93,7 +93,7 @@ struct eficonfig_select_file_info {
> >>>        bool file_selected;
> >>>    };
> >>>
> >>> -void eficonfig_print_msg(char *msg);
> >>> +void eficonfig_print_msg(const char *msg, efi_status_t status);
> >>>    void eficonfig_print_entry(void *data);
> >>>    void eficonfig_display_statusline(struct menu *m);
> >>>    char *eficonfig_choice_entry(void *data);
> >>
>

Re: [PATCH 4/5] eficonfig: include EFI_STATUS string in error message

[Heinrich Schuchardt]

On 2/13/23 10:42, Masahisa Kojima wrote:
> Hi Heinrich,
>
> On Mon, 13 Feb 2023 at 16:44, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 2/13/23 06:50, Masahisa Kojima wrote:
>>> On Fri, 10 Feb 2023 at 20:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>
>>>> On 2/2/23 10:24, Masahisa Kojima wrote:
>>>>> Current eficonfig_print_msg() does not show the return
>>>>> value of EFI Boot/Runtime Services when the service call fails.
>>>>> With this commit, user can know EFI_STATUS in the error message.
>>>>
>>>> Why do we need function eficonfig_print_msg()?
>>>>
>>>> I cannot see why the printing only parameter msg with log_err() should
>>>> not be good enough.
>>>
>>> ANSI_CLEAR_CONSOLE is sent before drawing the menu. I think it is
>>> difficult for user
>>> to know some error occurs by the user operation, user needs scroll up
>>> to see the error message
>>> when we use log_err(
>> Understood. But why do we need the status value (or with this patch the
>> long text for the status value)?
>
> At first, I planned to add additional error messages specific to some
> status value, but it will increase the eficonfig_print_msg() calls.

Which message remains unclear without the extra information?


> Instead of adding eficonfig_print_msg() calls,
> I think printing the status value(or text for the status value) will reduce the
> code size eventually.
> But printing the status code will not much help user to understand
> what the error cause is.
>
> Thanks,
> Masahisa Kojima
>
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> Regards,
>>> Masahisa Kojima
>>>
>>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>>
>>>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>>>>> ---
>>>>>     cmd/eficonfig.c       | 95 +++++++++++++++++++++++++++++++++++++------
>>>>>     cmd/eficonfig_sbkey.c | 16 ++++----
>>>>>     include/efi_config.h  |  2 +-
>>>>>     3 files changed, 93 insertions(+), 20 deletions(-)
>>>>>
>>>>> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
>>>>> index 0a17b8cf34..b0c8637676 100644
>>>>> --- a/cmd/eficonfig.c
>>>>> +++ b/cmd/eficonfig.c
>>>>> @@ -151,19 +151,90 @@ static void eficonfig_menu_adjust(struct efimenu *efi_menu, bool add)
>>>>>     #define eficonfig_menu_up(_a) eficonfig_menu_adjust(_a, false)
>>>>>     #define eficonfig_menu_down(_a) eficonfig_menu_adjust(_a, true)
>>>>>
>>>>> +struct efi_status_str {
>>>>> +     efi_status_t status;
>>>>> +     char *str;
>>>>> +};
>>>>> +
>>>>> +static const struct efi_status_str status_str_table[] = {
>>>>> +     {EFI_LOAD_ERROR,                "Load Error"},
>>>>> +     {EFI_INVALID_PARAMETER,         "Invalid Parameter"},
>>>>> +     {EFI_UNSUPPORTED,               "Unsupported"},
>>>>> +     {EFI_BAD_BUFFER_SIZE,           "Bad Buffer Size"},
>>>>> +     {EFI_BUFFER_TOO_SMALL,          "Buffer Too Small"},
>>>>> +     {EFI_NOT_READY,                 "Not Ready"},
>>>>> +     {EFI_DEVICE_ERROR,              "Device Error"},
>>>>> +     {EFI_WRITE_PROTECTED,           "Write Protected"},
>>>>> +     {EFI_OUT_OF_RESOURCES,          "Out of Resources"},
>>>>> +     {EFI_VOLUME_CORRUPTED,          "Volume Corrupted"},
>>>>> +     {EFI_VOLUME_FULL,               "Volume Full"},
>>>>> +     {EFI_NO_MEDIA,                  "No Media"},
>>>>> +     {EFI_MEDIA_CHANGED,             "Media Changed"},
>>>>> +     {EFI_NOT_FOUND,                 "Not Found"},
>>>>> +     {EFI_ACCESS_DENIED,             "Access Denied"},
>>>>> +     {EFI_NO_RESPONSE,               "No Response"},
>>>>> +     {EFI_NO_MAPPING,                "No Mapping"},
>>>>> +     {EFI_TIMEOUT,                   "Timeout"},
>>>>> +     {EFI_NOT_STARTED,               "Not Started"},
>>>>> +     {EFI_ALREADY_STARTED,           "Already Started"},
>>>>> +     {EFI_ABORTED,                   "Aborted"},
>>>>> +     {EFI_ICMP_ERROR,                "ICMP Error"},
>>>>> +     {EFI_TFTP_ERROR,                "TFTP Error"},
>>>>> +     {EFI_PROTOCOL_ERROR,            "Protocol Error"},
>>>>> +     {EFI_INCOMPATIBLE_VERSION,      "Incompatible Version"},
>>>>> +     {EFI_SECURITY_VIOLATION,        "Security Violation"},
>>>>> +     {EFI_CRC_ERROR,                 "CRC Error"},
>>>>> +     {EFI_END_OF_MEDIA,              "End of Media"},
>>>>> +     {EFI_END_OF_FILE,               "End of File"},
>>>>> +     {EFI_INVALID_LANGUAGE,          "Invalid Language"},
>>>>> +     {EFI_COMPROMISED_DATA,          "Compromised Data"},
>>>>> +     {EFI_IP_ADDRESS_CONFLICT,       "IP Address Conflict"},
>>>>> +     {EFI_HTTP_ERROR,                "HTTP Error"},
>>>>> +     {EFI_WARN_UNKNOWN_GLYPH,        "Warn Unknown Glyph"},
>>>>> +     {EFI_WARN_DELETE_FAILURE,       "Warn Delete Failure"},
>>>>> +     {EFI_WARN_WRITE_FAILURE,        "Warn Write Failure"},
>>>>> +     {EFI_WARN_BUFFER_TOO_SMALL,     "Warn Buffer Too Small"},
>>>>> +     {EFI_WARN_STALE_DATA,           "Warn Stale Data"},
>>>>> +     {EFI_WARN_FILE_SYSTEM,          "Warn File System"},
>>>>> +     {EFI_WARN_RESET_REQUIRED,       "Warn Reset Required"},
>>>>> +     {0, ""},
>>>>> +};
>>>>> +
>>>>> +/**
>>>>> + * struct get_status_str - get status string
>>>>> + *
>>>>> + * @status:  efi_status_t value to covert to string
>>>>> + * Return:   pointer to the string
>>>>> + */
>>>>> +static char *get_error_str(efi_status_t status)
>>>>> +{
>>>>> +     u32 i;
>>>>> +
>>>>> +     for (i = 0; status_str_table[i].status != 0; i++) {
>>>>> +             if (status == status_str_table[i].status)
>>>>> +                     return status_str_table[i].str;
>>>>> +     }
>>>>> +     return status_str_table[i].str;
>>>>> +}
>>>>> +
>>>>>     /**
>>>>>      * eficonfig_print_msg() - print message
>>>>>      *
>>>>>      * display the message to the user, user proceeds the screen
>>>>>      * with any key press.
>>>>>      *
>>>>> - * @items:           pointer to the structure of each menu entry
>>>>> - * @count:           the number of menu entry
>>>>> - * @menu_header:     pointer to the menu header string
>>>>> - * Return:   status code
>>>>> + * @msg:     pointer to the error message
>>>>> + * @status:  efi status code, set 0 if no status string output
>>>>>      */
>>>>> -void eficonfig_print_msg(char *msg)
>>>>> +void eficonfig_print_msg(const char *msg, efi_status_t status)
>>>>>     {
>>>>> +     char str[128];
>>>>> +
>>>>> +     if (status == 0)
>>>>> +             snprintf(str, sizeof(str), "%s", msg);
>>>>> +     else
>>>>> +             snprintf(str, sizeof(str), "%s (%s)", msg, get_error_str(status));
>>>>> +
>>>>>         /* Flush input */
>>>>>         while (tstc())
>>>>>                 getchar();
>>>>> @@ -171,7 +242,7 @@ void eficonfig_print_msg(char *msg)
>>>>>         printf(ANSI_CURSOR_HIDE
>>>>>                ANSI_CLEAR_CONSOLE
>>>>>                ANSI_CURSOR_POSITION
>>>>> -            "%s\n\n  Press any key to continue", 3, 4, msg);
>>>>> +            "%s\n\n  Press any key to continue", 3, 4, str);
>>>>>
>>>>>         getchar();
>>>>>     }
>>>>> @@ -580,7 +651,7 @@ static efi_status_t eficonfig_file_selected(void *data)
>>>>>                 new_len = u16_strlen(info->file_info->current_path) +
>>>>>                                      strlen(info->file_name);
>>>>>                 if (new_len >= EFICONFIG_FILE_PATH_MAX) {
>>>>> -                     eficonfig_print_msg("File path is too long!");
>>>>> +                     eficonfig_print_msg("File path is too long!", 0);
>>>>>                         return EFI_INVALID_PARAMETER;
>>>>>                 }
>>>>>                 tmp = &info->file_info->current_path[u16_strlen(info->file_info->current_path)];
>>>>> @@ -626,7 +697,7 @@ static efi_status_t eficonfig_select_volume(struct eficonfig_select_file_info *f
>>>>>         ret = efi_locate_handle_buffer_int(BY_PROTOCOL, &efi_simple_file_system_protocol_guid,
>>>>>                                            NULL, &count, (efi_handle_t **)&volume_handles);
>>>>>         if (ret != EFI_SUCCESS) {
>>>>> -             eficonfig_print_msg("No block device found!");
>>>>> +             eficonfig_print_msg("No block device found!", ret);
>>>>>                 return ret;
>>>>>         }
>>>>>
>>>>> @@ -850,7 +921,7 @@ static efi_status_t eficonfig_show_file_selection(struct eficonfig_select_file_i
>>>>>                 ret = EFI_CALL(root->open(root, &f, file_info->current_path,
>>>>>                                           EFI_FILE_MODE_READ, 0));
>>>>>                 if (ret != EFI_SUCCESS) {
>>>>> -                     eficonfig_print_msg("Reading volume failed!");
>>>>> +                     eficonfig_print_msg("Reading volume failed!", ret);
>>>>>                         free(efi_menu);
>>>>>                         ret = EFI_ABORTED;
>>>>>                         goto out;
>>>>> @@ -990,12 +1061,12 @@ static efi_status_t eficonfig_boot_edit_save(void *data)
>>>>>         struct eficonfig_boot_option *bo = data;
>>>>>
>>>>>         if (u16_strlen(bo->description) == 0) {
>>>>> -             eficonfig_print_msg("Boot Description is empty!");
>>>>> +             eficonfig_print_msg("Boot Description is empty!", 0);
>>>>>                 bo->edit_completed = false;
>>>>>                 return EFI_NOT_READY;
>>>>>         }
>>>>>         if (u16_strlen(bo->file_info.current_path) == 0) {
>>>>> -             eficonfig_print_msg("File is not selected!");
>>>>> +             eficonfig_print_msg("File is not selected!", 0);
>>>>>                 bo->edit_completed = false;
>>>>>                 return EFI_NOT_READY;
>>>>>         }
>>>>> @@ -2658,7 +2729,7 @@ static efi_status_t eficonfig_init(void)
>>>>>                 avail_row = rows - (EFICONFIG_MENU_HEADER_ROW_NUM +
>>>>>                                     EFICONFIG_MENU_DESC_ROW_NUM);
>>>>>                 if (avail_row <= 0) {
>>>>> -                     eficonfig_print_msg("Console size is too small!");
>>>>> +                     eficonfig_print_msg("Console size is too small!", 0);
>>>>>                         return EFI_INVALID_PARAMETER;
>>>>>                 }
>>>>>                 /* TODO: Should we check the minimum column size? */
>>>>> diff --git a/cmd/eficonfig_sbkey.c b/cmd/eficonfig_sbkey.c
>>>>> index caca27495e..7ae1953567 100644
>>>>> --- a/cmd/eficonfig_sbkey.c
>>>>> +++ b/cmd/eficonfig_sbkey.c
>>>>> @@ -150,7 +150,7 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
>>>>>         free(buf);
>>>>>
>>>>>         if (!size) {
>>>>> -             eficonfig_print_msg("ERROR! File is empty.");
>>>>> +             eficonfig_print_msg("ERROR! File is empty.", 0);
>>>>>                 ret = EFI_INVALID_PARAMETER;
>>>>>                 goto out;
>>>>>         }
>>>>> @@ -163,11 +163,13 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
>>>>>
>>>>>         ret = EFI_CALL(f->read(f, &size, buf));
>>>>>         if (ret != EFI_SUCCESS) {
>>>>> -             eficonfig_print_msg("ERROR! Failed to read file.");
>>>>> +             eficonfig_print_msg("ERROR! Failed to read file.", ret);
>>>>>                 goto out;
>>>>>         }
>>>>>         if (!file_have_auth_header(buf, size)) {
>>>>> -             eficonfig_print_msg("ERROR! Invalid file format. Only .auth variables is allowed.");
>>>>> +             eficonfig_print_msg(
>>>>> +                     "ERROR! Invalid file format. Only .auth variables is allowed.",
>>>>> +                     0);
>>>>>                 ret = EFI_INVALID_PARAMETER;
>>>>>                 goto out;
>>>>>         }
>>>>> @@ -175,7 +177,7 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
>>>>>         ret = file_is_null_key((struct efi_variable_authentication_2 *)buf,
>>>>>                                size, &null_key);
>>>>>         if (ret != EFI_SUCCESS) {
>>>>> -             eficonfig_print_msg("ERROR! Invalid file format.");
>>>>> +             eficonfig_print_msg("ERROR! Invalid file format.", ret);
>>>>>                 goto out;
>>>>>         }
>>>>>
>>>>> @@ -202,7 +204,7 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
>>>>>         ret = efi_set_variable_int((u16 *)data, efi_auth_var_get_guid((u16 *)data),
>>>>>                                    attr, size, buf, false);
>>>>>         if (ret != EFI_SUCCESS)
>>>>> -             eficonfig_print_msg("ERROR! Failed to update signature database");
>>>>> +             eficonfig_print_msg("ERROR! Failed to update signature database", ret);
>>>>>
>>>>>     out:
>>>>>         free(file_info.current_path);
>>>>> @@ -283,7 +285,7 @@ static efi_status_t eficonfig_process_show_siglist(void *data)
>>>>>                                 break;
>>>>>                         }
>>>>>                         default:
>>>>> -                             eficonfig_print_msg("ERROR! Unsupported format.");
>>>>> +                             eficonfig_print_msg("ERROR! Unsupported format.", 0);
>>>>>                                 return EFI_INVALID_PARAMETER;
>>>>>                         }
>>>>>                 }
>>>>> @@ -394,7 +396,7 @@ static efi_status_t enumerate_and_show_signature_database(void *varname)
>>>>>
>>>>>         db = efi_get_var(varname, efi_auth_var_get_guid(varname), &db_size);
>>>>>         if (!db) {
>>>>> -             eficonfig_print_msg("There is no entry in the signature database.");
>>>>> +             eficonfig_print_msg("There is no entry in the signature database.", 0);
>>>>>                 return EFI_NOT_FOUND;
>>>>>         }
>>>>>
>>>>> diff --git a/include/efi_config.h b/include/efi_config.h
>>>>> index 01ce9b2b06..19b1686907 100644
>>>>> --- a/include/efi_config.h
>>>>> +++ b/include/efi_config.h
>>>>> @@ -93,7 +93,7 @@ struct eficonfig_select_file_info {
>>>>>         bool file_selected;
>>>>>     };
>>>>>
>>>>> -void eficonfig_print_msg(char *msg);
>>>>> +void eficonfig_print_msg(const char *msg, efi_status_t status);
>>>>>     void eficonfig_print_entry(void *data);
>>>>>     void eficonfig_display_statusline(struct menu *m);
>>>>>     char *eficonfig_choice_entry(void *data);
>>>>
>>

Re: [PATCH 4/5] eficonfig: include EFI_STATUS string in error message

[Masahisa Kojima]

On Mon, 13 Feb 2023 at 18:46, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 2/13/23 10:42, Masahisa Kojima wrote:
> > Hi Heinrich,
> >
> > On Mon, 13 Feb 2023 at 16:44, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 2/13/23 06:50, Masahisa Kojima wrote:
> >>> On Fri, 10 Feb 2023 at 20:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>
> >>>> On 2/2/23 10:24, Masahisa Kojima wrote:
> >>>>> Current eficonfig_print_msg() does not show the return
> >>>>> value of EFI Boot/Runtime Services when the service call fails.
> >>>>> With this commit, user can know EFI_STATUS in the error message.
> >>>>
> >>>> Why do we need function eficonfig_print_msg()?
> >>>>
> >>>> I cannot see why the printing only parameter msg with log_err() should
> >>>> not be good enough.
> >>>
> >>> ANSI_CLEAR_CONSOLE is sent before drawing the menu. I think it is
> >>> difficult for user
> >>> to know some error occurs by the user operation, user needs scroll up
> >>> to see the error message
> >>> when we use log_err(
> >> Understood. But why do we need the status value (or with this patch the
> >> long text for the status value)?
> >
> > At first, I planned to add additional error messages specific to some
> > status value, but it will increase the eficonfig_print_msg() calls.
>
> Which message remains unclear without the extra information?

Not for the specific message.

At first I planned to add the error message when the variable storage
is not enough to store the efi variable like:

ret = efi_set_variable_int(var_name, &efi_global_variable_guid,
          EFI_VARIABLE_NON_VOLATILE |
          EFI_VARIABLE_BOOTSERVICE_ACCESS |
          EFI_VARIABLE_RUNTIME_ACCESS,
          opt[i].size, opt[i].lo, false);
- if (ret != EFI_SUCCESS)
+ if (ret != EFI_OUT_OF_RESOURCES)
+     efi_print_msg("variable storage is not enough");
+ else if (ret != EFI_XXX)
+     efi_print_msg("another error message");

This will result in an increase of efi_print_msg() calls,
I think it is better to print a status value or text.

Thanks,
Masahisa Kojima

>
>
> > Instead of adding eficonfig_print_msg() calls,
> > I think printing the status value(or text for the status value) will reduce the
> > code size eventually.
> > But printing the status code will not much help user to understand
> > what the error cause is.
> >
> > Thanks,
> > Masahisa Kojima
> >
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>> Regards,
> >>> Masahisa Kojima
> >>>
> >>>>
> >>>> Best regards
> >>>>
> >>>> Heinrich
> >>>>
> >>>>>
> >>>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> >>>>> ---
> >>>>>     cmd/eficonfig.c       | 95 +++++++++++++++++++++++++++++++++++++------
> >>>>>     cmd/eficonfig_sbkey.c | 16 ++++----
> >>>>>     include/efi_config.h  |  2 +-
> >>>>>     3 files changed, 93 insertions(+), 20 deletions(-)
> >>>>>
> >>>>> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> >>>>> index 0a17b8cf34..b0c8637676 100644
> >>>>> --- a/cmd/eficonfig.c
> >>>>> +++ b/cmd/eficonfig.c
> >>>>> @@ -151,19 +151,90 @@ static void eficonfig_menu_adjust(struct efimenu *efi_menu, bool add)
> >>>>>     #define eficonfig_menu_up(_a) eficonfig_menu_adjust(_a, false)
> >>>>>     #define eficonfig_menu_down(_a) eficonfig_menu_adjust(_a, true)
> >>>>>
> >>>>> +struct efi_status_str {
> >>>>> +     efi_status_t status;
> >>>>> +     char *str;
> >>>>> +};
> >>>>> +
> >>>>> +static const struct efi_status_str status_str_table[] = {
> >>>>> +     {EFI_LOAD_ERROR,                "Load Error"},
> >>>>> +     {EFI_INVALID_PARAMETER,         "Invalid Parameter"},
> >>>>> +     {EFI_UNSUPPORTED,               "Unsupported"},
> >>>>> +     {EFI_BAD_BUFFER_SIZE,           "Bad Buffer Size"},
> >>>>> +     {EFI_BUFFER_TOO_SMALL,          "Buffer Too Small"},
> >>>>> +     {EFI_NOT_READY,                 "Not Ready"},
> >>>>> +     {EFI_DEVICE_ERROR,              "Device Error"},
> >>>>> +     {EFI_WRITE_PROTECTED,           "Write Protected"},
> >>>>> +     {EFI_OUT_OF_RESOURCES,          "Out of Resources"},
> >>>>> +     {EFI_VOLUME_CORRUPTED,          "Volume Corrupted"},
> >>>>> +     {EFI_VOLUME_FULL,               "Volume Full"},
> >>>>> +     {EFI_NO_MEDIA,                  "No Media"},
> >>>>> +     {EFI_MEDIA_CHANGED,             "Media Changed"},
> >>>>> +     {EFI_NOT_FOUND,                 "Not Found"},
> >>>>> +     {EFI_ACCESS_DENIED,             "Access Denied"},
> >>>>> +     {EFI_NO_RESPONSE,               "No Response"},
> >>>>> +     {EFI_NO_MAPPING,                "No Mapping"},
> >>>>> +     {EFI_TIMEOUT,                   "Timeout"},
> >>>>> +     {EFI_NOT_STARTED,               "Not Started"},
> >>>>> +     {EFI_ALREADY_STARTED,           "Already Started"},
> >>>>> +     {EFI_ABORTED,                   "Aborted"},
> >>>>> +     {EFI_ICMP_ERROR,                "ICMP Error"},
> >>>>> +     {EFI_TFTP_ERROR,                "TFTP Error"},
> >>>>> +     {EFI_PROTOCOL_ERROR,            "Protocol Error"},
> >>>>> +     {EFI_INCOMPATIBLE_VERSION,      "Incompatible Version"},
> >>>>> +     {EFI_SECURITY_VIOLATION,        "Security Violation"},
> >>>>> +     {EFI_CRC_ERROR,                 "CRC Error"},
> >>>>> +     {EFI_END_OF_MEDIA,              "End of Media"},
> >>>>> +     {EFI_END_OF_FILE,               "End of File"},
> >>>>> +     {EFI_INVALID_LANGUAGE,          "Invalid Language"},
> >>>>> +     {EFI_COMPROMISED_DATA,          "Compromised Data"},
> >>>>> +     {EFI_IP_ADDRESS_CONFLICT,       "IP Address Conflict"},
> >>>>> +     {EFI_HTTP_ERROR,                "HTTP Error"},
> >>>>> +     {EFI_WARN_UNKNOWN_GLYPH,        "Warn Unknown Glyph"},
> >>>>> +     {EFI_WARN_DELETE_FAILURE,       "Warn Delete Failure"},
> >>>>> +     {EFI_WARN_WRITE_FAILURE,        "Warn Write Failure"},
> >>>>> +     {EFI_WARN_BUFFER_TOO_SMALL,     "Warn Buffer Too Small"},
> >>>>> +     {EFI_WARN_STALE_DATA,           "Warn Stale Data"},
> >>>>> +     {EFI_WARN_FILE_SYSTEM,          "Warn File System"},
> >>>>> +     {EFI_WARN_RESET_REQUIRED,       "Warn Reset Required"},
> >>>>> +     {0, ""},
> >>>>> +};
> >>>>> +
> >>>>> +/**
> >>>>> + * struct get_status_str - get status string
> >>>>> + *
> >>>>> + * @status:  efi_status_t value to covert to string
> >>>>> + * Return:   pointer to the string
> >>>>> + */
> >>>>> +static char *get_error_str(efi_status_t status)
> >>>>> +{
> >>>>> +     u32 i;
> >>>>> +
> >>>>> +     for (i = 0; status_str_table[i].status != 0; i++) {
> >>>>> +             if (status == status_str_table[i].status)
> >>>>> +                     return status_str_table[i].str;
> >>>>> +     }
> >>>>> +     return status_str_table[i].str;
> >>>>> +}
> >>>>> +
> >>>>>     /**
> >>>>>      * eficonfig_print_msg() - print message
> >>>>>      *
> >>>>>      * display the message to the user, user proceeds the screen
> >>>>>      * with any key press.
> >>>>>      *
> >>>>> - * @items:           pointer to the structure of each menu entry
> >>>>> - * @count:           the number of menu entry
> >>>>> - * @menu_header:     pointer to the menu header string
> >>>>> - * Return:   status code
> >>>>> + * @msg:     pointer to the error message
> >>>>> + * @status:  efi status code, set 0 if no status string output
> >>>>>      */
> >>>>> -void eficonfig_print_msg(char *msg)
> >>>>> +void eficonfig_print_msg(const char *msg, efi_status_t status)
> >>>>>     {
> >>>>> +     char str[128];
> >>>>> +
> >>>>> +     if (status == 0)
> >>>>> +             snprintf(str, sizeof(str), "%s", msg);
> >>>>> +     else
> >>>>> +             snprintf(str, sizeof(str), "%s (%s)", msg, get_error_str(status));
> >>>>> +
> >>>>>         /* Flush input */
> >>>>>         while (tstc())
> >>>>>                 getchar();
> >>>>> @@ -171,7 +242,7 @@ void eficonfig_print_msg(char *msg)
> >>>>>         printf(ANSI_CURSOR_HIDE
> >>>>>                ANSI_CLEAR_CONSOLE
> >>>>>                ANSI_CURSOR_POSITION
> >>>>> -            "%s\n\n  Press any key to continue", 3, 4, msg);
> >>>>> +            "%s\n\n  Press any key to continue", 3, 4, str);
> >>>>>
> >>>>>         getchar();
> >>>>>     }
> >>>>> @@ -580,7 +651,7 @@ static efi_status_t eficonfig_file_selected(void *data)
> >>>>>                 new_len = u16_strlen(info->file_info->current_path) +
> >>>>>                                      strlen(info->file_name);
> >>>>>                 if (new_len >= EFICONFIG_FILE_PATH_MAX) {
> >>>>> -                     eficonfig_print_msg("File path is too long!");
> >>>>> +                     eficonfig_print_msg("File path is too long!", 0);
> >>>>>                         return EFI_INVALID_PARAMETER;
> >>>>>                 }
> >>>>>                 tmp = &info->file_info->current_path[u16_strlen(info->file_info->current_path)];
> >>>>> @@ -626,7 +697,7 @@ static efi_status_t eficonfig_select_volume(struct eficonfig_select_file_info *f
> >>>>>         ret = efi_locate_handle_buffer_int(BY_PROTOCOL, &efi_simple_file_system_protocol_guid,
> >>>>>                                            NULL, &count, (efi_handle_t **)&volume_handles);
> >>>>>         if (ret != EFI_SUCCESS) {
> >>>>> -             eficonfig_print_msg("No block device found!");
> >>>>> +             eficonfig_print_msg("No block device found!", ret);
> >>>>>                 return ret;
> >>>>>         }
> >>>>>
> >>>>> @@ -850,7 +921,7 @@ static efi_status_t eficonfig_show_file_selection(struct eficonfig_select_file_i
> >>>>>                 ret = EFI_CALL(root->open(root, &f, file_info->current_path,
> >>>>>                                           EFI_FILE_MODE_READ, 0));
> >>>>>                 if (ret != EFI_SUCCESS) {
> >>>>> -                     eficonfig_print_msg("Reading volume failed!");
> >>>>> +                     eficonfig_print_msg("Reading volume failed!", ret);
> >>>>>                         free(efi_menu);
> >>>>>                         ret = EFI_ABORTED;
> >>>>>                         goto out;
> >>>>> @@ -990,12 +1061,12 @@ static efi_status_t eficonfig_boot_edit_save(void *data)
> >>>>>         struct eficonfig_boot_option *bo = data;
> >>>>>
> >>>>>         if (u16_strlen(bo->description) == 0) {
> >>>>> -             eficonfig_print_msg("Boot Description is empty!");
> >>>>> +             eficonfig_print_msg("Boot Description is empty!", 0);
> >>>>>                 bo->edit_completed = false;
> >>>>>                 return EFI_NOT_READY;
> >>>>>         }
> >>>>>         if (u16_strlen(bo->file_info.current_path) == 0) {
> >>>>> -             eficonfig_print_msg("File is not selected!");
> >>>>> +             eficonfig_print_msg("File is not selected!", 0);
> >>>>>                 bo->edit_completed = false;
> >>>>>                 return EFI_NOT_READY;
> >>>>>         }
> >>>>> @@ -2658,7 +2729,7 @@ static efi_status_t eficonfig_init(void)
> >>>>>                 avail_row = rows - (EFICONFIG_MENU_HEADER_ROW_NUM +
> >>>>>                                     EFICONFIG_MENU_DESC_ROW_NUM);
> >>>>>                 if (avail_row <= 0) {
> >>>>> -                     eficonfig_print_msg("Console size is too small!");
> >>>>> +                     eficonfig_print_msg("Console size is too small!", 0);
> >>>>>                         return EFI_INVALID_PARAMETER;
> >>>>>                 }
> >>>>>                 /* TODO: Should we check the minimum column size? */
> >>>>> diff --git a/cmd/eficonfig_sbkey.c b/cmd/eficonfig_sbkey.c
> >>>>> index caca27495e..7ae1953567 100644
> >>>>> --- a/cmd/eficonfig_sbkey.c
> >>>>> +++ b/cmd/eficonfig_sbkey.c
> >>>>> @@ -150,7 +150,7 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
> >>>>>         free(buf);
> >>>>>
> >>>>>         if (!size) {
> >>>>> -             eficonfig_print_msg("ERROR! File is empty.");
> >>>>> +             eficonfig_print_msg("ERROR! File is empty.", 0);
> >>>>>                 ret = EFI_INVALID_PARAMETER;
> >>>>>                 goto out;
> >>>>>         }
> >>>>> @@ -163,11 +163,13 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
> >>>>>
> >>>>>         ret = EFI_CALL(f->read(f, &size, buf));
> >>>>>         if (ret != EFI_SUCCESS) {
> >>>>> -             eficonfig_print_msg("ERROR! Failed to read file.");
> >>>>> +             eficonfig_print_msg("ERROR! Failed to read file.", ret);
> >>>>>                 goto out;
> >>>>>         }
> >>>>>         if (!file_have_auth_header(buf, size)) {
> >>>>> -             eficonfig_print_msg("ERROR! Invalid file format. Only .auth variables is allowed.");
> >>>>> +             eficonfig_print_msg(
> >>>>> +                     "ERROR! Invalid file format. Only .auth variables is allowed.",
> >>>>> +                     0);
> >>>>>                 ret = EFI_INVALID_PARAMETER;
> >>>>>                 goto out;
> >>>>>         }
> >>>>> @@ -175,7 +177,7 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
> >>>>>         ret = file_is_null_key((struct efi_variable_authentication_2 *)buf,
> >>>>>                                size, &null_key);
> >>>>>         if (ret != EFI_SUCCESS) {
> >>>>> -             eficonfig_print_msg("ERROR! Invalid file format.");
> >>>>> +             eficonfig_print_msg("ERROR! Invalid file format.", ret);
> >>>>>                 goto out;
> >>>>>         }
> >>>>>
> >>>>> @@ -202,7 +204,7 @@ static efi_status_t eficonfig_process_enroll_key(void *data)
> >>>>>         ret = efi_set_variable_int((u16 *)data, efi_auth_var_get_guid((u16 *)data),
> >>>>>                                    attr, size, buf, false);
> >>>>>         if (ret != EFI_SUCCESS)
> >>>>> -             eficonfig_print_msg("ERROR! Failed to update signature database");
> >>>>> +             eficonfig_print_msg("ERROR! Failed to update signature database", ret);
> >>>>>
> >>>>>     out:
> >>>>>         free(file_info.current_path);
> >>>>> @@ -283,7 +285,7 @@ static efi_status_t eficonfig_process_show_siglist(void *data)
> >>>>>                                 break;
> >>>>>                         }
> >>>>>                         default:
> >>>>> -                             eficonfig_print_msg("ERROR! Unsupported format.");
> >>>>> +                             eficonfig_print_msg("ERROR! Unsupported format.", 0);
> >>>>>                                 return EFI_INVALID_PARAMETER;
> >>>>>                         }
> >>>>>                 }
> >>>>> @@ -394,7 +396,7 @@ static efi_status_t enumerate_and_show_signature_database(void *varname)
> >>>>>
> >>>>>         db = efi_get_var(varname, efi_auth_var_get_guid(varname), &db_size);
> >>>>>         if (!db) {
> >>>>> -             eficonfig_print_msg("There is no entry in the signature database.");
> >>>>> +             eficonfig_print_msg("There is no entry in the signature database.", 0);
> >>>>>                 return EFI_NOT_FOUND;
> >>>>>         }
> >>>>>
> >>>>> diff --git a/include/efi_config.h b/include/efi_config.h
> >>>>> index 01ce9b2b06..19b1686907 100644
> >>>>> --- a/include/efi_config.h
> >>>>> +++ b/include/efi_config.h
> >>>>> @@ -93,7 +93,7 @@ struct eficonfig_select_file_info {
> >>>>>         bool file_selected;
> >>>>>     };
> >>>>>
> >>>>> -void eficonfig_print_msg(char *msg);
> >>>>> +void eficonfig_print_msg(const char *msg, efi_status_t status);
> >>>>>     void eficonfig_print_entry(void *data);
> >>>>>     void eficonfig_display_statusline(struct menu *m);
> >>>>>     char *eficonfig_choice_entry(void *data);
> >>>>
> >>
>