[PATCH] cmd: sysboot: dont overwrite bootfile env

[Art Nikpal]

> 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 at chromium.org> wrote:
>
> 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