[RESEND v9 2/9] eficonfig: menu-driven addition of UEFI boot option

Masahisa Kojima masahisa.kojima at linaro.org
Fri Jul 22 04:01:31 CEST 2022


Hi Ilias,

On Tue, 19 Jul 2022 at 19:11, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hello Kojima-san,
>
> [...]
>
> > +
> > +/**
> > + * eficonfig_process_common() - main handler for UEFI menu
> > + *
> > + * Construct the structures required to show the menu, then handle
> > + * the user input interacting with u-boot menu functions.
> > + *
> > + * @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
> > + */
> > +efi_status_t eficonfig_process_common(const struct eficonfig_item *items, int count,
> > +                                     char *menu_header)
> > +{
> > +       u32 i;
> > +       efi_status_t ret;
> > +       struct menu *menu;
> > +       void *choice = NULL;
> > +       struct eficonfig_entry *entry;
> > +       struct efimenu *efi_menu;
> > +       struct eficonfig_entry *iter = NULL;
> > +
> > +       if (count > EFICONFIG_ENTRY_NUM_MAX)
> > +               return EFI_OUT_OF_RESOURCES;
> > +
> > +       efi_menu = calloc(1, sizeof(struct efimenu));
> > +       if (!efi_menu)
> > +               return EFI_OUT_OF_RESOURCES;
> > +
> > +       efi_menu->delay = -1;
> > +       efi_menu->active = 0;
> > +       efi_menu->first = NULL;
> > +
> > +       if (menu_header) {
> > +               efi_menu->menu_header = strdup(menu_header);
> > +               if (!efi_menu->menu_header) {
> > +                       free(efi_menu);
> > +                       return EFI_OUT_OF_RESOURCES;
> > +               }
> > +       }
> > +
> > +       for (i = 0; i < count; i++) {
> > +               entry = calloc(1, sizeof(struct eficonfig_entry));
> > +               if (!entry) {
> > +                       ret = EFI_LOAD_ERROR;
> > +                       goto out;
> > +               }
> > +
> > +               entry->num = i;
> > +               entry->title = items->title;
> > +               sprintf(entry->key, "%d", i);
> > +               entry->efi_menu = efi_menu;
> > +               entry->func = items->func;
> > +               entry->data = items->data;
> > +               entry->next = NULL;
> > +
> > +               if (!iter)
> > +                       efi_menu->first = entry;
> > +               else
> > +                       iter->next = entry;
>
> Is there a reason we aren't using list_add etc on this list?
>
> > +
> > +               iter = entry;
> > +               items++;
> > +       }
> > +       efi_menu->count = count;
> > +
> > +       menu = menu_create(NULL, 0, 1, eficonfig_display_statusline,
> > +                          eficonfig_print_entry, eficonfig_choice_entry,
> > +                          efi_menu);
> > +       if (!menu) {
> > +               ret = EFI_INVALID_PARAMETER;
> > +               goto out;
> > +       }
> > +
> > +       for (entry = efi_menu->first; entry; entry = entry->next) {
> > +               if (!menu_item_add(menu, entry->key, entry)) {
> > +                       ret = EFI_INVALID_PARAMETER;
> > +                       goto out;
> > +               }
> > +       }
> > +
>
> [...]
>
> > +
> > +/**
> > + * eficonfig_boot_add_optional_data() - handle user input for optional data
> > + *
> > + * @data:      pinter to the internal boot option structure
> > + * Return:     status code
> > + */
> > +static efi_status_t eficonfig_boot_add_optional_data(void *data)
> > +{
> > +       u16 *tmp;
> > +       efi_status_t ret;
> > +       struct eficonfig_boot_option *bo = data;
> > +
> > +       printf(ANSI_CLEAR_CONSOLE
> > +              ANSI_CURSOR_POSITION
> > +              "\n  ** Edit Optional Data **\n"
> > +              "\n"
> > +              "  enter optional data:"
> > +              ANSI_CURSOR_POSITION
> > +              "  Press ENTER to complete, ESC/CTRL+C to quit",
> > +              0, 1, 8, 1);
> > +
> > +       tmp = calloc(1, EFICONFIG_OPTIONAL_DATA_MAX * sizeof(u16));
>
> if (!tmp)
>     etc
>
> > +       ret = efi_console_get_u16_string(cin, tmp, EFICONFIG_OPTIONAL_DATA_MAX, NULL, 4, 24);
> > +
> > +       if (ret == EFI_SUCCESS)
> > +               u16_strcpy(bo->optional_data, tmp);
> > +
> > +       free(tmp);
> > +
> > +       /* to stay the parent menu */
> > +       ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret;
> > +
> > +       return ret;
> > +}
> > +
> > +/**
> > + * eficonfig_boot_edit_save() - handler to save the boot option
> > + *
> > + * @data:      pinter to the internal boot option structure
>
> s/pinter/pointer/
>
> [...]
>
> > +
> > +/**
> > + * eficonfig_edit_boot_option() - prepare boot option structure for editing
> > + *
> > + * Construct the boot option structure and copy the existing value
> > + *
> > + * @varname:           pointer to the UEFI variable name
> > + * @bo:                        pointer to the boot option
> > + * @description:       pointer to the description
> > + * @optional_data:     pointer to the optional_data
> > + * @optional_data_size:        optional_data_size
> > + * @dp:                        pointer to the device path
> > + * @header_str:                pointer to the header string
> > + * Return      :       status code
> > + */
> > +static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_boot_option *bo,
> > +                                              u16 *description, const u8 *optional_data,
> > +                                              efi_uintn_t optional_data_size,
> > +                                              struct efi_device_path *dp,
> > +                                              char *header_str)
> > +{
> > +       size_t len;
> > +       char *buf = NULL;
> > +       efi_status_t ret;
> > +       char *iter = NULL;
> > +       char *tmp = NULL, *p;
> > +       efi_uintn_t dp_size, fp_size;
> > +       struct efi_device_path *device_dp = NULL;
> > +       struct efi_device_path_file_path *fp;
> > +
> > +       bo->file_info.current_path = calloc(1, EFICONFIG_FILE_PATH_BUF_SIZE);
> > +       if (!bo->file_info.current_path) {
> > +               ret =  EFI_OUT_OF_RESOURCES;
> > +               goto out;
> > +       }
> > +
> > +       bo->description = calloc(1, EFICONFIG_DESCRIPTION_MAX * sizeof(u16));
> > +       if (!bo->description) {
> > +               ret =  EFI_OUT_OF_RESOURCES;
> > +               goto out;
> > +       }
> > +
> > +       bo->optional_data = calloc(1, EFICONFIG_OPTIONAL_DATA_MAX * sizeof(u16));
> > +       if (!bo->optional_data) {
> > +               ret =  EFI_OUT_OF_RESOURCES;
> > +               goto out;
> > +       }
> > +
> > +       if (description && u16_strlen(description) >= EFICONFIG_DESCRIPTION_MAX) {
> > +               ret = EFI_INVALID_PARAMETER;
> > +               goto out;
> > +       }
>
> Can we move this test earlier in the function?
>
> > +       if (description)
>
> and if we do this 'if' will go away
>
> > +               u16_strcpy(bo->description, description);
> > +
>
> [...]
>
> > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > index a8e8521e3e..e5a08f6b59 100644
> > --- a/lib/efi_loader/efi_disk.c
> > +++ b/lib/efi_loader/efi_disk.c
> > @@ -778,3 +778,49 @@ efi_status_t efi_disk_init(void)
> >
> >         return EFI_SUCCESS;
> >  }
> > +
> > +/**
> > + * efi_disk_get_device_name() - get U-Boot device name associated with EFI handle
> > + *
> > + * @handle:    pointer to the EFI handle
> > + * @buf:       pointer to the buffer to store the string
> > + * @size:      size of buffer
> > + * Return:     status code
> > + */
> > +efi_status_t efi_disk_get_device_name(const efi_handle_t handle, char *buf, int size)
> > +{
> > +       int diskid;
> > +       enum uclass_id id;
> > +       unsigned int part;
> > +       struct udevice *dev;
> > +       struct blk_desc *desc;
> > +       const char *if_typename;
> > +       bool is_partition = false;
> > +       struct disk_part *part_data;
> > +
> > +       if (!handle || !buf || !size)
> > +               return EFI_INVALID_PARAMETER;
> > +
> > +       dev = handle->dev;
> > +       id = device_get_uclass_id(dev);
> > +       if (id == UCLASS_BLK) {
> > +               desc = dev_get_uclass_plat(dev);
> > +       } else if (id == UCLASS_PARTITION) {
> > +               desc = dev_get_uclass_plat(dev_get_parent(dev));
> > +               is_partition = true;
> > +       } else {
> > +               return EFI_INVALID_PARAMETER;
> > +       }
> > +       if_typename = blk_get_if_type_name(desc->if_type);
> > +       diskid = desc->devnum;
> > +
> > +       if (is_partition) {
> > +               part_data = dev_get_uclass_plat(dev);
> > +               part = part_data->partnum;
> > +               snprintf(buf, size, "%s %d:%d", if_typename, diskid, part);
> > +       } else {
> > +               snprintf(buf, size, "%s %d", if_typename, diskid);
> > +       }
>
> Shouldn't we be checking for the snprintf result here, since we are
> providing the size?

Thank you for your comments.
I have modified all of the comments.

Thanks,
Masahisa Kojima

>
> [...]
>
> Thanks
> /Ilias


More information about the U-Boot mailing list