| Message ID | 20230202092447.28590-5-masahisa.kojima@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | improve eficonfig usability | expand |
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);
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); >
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); >>
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); > >> >
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); >>>> >>
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); > >>>> > >> >
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);
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(-)