| Message ID | 20211013033912.3297227-1-art@khadas.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | cmd: sysboot: dont overwrite bootfile env | expand |
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-art@khadas.com/ > > 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
> 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-art@khadas.com/ > > > > 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
Hi Art, On Wed, 13 Oct 2021 at 22:34, Art Nikpal <email2tema@gmail.com> wrote: > > > 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 OK I am not sure why that is, but I will rebase and send v3. [..] Regards, Simon
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; 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; + 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,
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@khadas.com> --- cmd/sysboot.c | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-)