Re: [PATCH] cmd: sysboot: dont overwrite bootfile env

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.

yes ! i think need update your series because cant apply it for current uboot state

On Thu, Oct 14, 2021 at 12:58 AM Simon Glass sjg@chromium.org wrote:

Hi,

On Tue, 12 Oct 2021 at 21:39, Artem Lapkin email2tema@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-ar...

Signed-off-by: Artem Lapkin art@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