[RESEND v9 2/9] eficonfig: menu-driven addition of UEFI boot option
Ilias Apalodimas
ilias.apalodimas at linaro.org
Tue Jul 19 12:11:03 CEST 2022
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
More information about the U-Boot
mailing list