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

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