[v3,16/18] pxe: Refactor sysboot to have one helper

The only difference between the three helpers is the filesystem type.
Factor this out and call the filesystem functions directly, instead of
through the command-line interpreter. This allows the file size to be
obtained directly, instead of via an environment variable.

We cannot do the same thing with PXE's tftpboot since there is no API
at present to obtain information about the file that was read. So there
is no point in changing pxe_getfile_func to use a ulong for the address,
for example.

This is as far as the refactoring can go for the present.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 cmd/sysboot.c | 94 ++++++++++++++++++++-------------------------------
 1 file changed, 36 insertions(+), 58 deletions(-)

OK is clear

Reviewed and Tested on -master Mon 18 Oct 2021 04:40:29 PM CST
Reviewed-by: Artem Lapkin <email2tema@gmail.com>
Tested-by:  Artem Lapkin <email2tema@gmail.com>

On Thu, Oct 14, 2021 at 9:51 PM Simon Glass <sjg@chromium.org> wrote:
>
> The only difference between the three helpers is the filesystem type.
> Factor this out and call the filesystem functions directly, instead of
> through the command-line interpreter. This allows the file size to be
> obtained directly, instead of via an environment variable.
>
> We cannot do the same thing with PXE's tftpboot since there is no API
> at present to obtain information about the file that was read. So there
> is no point in changing pxe_getfile_func to use a ulong for the address,
> for example.
>
> This is as far as the refactoring can go for the present.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v1)
>
>  cmd/sysboot.c | 94 ++++++++++++++++++++-------------------------------
>  1 file changed, 36 insertions(+), 58 deletions(-)
>
> diff --git a/cmd/sysboot.c b/cmd/sysboot.c
> index 6344ecd357b..04c07020269 100644
> --- a/cmd/sysboot.c
> +++ b/cmd/sysboot.c
> @@ -6,63 +6,40 @@
>  #include <fs.h>
>  #include <pxe_utils.h>
>
> -static char *fs_argv[5];
> -
> -static int do_get_ext2(struct pxe_context *ctx, const char *file_path,
> -                      char *file_addr, ulong *sizep)
> +/**
> + * struct sysboot_info - useful information for sysboot helpers
> + *
> + * @fstype: Filesystem type (FS_TYPE_...)
> + * @ifname: Interface name (e.g. "ide", "scsi")
> + * @dev_part_str is in the format:
> + *     <dev>.<hw_part>:<part> where <dev> is the device number,
> + *     <hw_part> is the optional hardware partition number and
> + *     <part> is the partition number
> + */
> +struct sysboot_info {
> +       int fstype;
> +       const char *ifname;
> +       const char *dev_part_str;
> +};
> +
> +static int sysboot_read_file(struct pxe_context *ctx, const char *file_path,
> +                            char *file_addr, ulong *sizep)
>  {
> -#ifdef CONFIG_CMD_EXT2
> +       struct sysboot_info *info = ctx->userdata;
> +       loff_t len_read;
> +       ulong addr;
>         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);
> +       addr = simple_strtoul(file_addr, NULL, 16);
> +       ret = fs_set_blk_dev(info->ifname, info->dev_part_str, info->fstype);
>         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, 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);
> +               return ret;
> +       ret = fs_read(file_path, addr, 0, 0, &len_read);
>         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, ulong *sizep)
> -{
> -#ifdef CONFIG_CMD_FS_GENERIC
> -       int ret;
> -
> -       fs_argv[0] = "load";
> -       fs_argv[3] = file_addr;
> -       fs_argv[4] = (void *)file_path;
> +               return ret;
> +       *sizep = len_read;
>
> -       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;
> +       return 0;
>  }
>
>  /*
> @@ -74,9 +51,9 @@ static int do_sysboot(struct cmd_tbl *cmdtp, int flag, int argc,
>                       char *const argv[])
>  {
>         unsigned long pxefile_addr_r;
> -       pxe_getfile_func getfile;
>         struct pxe_context ctx;
>         char *pxefile_addr_str;
> +       struct sysboot_info info;
>         char *filename;
>         int prompt = 0;
>         int ret;
> @@ -106,24 +83,25 @@ static int do_sysboot(struct cmd_tbl *cmdtp, int flag, int argc,
>         }
>
>         if (strstr(argv[3], "ext2")) {
> -               getfile = do_get_ext2;
> +               info.fstype = FS_TYPE_EXT;
>         } else if (strstr(argv[3], "fat")) {
> -               getfile = do_get_fat;
> +               info.fstype = FS_TYPE_FAT;
>         } else if (strstr(argv[3], "any")) {
> -               getfile = do_get_any;
> +               info.fstype = FS_TYPE_ANY;
>         } else {
>                 printf("Invalid filesystem: %s\n", argv[3]);
>                 return 1;
>         }
> -       fs_argv[1] = argv[1];
> -       fs_argv[2] = argv[2];
> +       info.ifname = argv[1];
> +       info.dev_part_str = argv[2];
>
>         if (strict_strtoul(pxefile_addr_str, 16, &pxefile_addr_r) < 0) {
>                 printf("Invalid pxefile address: %s\n", pxefile_addr_str);
>                 return 1;
>         }
>
> -       if (pxe_setup_ctx(&ctx, cmdtp, getfile, NULL, true, filename)) {
> +       if (pxe_setup_ctx(&ctx, cmdtp, sysboot_read_file, &info, true,
> +                         filename)) {
>                 printf("Out of memory\n");
>                 return CMD_RET_FAILURE;
>         }
> --
> 2.33.0.1079.g6e70778dc9-goog
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

On Thu, Oct 14, 2021 at 12:48:09PM -0600, Simon Glass wrote:

> The only difference between the three helpers is the filesystem type.
> Factor this out and call the filesystem functions directly, instead of
> through the command-line interpreter. This allows the file size to be
> obtained directly, instead of via an environment variable.
> 
> We cannot do the same thing with PXE's tftpboot since there is no API
> at present to obtain information about the file that was read. So there
> is no point in changing pxe_getfile_func to use a ulong for the address,
> for example.
> 
> This is as far as the refactoring can go for the present.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Artem Lapkin <email2tema@gmail.com>
> Tested-by:  Artem Lapkin <email2tema@gmail.com>
> Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Applied to u-boot/master, thanks!