Hi Kojima-san,
On Thu, Mar 24, 2022 at 10:54:41PM +0900, Masahisa Kojima wrote:
> +
I haven't been able to get the patch working yet. I'll send more feedback
once I do. Here's a few comments I have
[...]
> +struct efi_bootmenu_file_entry_data {
> + struct efi_bootmenu_boot_option *bo;
> + struct efi_file_info *f;
> +};
> +
> +static efi_status_t efi_bootmenu_process_boot_selected(void *data, bool
> *exit);
> +static efi_status_t efi_bootmenu_process_add_boot_option(void *data, bool
> *exit);
> +static efi_status_t efi_bootmenu_process_delete_boot_option(void *data, bool
> *exit);
> +static efi_status_t efi_bootmenu_process_change_boot_order(void *data, bool
> *exit);
I think you can re-arrange some of the functions and get rid of the forward
declarations
> +
> +static struct efi_bootmenu_item maintenance_menu_items[] = {
const ?
> + {u"Add Boot Option", efi_bootmenu_process_add_boot_option},
> + {u"Delete Boot Option", efi_bootmenu_process_delete_boot_option},
> + {u"Change Boot Order", efi_bootmenu_process_change_boot_order},
> + {u"Quit", NULL},
> +};
> +
> +static void efi_bootmenu_print_entry(void *data)
> +{
> + struct efi_bootmenu_entry *entry = data;
[...]
> + new_len = u16_strlen(info->bo->current_path) +
> + /* TODO: show error notification to user */
> + log_err("file path is too long\n");
> + return EFI_INVALID_PARAMETER;
Can we just check for new_len + 1 here and get rid of the follow up check ?
> + }
> + u16_strlcat(info->bo->current_path, info->f->file_name,
> EFI_BOOTMENU_FILE_PATH_MAX);
> + if (info->f->attribute & EFI_FILE_DIRECTORY) {
> + if (new_len + 1 >= EFI_BOOTMENU_FILE_PATH_MAX) {
> + log_err("file path is too long\n");
> + return EFI_INVALID_PARAMETER;
> + }
> + u16_strlcat(info->bo->current_path, u"\\",
> EFI_BOOTMENU_FILE_PATH_MAX);
> + } else {
> + info->bo->file_selected = true;
> + }
[...]
> + menu_item = calloc(count + 1, sizeof(struct efi_bootmenu_item));
> + if (!menu_item) {
> + efi_file_close_int(f);
> + free(dir_buf);
> + ret = EFI_OUT_OF_RESOURCES;
> + goto out;
> + }
> +
> + /* read directory and construct menu structure */
> + efi_file_setpos_int(f, 0);
> + iter = menu_item;
> + ptr = (struct efi_file_info *)dir_buf;
This will cause an unaligned access later on when you access
ptr->attribute. Any reason we can't allocate ptr directly?
> + for (i = 0; i < count; i++) {
> + int name_len;
> + u16 *name;
> + struct efi_bootmenu_file_entry_data *info;
> +
> + len = size;
> + ret = efi_file_read_int(f, &len, ptr);
> + if (ret != EFI_SUCCESS || len == 0)
> + goto err;
> +
> + if (ptr->attribute & EFI_FILE_DIRECTORY) {
> + /* append u'/' at the end of directory name */
> + name_len = u16_strsize(ptr->file_name) +
> sizeof(u16);
> + name = calloc(1, name_len);
> + if (!name) {
> + ret = EFI_OUT_OF_RESOURCES;
> + goto err;
> + }
> + u16_strcpy(name, ptr->file_name);
> + name[u16_strlen(ptr->file_name)] = u'/';
> + } else {
> + name_len = u16_strsize(ptr->file_name);
> + name = calloc(1, name_len);
> + if (!name) {
> + ret = EFI_OUT_OF_RESOURCES;
> + goto err;
> + }
> + u16_strcpy(name, ptr->file_name);
> + }
> +
> + info = calloc(1, sizeof(struct
> efi_bootmenu_file_entry_data));
> + if (!info) {
> + ret = EFI_OUT_OF_RESOURCES;
> + goto err;
> + }
> + info->f = ptr;
> + info->bo = bo;
> + iter->title = name;
> + iter->func = efi_bootmenu_file_selected;
> + iter->data = info;
> + iter++;
> +
> + size -= len;
> + ptr = (struct efi_file_info *)((char *)ptr + len);
ditto
> + }
> +
> + /* add "Quit" entry */
> + iter->title = u"Quit";
> + iter->func = NULL;
> + iter->data = NULL;
> + count += 1;
> +
> + ret = efi_bootmenu_process_common(menu_item, count, -1);
> +err:
> + efi_file_close_int(f);
> + iter = menu_item;
> + for (i = 0; i < count - 1; i++, iter++) {
> + free(iter->title);
> + free(iter->data);
> + }
> +
> + free(dir_buf);
> + free(menu_item);
> +
> + if (ret != EFI_SUCCESS)
> + break;
> + }
> +
> +out:
> + free(buf);
> + return ret;
> +}
> +
[...]
Regards
/Ilias
