Hi, On Tue, 12 Oct 2021 at 21:39, Artem Lapkin <[email protected]> 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/[email protected]/ > > Signed-off-by: Artem Lapkin <[email protected]> > --- > 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
