Hi Ilias,
On Tue, 19 Jul 2022 at 19:11, Ilias Apalodimas
<ilias.apalodi...@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
