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

Masahisa Kojima masahisa.kojima at linaro.org
Mon Feb 13 11:11:21 CET 2023


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


More information about the U-Boot mailing list