[PATCH] cmd: sysboot: dont overwrite bootfile env

[Simon Glass]

Hi,

On Tue, 12 Oct 2021 at 21:39, Artem Lapkin <email2tema at gmail.com> wrote:
>
> Problem
>
> PXE cannot boot normally after Sysboot changed the bootfile env (called
> from boot_extlinux) from the default "boot.scr.uimg" to
> "/boot/extlinux/extlinux.conf".
>
> In addition, an unbootable extlinux configuration will also make the PXE
> boot unbootable, because it will use the incorrect path "/boot/extlinux/"
> from the bootfile env.
>
> Solution
>
> sysboot must care about bootfile and restore default value after usage.
>
> Come from:
> https://patchwork.ozlabs.org/project/uboot/patch/20211012085544.3206394-1-art@khadas.com/
>
> Signed-off-by: Artem Lapkin <art at khadas.com>
> ---
>  cmd/sysboot.c | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)

Please also see this refactor which conflicts with this patch:

http://patchwork.ozlabs.org/project/uboot/list/?series=264265

I think that series should be reviewed/applied first since it was sent
in August.

>
> diff --git a/cmd/sysboot.c b/cmd/sysboot.c
> index af6a2f1b7f..99b11cc127 100644
> --- a/cmd/sysboot.c
> +++ b/cmd/sysboot.c
> @@ -2,6 +2,7 @@
>
>  #include <common.h>
>  #include <command.h>
> +#include <malloc.h>
>  #include <env.h>
>  #include <fs.h>
>  #include "pxe_utils.h"
> @@ -61,8 +62,9 @@ static int do_sysboot(struct cmd_tbl *cmdtp, int flag, int argc,
>         unsigned long pxefile_addr_r;
>         struct pxe_menu *cfg;
>         char *pxefile_addr_str;
> -       char *filename;
> +       char *filename, *filename_bak;

Can we see filename_bak to NULL so we can simply the free() below?

>         int prompt = 0;
> +       int ret = 0;
>
>         is_pxe = false;
>
> @@ -83,9 +85,10 @@ static int do_sysboot(struct cmd_tbl *cmdtp, int flag, int argc,
>                 pxefile_addr_str = argv[4];
>         }
>
> -       if (argc < 6) {
> -               filename = env_get("bootfile");
> -       } else {
> +       filename = env_get("bootfile");
> +       if (argc > 5) {
> +               filename_bak = malloc(strlen(filename) + 1);
> +               strcpy(filename_bak, filename);
>                 filename = argv[5];
>                 env_set("bootfile", filename);
>         }
> @@ -98,26 +101,26 @@ static int do_sysboot(struct cmd_tbl *cmdtp, int flag, int argc,
>                 do_getfile = do_get_any;
>         } else {
>                 printf("Invalid filesystem: %s\n", argv[3]);
> -               return 1;
> +               goto err;
>         }
>         fs_argv[1] = argv[1];
>         fs_argv[2] = argv[2];
>
>         if (strict_strtoul(pxefile_addr_str, 16, &pxefile_addr_r) < 0) {
>                 printf("Invalid pxefile address: %s\n", pxefile_addr_str);
> -               return 1;
> +               goto err;
>         }
>
>         if (get_pxe_file(cmdtp, filename, pxefile_addr_r) < 0) {
>                 printf("Error reading config file\n");
> -               return 1;
> +               goto err;
>         }
>
>         cfg = parse_pxefile(cmdtp, pxefile_addr_r);
>
>         if (!cfg) {
>                 printf("Error parsing config file\n");
> -               return 1;
> +               goto err;
>         }
>
>         if (prompt)
> @@ -126,8 +129,15 @@ static int do_sysboot(struct cmd_tbl *cmdtp, int flag, int argc,
>         handle_pxe_menu(cmdtp, cfg);
>
>         destroy_pxe_menu(cfg);
> -
> -       return 0;
> +       goto ret;

This is a bit ugly. Could we set 'ret' to 1 in the error cases above,
or set it to 1 at the top ad 0 here, or pull the parsing code into a
function...?

> + err:
> +       ret = 1;
> + ret:
> +       if (filename_bak) {
> +               env_set("bootfile", filename_bak);
> +               free(filename_bak);
> +       }
> +       return ret;
>  }
>
>  U_BOOT_CMD(sysboot, 7, 1, do_sysboot,
> --
> 2.25.1
>

Regards,
Simon