From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id E5D27C433EF for ; Thu, 31 Mar 2022 08:31:58 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 22EBA84172; Thu, 31 Mar 2022 10:31:56 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="UpfAyqjH"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 264EC84172; Thu, 31 Mar 2022 10:31:53 +0200 (CEST) Received: from mail-wm1-x32d.google.com (mail-wm1-x32d.google.com [IPv6:2a00:1450:4864:20::32d]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 5EC0D84127 for ; Thu, 31 Mar 2022 10:31:48 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=ilias.apalodimas@linaro.org Received: by mail-wm1-x32d.google.com with SMTP id r64so13758626wmr.4 for ; Thu, 31 Mar 2022 01:31:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=EtGz7Q1SskSFL6jj9WCmvbOZokvj2WgNuWveUakcRKQ=; b=UpfAyqjH4ggA+7WG1uFE7nDNq+I+TLMNTsEB30m1cnkUMqdGDF8cKpU0gQ/Vn+JLBm aNM3C6ug3yzKfzra/aoitHAqILw78IXD5CgF5hQHrVW9b3WPFKjweWsjvnn12XZUvBW5 1u8cYeHbDCbGPw+6VqLuQ1zJNZHT5/5CYrji9uqe+q8c6LWHz2FNGL3zoolR3kdoFdxh 1zyd4nJwU99tFQplSiBd69xqxCykchreasGEOSGu+qFd2gi0U7NOLZJlzpjun8zAiaMx cH9B6fGoH999ySCKr4pF7RqCnlEdu4CSYZ9rJStnJDMHnxCiG3QDkfAGcPZJ3i0gpiIq mZgg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=EtGz7Q1SskSFL6jj9WCmvbOZokvj2WgNuWveUakcRKQ=; b=oG4wUElKg1iCbRHpc9DE3/Ut2b35mS9Hhtg3GXTear6dnd+pKDIjrdTEblMdViFuda cd2tutXxMmZ7U1yepCh90LZ4PXgvjiM+vNBZYf91mwPrROJfmnYPGYDjdHgPvtK91urp gBnIfUInY/nOolw6XBhx3nzhrKo3ciEv1TN+RwHtPApX8Vyi/Ubu3ltJqzMMWyeh2h3G wiykpaq3Y5e4EWacFMhBTzm0n18toNcMbTFbwp9eiA8vLAqeoA0F6pZhuLyYpbzOn3RG p/E6bNpyTnRKbjVpyItBtgRtk5B0HTdGQUMF/QfC/i6cgfOHF9RGBBSIPNQPfOCFbvdd 0kqw== X-Gm-Message-State: AOAM532KoKMfZGZrQMmiGo+OXRHqj4W04hHrFEjzY/Lz/l4MFFTK/a8m fRal7vgQardyFJwHEiQBuPcbew== X-Google-Smtp-Source: ABdhPJydZrpxPO7rPi7UyF9wWsSj02r3lE8qqJ21PGMZ2/Ndc48mdH7fpKSvz34DTqYZ52iZxYl4Lw== X-Received: by 2002:a1c:7715:0:b0:380:ed9b:debd with SMTP id t21-20020a1c7715000000b00380ed9bdebdmr3726117wmi.54.1648715507875; Thu, 31 Mar 2022 01:31:47 -0700 (PDT) Received: from hades (athedsl-4461682.home.otenet.gr. [94.71.4.98]) by smtp.gmail.com with ESMTPSA id z18-20020a5d6412000000b0020400dde72esm19031787wru.37.2022.03.31.01.31.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 31 Mar 2022 01:31:47 -0700 (PDT) Date: Thu, 31 Mar 2022 11:31:44 +0300 From: Ilias Apalodimas To: Masahisa Kojima Cc: u-boot@lists.denx.de, Heinrich Schuchardt , Simon Glass , Takahiro Akashi , Francois Ozog , Mark Kettenis Subject: Re: [PATCH v4 09/11] efi_loader: add menu-driven UEFI Boot Variable maintenance Message-ID: References: <20220324135443.1571-1-masahisa.kojima@linaro.org> <20220324135443.1571-10-masahisa.kojima@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220324135443.1571-10-masahisa.kojima@linaro.org> X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean 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