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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5AECEC433EF for ; Tue, 9 Nov 2021 08:09:41 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id DC11D6112F for ; Tue, 9 Nov 2021 08:09:40 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org DC11D6112F Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 312CB8390A; Tue, 9 Nov 2021 09:09:39 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com 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=gmail.com header.i=@gmail.com header.b="P0RvEP2R"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 700B88390A; Tue, 9 Nov 2021 09:09:37 +0100 (CET) Received: from mail-ot1-x329.google.com (mail-ot1-x329.google.com [IPv6:2607:f8b0:4864:20::329]) (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 E71A583908 for ; Tue, 9 Nov 2021 09:09:31 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=rfried.dev@gmail.com Received: by mail-ot1-x329.google.com with SMTP id v40-20020a056830092800b0055591caa9c6so29744737ott.4 for ; Tue, 09 Nov 2021 00:09:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=TPvF2n5tOqmtJNFJWFC2wd6PgyKN1Hpv9kSYFF9Q/+Y=; b=P0RvEP2RjjCka0DXoCYtZKyZmslsMcxWfHaLhcqSZb+arcYflGd8dRaKrFG7aO7aVN u0sROjaa4ZsZBMHmJhscYl6UP0ALwTh0voc7U52DRR8nxCWNsdmqVf/AZWKlcwSejZVO wWhs7LLAdbS9cRq6R7t7A+QVkdl+CL4X1bVPMcHH24EkgXgNdzWRDY5VgWvmhwO6FfwO Hjvx0Qg6gzBkDiQHU+HbJOpIp89VD6z7ECY1y0kZjBGBkm1Q1W7OTEsdnm/FORkGmXYf sPWJocDPk0gEGCZwlUngysJ1wZ2+OdnHjntBdnG4TKZCpaok1RAvmMpBAPUf8k1INNrl I7Tw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=TPvF2n5tOqmtJNFJWFC2wd6PgyKN1Hpv9kSYFF9Q/+Y=; b=k72F7BjkjfJyFcfcEOYJ7PK5H2ffegQcKMHtO1xFsmt3CEwyw4wnWWe8QNDpQm2dsV cdL2lym6OM8sYmgWZPFmRetlhCYYBUxdsAHmC+lCsbh1rhgo8VsENH7fgw2Q/+Ipy1Gf NUERCXEkmorLo+keP1FKFlS4GgnmmiyLzPmxnsR7unGVnMZL5Cn8DQT46XSeoEpfpUBO FmlyQ16D0yP2P3v2Dt4TJyi55xGoo2j03rQcXwUFJC6/TO6gs+Ho6BRbl6O4cPFOodzZ K6zXMEM6Ca5QLLOWveKKxLIYSgiKLco4B8j6j9DKDoNbozUsVwY9sIaHgfAM6wmdKkcY 1+rQ== X-Gm-Message-State: AOAM533G3P7IKu5m39U5E4NcutxsR7Es6JsijGe0qSb1IdfPGXbZbJlR /oSwcHj+fVuiONbKzYKt4K9ZJx5rhnn0hawGsSrrR3UXYUtmBA== X-Google-Smtp-Source: ABdhPJy8+8DzPtF+wRLAVmOxte1ckgjO3xUPWorCVWruoJAcctdPrxjmithLL10rtEf8ZrhpY2PyrI8BGYXHBE3/F7Q= X-Received: by 2002:a9d:6f09:: with SMTP id n9mr4422336otq.357.1636445370601; Tue, 09 Nov 2021 00:09:30 -0800 (PST) MIME-Version: 1.0 References: <20211014184811.482560-1-sjg@chromium.org> <20211014124803.v3.15.I5ea9daa8a0f8ddcedd99a7dcf136c195b5c9eb50@changeid> In-Reply-To: <20211014124803.v3.15.I5ea9daa8a0f8ddcedd99a7dcf136c195b5c9eb50@changeid> From: Ramon Fried Date: Tue, 9 Nov 2021 10:09:19 +0200 Message-ID: Subject: Re: [PATCH v3 15/18] pxe: Return the file size from the getfile() function To: Simon Glass Cc: U-Boot Mailing List , Patrice Chotard , Artem Lapkin , Tom Rini , Joe Hershberger , Heinrich Schuchardt , Peter Hoyes Content-Type: text/plain; charset="UTF-8" X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 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.2 at phobos.denx.de X-Virus-Status: Clean On Thu, Oct 14, 2021 at 9:51 PM Simon Glass wrote: > > It is pretty strange that the pxe code uses the 'filesize' environment > variable find the size of a file it has just read. > > Partly this is because it uses the command-line interpreter to parse its > request to load the file. > > As a first step towards unwinding this, return it directly from the > getfile() function. This makes the code a bit longer, for now, but will be > cleaned up in future patches. > > Signed-off-by: Simon Glass > --- > > (no changes since v1) > > boot/pxe_utils.c | 70 ++++++++++++++++++++++++++++----------------- > cmd/pxe.c | 7 ++++- > cmd/sysboot.c | 21 ++++++++++++-- > include/pxe_utils.h | 13 ++++++++- > 4 files changed, 79 insertions(+), 32 deletions(-) > > diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c > index f36f1f8a60c..e377e16be56 100644 > --- a/boot/pxe_utils.c > +++ b/boot/pxe_utils.c > @@ -30,6 +30,20 @@ > > #define MAX_TFTP_PATH_LEN 512 > > +int pxe_get_file_size(ulong *sizep) > +{ > + const char *val; > + > + val = from_env("filesize"); > + if (!val) > + return -ENOENT; > + > + if (strict_strtoul(val, 16, sizep) < 0) > + return -EINVAL; > + > + return 0; > +} > + > /** > * format_mac_pxe() - obtain a MAC address in the PXE format > * > @@ -75,14 +89,17 @@ int format_mac_pxe(char *outbuf, size_t outbuf_len) > * @ctx: PXE context > * @file_path: File path to read (relative to the PXE file) > * @file_addr: Address to load file to > + * @filesizep: If not NULL, returns the file size in bytes > * Returns 1 for success, or < 0 on error > */ > static int get_relfile(struct pxe_context *ctx, const char *file_path, > - unsigned long file_addr) > + unsigned long file_addr, ulong *filesizep) > { > size_t path_len; > char relfile[MAX_TFTP_PATH_LEN + 1]; > char addr_buf[18]; > + ulong size; > + int ret; > > if (file_path[0] == '/' && ctx->allow_abs_path) > *relfile = '\0'; > @@ -103,7 +120,13 @@ static int get_relfile(struct pxe_context *ctx, const char *file_path, > > sprintf(addr_buf, "%lx", file_addr); > > - return ctx->getfile(ctx, relfile, addr_buf); > + ret = ctx->getfile(ctx, relfile, addr_buf, &size); > + if (ret < 0) > + return log_msg_ret("get", ret); > + if (filesizep) > + *filesizep = size; > + > + return 1; > } > > /** > @@ -117,29 +140,17 @@ static int get_relfile(struct pxe_context *ctx, const char *file_path, > * Returns 1 for success, or < 0 on error > */ > int get_pxe_file(struct pxe_context *ctx, const char *file_path, > - unsigned long file_addr) > + ulong file_addr) > { > - unsigned long config_file_size; > - char *tftp_filesize; > + ulong size; > int err; > char *buf; > > - err = get_relfile(ctx, file_path, file_addr); > + err = get_relfile(ctx, file_path, file_addr, &size); > if (err < 0) > return err; > > - /* > - * the file comes without a NUL byte at the end, so find out its size > - * and add the NUL byte. > - */ > - tftp_filesize = from_env("filesize"); > - if (!tftp_filesize) > - return -ENOENT; > - > - if (strict_strtoul(tftp_filesize, 16, &config_file_size) < 0) > - return -EINVAL; > - > - buf = map_sysmem(file_addr + config_file_size, 1); > + buf = map_sysmem(file_addr + size, 1); > *buf = '\0'; > unmap_sysmem(buf); > > @@ -184,12 +195,13 @@ int get_pxelinux_path(struct pxe_context *ctx, const char *file, > * @file_path: File path to read (relative to the PXE file) > * @envaddr_name: Name of environment variable which contains the address to > * load to > + * @filesizep: Returns the file size in bytes > * Returns 1 on success, -ENOENT if @envaddr_name does not exist as an > * environment variable, -EINVAL if its format is not valid hex, or other > * value < 0 on other error > */ > static int get_relfile_envaddr(struct pxe_context *ctx, const char *file_path, > - const char *envaddr_name) > + const char *envaddr_name, ulong *filesizep) > { > unsigned long file_addr; > char *envaddr; > @@ -201,7 +213,7 @@ static int get_relfile_envaddr(struct pxe_context *ctx, const char *file_path, > if (strict_strtoul(envaddr, 16, &file_addr) < 0) > return -EINVAL; > > - return get_relfile(ctx, file_path, file_addr); > + return get_relfile(ctx, file_path, file_addr, filesizep); > } > > /** > @@ -357,8 +369,8 @@ static void label_boot_fdtoverlay(struct pxe_context *ctx, > goto skip_overlay; > > /* Load overlay file */ > - err = get_relfile_envaddr(ctx, overlayfile, > - "fdtoverlay_addr_r"); > + err = get_relfile_envaddr(ctx, overlayfile, "fdtoverlay_addr_r", > + NULL); > if (err < 0) { > printf("Failed loading overlay %s\n", overlayfile); > goto skip_overlay; > @@ -438,7 +450,10 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label) > } > > if (label->initrd) { > - if (get_relfile_envaddr(ctx, label->initrd, "ramdisk_addr_r") < 0) { > + ulong size; > + > + if (get_relfile_envaddr(ctx, label->initrd, "ramdisk_addr_r", > + &size) < 0) { > printf("Skipping %s for failure retrieving initrd\n", > label->name); > return 1; > @@ -447,11 +462,12 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label) > bootm_argv[2] = initrd_str; > strncpy(bootm_argv[2], env_get("ramdisk_addr_r"), 18); > strcat(bootm_argv[2], ":"); > - strncat(bootm_argv[2], env_get("filesize"), 9); > + strcat(bootm_argv[2], simple_xtoa(size)); > bootm_argc = 3; > } > > - if (get_relfile_envaddr(ctx, label->kernel, "kernel_addr_r") < 0) { > + if (get_relfile_envaddr(ctx, label->kernel, "kernel_addr_r", > + NULL) < 0) { > printf("Skipping %s for failure retrieving kernel\n", > label->name); > return 1; > @@ -592,7 +608,7 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label) > > if (fdtfile) { > int err = get_relfile_envaddr(ctx, fdtfile, > - "fdt_addr_r"); > + "fdt_addr_r", NULL); > > free(fdtfilefree); > if (err < 0) { > @@ -1384,7 +1400,7 @@ void handle_pxe_menu(struct pxe_context *ctx, struct pxe_menu *cfg) > if (IS_ENABLED(CONFIG_CMD_BMP)) { > /* display BMP if available */ > if (cfg->bmp) { > - if (get_relfile(ctx, cfg->bmp, image_load_addr)) { > + if (get_relfile(ctx, cfg->bmp, image_load_addr, NULL)) { > if (CONFIG_IS_ENABLED(CMD_CLS)) > run_command("cls", 0); > bmp_display(image_load_addr, > diff --git a/cmd/pxe.c b/cmd/pxe.c > index e319db51ef5..81703386c42 100644 > --- a/cmd/pxe.c > +++ b/cmd/pxe.c > @@ -25,15 +25,20 @@ const char *pxe_default_paths[] = { > }; > > static int do_get_tftp(struct pxe_context *ctx, const char *file_path, > - char *file_addr) > + char *file_addr, ulong *sizep) > { > char *tftp_argv[] = {"tftp", NULL, NULL, NULL}; > + int ret; > > tftp_argv[1] = file_addr; > tftp_argv[2] = (void *)file_path; > > if (do_tftpb(ctx->cmdtp, 0, 3, tftp_argv)) > return -ENOENT; > + ret = pxe_get_file_size(sizep); > + if (ret) > + return log_msg_ret("tftp", ret); > + ctx->pxe_file_size = *sizep; > > return 1; > } > diff --git a/cmd/sysboot.c b/cmd/sysboot.c > index c45fed774d6..6344ecd357b 100644 > --- a/cmd/sysboot.c > +++ b/cmd/sysboot.c > @@ -9,43 +9,58 @@ > static char *fs_argv[5]; > > static int do_get_ext2(struct pxe_context *ctx, const char *file_path, > - char *file_addr) > + char *file_addr, ulong *sizep) > { > #ifdef CONFIG_CMD_EXT2 > + int ret; > + > fs_argv[0] = "ext2load"; > fs_argv[3] = file_addr; > fs_argv[4] = (void *)file_path; > > if (!do_ext2load(ctx->cmdtp, 0, 5, fs_argv)) > return 1; > + ret = pxe_get_file_size(sizep); > + if (ret) > + return log_msg_ret("tftp", ret); > #endif > return -ENOENT; > } > > static int do_get_fat(struct pxe_context *ctx, const char *file_path, > - char *file_addr) > + char *file_addr, ulong *sizep) > { > #ifdef CONFIG_CMD_FAT > + int ret; > + > fs_argv[0] = "fatload"; > fs_argv[3] = file_addr; > fs_argv[4] = (void *)file_path; > > if (!do_fat_fsload(ctx->cmdtp, 0, 5, fs_argv)) > return 1; > + ret = pxe_get_file_size(sizep); > + if (ret) > + return log_msg_ret("tftp", ret); > #endif > return -ENOENT; > } > > static int do_get_any(struct pxe_context *ctx, const char *file_path, > - char *file_addr) > + char *file_addr, ulong *sizep) > { > #ifdef CONFIG_CMD_FS_GENERIC > + int ret; > + > fs_argv[0] = "load"; > fs_argv[3] = file_addr; > fs_argv[4] = (void *)file_path; > > if (!do_load(ctx->cmdtp, 0, 5, fs_argv, FS_TYPE_ANY)) > return 1; > + ret = pxe_get_file_size(sizep); > + if (ret) > + return log_msg_ret("tftp", ret); > #endif > return -ENOENT; > } > diff --git a/include/pxe_utils.h b/include/pxe_utils.h > index 8b50f2e6861..194a5ed8cc7 100644 > --- a/include/pxe_utils.h > +++ b/include/pxe_utils.h > @@ -77,7 +77,7 @@ struct pxe_menu { > > struct pxe_context; > typedef int (*pxe_getfile_func)(struct pxe_context *ctx, const char *file_path, > - char *file_addr); > + char *file_addr, ulong *filesizep); > > /** > * struct pxe_context - context information for PXE parsing > @@ -88,6 +88,7 @@ typedef int (*pxe_getfile_func)(struct pxe_context *ctx, const char *file_path, > * @allow_abs_path: true to allow absolute paths > * @bootdir: Directory that files are loaded from ("" if no directory). This is > * allocated > + * @pxe_file_size: Size of the PXE file > */ > struct pxe_context { > struct cmd_tbl *cmdtp; > @@ -98,6 +99,7 @@ struct pxe_context { > * @file_path: Path to the file > * @file_addr: String containing the hex address to put the file in > * memory > + * @filesizep: Returns the file size in bytes > * Return 0 if OK, -ve on error > */ > pxe_getfile_func getfile; > @@ -105,6 +107,7 @@ struct pxe_context { > void *userdata; > bool allow_abs_path; > char *bootdir; > + ulong pxe_file_size; > }; > > /** > @@ -225,4 +228,12 @@ void pxe_destroy_ctx(struct pxe_context *ctx); > */ > int pxe_process(struct pxe_context *ctx, ulong pxefile_addr_r, bool prompt); > > +/** > + * pxe_get_file_size() - Read the value of the 'filesize' environment variable > + * > + * @sizep: Place to put the value > + * @return 0 if OK, -ENOENT if no such variable, -EINVAL if format is invalid > + */ > +int pxe_get_file_size(ulong *sizep); > + > #endif /* __PXE_UTILS_H */ > -- > 2.33.0.1079.g6e70778dc9-goog > Reviewed-by: Ramon Fried