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

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

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?

[...]

Thanks
/Ilias