[PATCH v4 09/11] efi_loader: add menu-driven UEFI Boot Variable maintenance
Masahisa Kojima
masahisa.kojima at linaro.org
Thu Apr 14 11:25:24 CEST 2022
Hi Ilias,
On Thu, 31 Mar 2022 at 17:31, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> 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 ?
Sorry for the late reply.
If the selected entry is a file, not the directory, no u'\\' will be added.
Strictly speaking, checking for new_len + 1 here ends up that file path name
max size is (EFI_BOOTMENU_FILE_PATH_MAX -1).
>
> > + }
> > + 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?
I will fix other comments.
Thanks,
Masahisa Kojima
>
> > + 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
More information about the U-Boot
mailing list