On Mon, 13 Feb 2023 at 18:46, Heinrich Schuchardt <xypron.g...@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.g...@gmx.de> > > wrote: > >> > >> On 2/13/23 06:50, Masahisa Kojima wrote: > >>> On Fri, 10 Feb 2023 at 20:57, Heinrich Schuchardt <xypron.g...@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.koj...@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); > >>>> > >> >