[PATCH] bootm: Fix flags used for bootargs string substitution
Commit <51bb33846ad2> introduced a feature of bootargs string substitution and changed a flag used in bootm_process_cmdline_env() call to be either true or false. With this flag value, condition in bootm_process_cmdline() `if (flags & BOOTM_CL_SUBST)` is never true and process_subst() is never called.
Signed-off-by: Piotr Kubik piotr.kubik@iopsys.eu --- boot/bootm.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/boot/bootm.c b/boot/bootm.c index 8f96a80d42..e96489e549 100644 --- a/boot/bootm.c +++ b/boot/bootm.c @@ -778,7 +778,8 @@ int do_bootm_states(struct cmd_tbl *cmdtp, int flag, int argc, if (!ret && (states & BOOTM_STATE_OS_BD_T)) ret = boot_fn(BOOTM_STATE_OS_BD_T, argc, argv, images); if (!ret && (states & BOOTM_STATE_OS_PREP)) { - ret = bootm_process_cmdline_env(images->os.os == IH_OS_LINUX); + ret = bootm_process_cmdline_env(images->os.os == IH_OS_LINUX ? + BOOTM_CL_ALL : 0); if (ret) { printf("Cmdline setup failed (err=%d)\n", ret); ret = CMD_RET_FAILURE;
On Tue, Oct 17, 2023 at 12:53:05PM +0200, Piotr Kubik wrote:
Commit <51bb33846ad2> introduced a feature of bootargs
Checkpatch will complain that this should be: In commit 51bb33846ad2 ("bootm: Support string substitution in bootargs") And this is the kind of thing I would fixup on applying if there was no other feedback. However:
string substitution and changed a flag used in bootm_process_cmdline_env() call to be either true or false. With this flag value, condition in bootm_process_cmdline() `if (flags & BOOTM_CL_SUBST)` is never true and process_subst() is never called.
Signed-off-by: Piotr Kubik piotr.kubik@iopsys.eu
boot/bootm.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/boot/bootm.c b/boot/bootm.c index 8f96a80d42..e96489e549 100644 --- a/boot/bootm.c +++ b/boot/bootm.c @@ -778,7 +778,8 @@ int do_bootm_states(struct cmd_tbl *cmdtp, int flag, int argc, if (!ret && (states & BOOTM_STATE_OS_BD_T)) ret = boot_fn(BOOTM_STATE_OS_BD_T, argc, argv, images); if (!ret && (states & BOOTM_STATE_OS_PREP)) {
ret = bootm_process_cmdline_env(images->os.os == IH_OS_LINUX);
ret = bootm_process_cmdline_env(images->os.os == IH_OS_LINUX ?BOOTM_CL_ALL : 0);
This gets hard to read. I would prefer a comment and something like: int flags = 0; if (images->os.os == IH_OS_LINUX) flags = BOOTM_CL_ALL; ret = bootm_process_cmdline_env(flags);
As the compiler should be just as smart, and that'll be clear about what's going on. Thanks.
Hi Piotr,
On Tue, 17 Oct 2023 at 10:35, Tom Rini trini@konsulko.com wrote:
On Tue, Oct 17, 2023 at 12:53:05PM +0200, Piotr Kubik wrote:
Commit <51bb33846ad2> introduced a feature of bootargs
Checkpatch will complain that this should be: In commit 51bb33846ad2 ("bootm: Support string substitution in bootargs") And this is the kind of thing I would fixup on applying if there was no other feedback. However:
string substitution and changed a flag used in bootm_process_cmdline_env() call to be either true or false. With this flag value, condition in bootm_process_cmdline() `if (flags & BOOTM_CL_SUBST)` is never true and process_subst() is never called.
Signed-off-by: Piotr Kubik piotr.kubik@iopsys.eu
boot/bootm.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/boot/bootm.c b/boot/bootm.c index 8f96a80d42..e96489e549 100644 --- a/boot/bootm.c +++ b/boot/bootm.c @@ -778,7 +778,8 @@ int do_bootm_states(struct cmd_tbl *cmdtp, int flag, int argc, if (!ret && (states & BOOTM_STATE_OS_BD_T)) ret = boot_fn(BOOTM_STATE_OS_BD_T, argc, argv, images); if (!ret && (states & BOOTM_STATE_OS_PREP)) {
ret = bootm_process_cmdline_env(images->os.os == IH_OS_LINUX);
ret = bootm_process_cmdline_env(images->os.os == IH_OS_LINUX ?BOOTM_CL_ALL : 0);
Oh wow that is a terrible bug. We have lots of test coverage of bootm_process_cmdline_env() and below, but bootm itself is still quite spotty.
I wonder if we could add something to run_fit_test to check this. We have lots of tests for bootm_process_cmdline_env() but not much of bootm itself. It might be possible to add just a few lines there, e.g. to set the bootargs to something and check what ends up in bootargs?
This gets hard to read. I would prefer a comment and something like: int flags = 0; if (images->os.os == IH_OS_LINUX) flags = BOOTM_CL_ALL; ret = bootm_process_cmdline_env(flags);
As the compiler should be just as smart, and that'll be clear about what's going on. Thanks.
Regards, Simon
Hi Simon, Tom Thanks for your feedback
On Tue, 17 Oct 2023 at 10:35, Tom Rini trini@konsulko.com wrote:
Checkpatch will complain that this should be: In commit 51bb33846ad2 ("bootm: Support string substitution in bootargs")
Actually it did not complain, but I'll fix.
On 18.10.2023 05:32, Simon Glass wrote: Oh wow that is a terrible bug. We have lots of test coverage of bootm_process_cmdline_env() and below, but bootm itself is still quite spotty.
I wonder if we could add something to run_fit_test to check this. We have lots of tests for bootm_process_cmdline_env() but not much of bootm itself. It might be possible to add just a few lines there, e.g. to set the bootargs to something and check what ends up in bootargs?
That's a good idea, I'll check
ret = bootm_process_cmdline_env(images->os.os == IH_OS_LINUX ?BOOTM_CL_ALL : 0);This gets hard to read. I would prefer a comment and something like: int flags = 0; if (images->os.os == IH_OS_LINUX) flags = BOOTM_CL_ALL; ret = bootm_process_cmdline_env(flags);
As the compiler should be just as smart, and that'll be clear about what's going on. Thanks.
Well, I followed previous way of coding this part and it was: ret = bootm_process_cmdline_env(images->os.os == IH_OS_LINUX ? BOOTM_CL_SILENT : 0);
But sure, I can update to make it more readable.
Regards, Piotr
Changes from v1: - fix commit message - improve code readability - add bootargs substitution test to test_fit.py
Piotr Kubik (1): bootm: Fix flags used for bootargs string substitution
boot/bootm.c | 6 +++++- test/py/tests/test_fit.py | 8 ++++++++ 2 files changed, 13 insertions(+), 1 deletion(-)
Commit 51bb33846ad2 ("bootm: Support string substitution in bootargs") introduced a feature of bootargs string substitution and changed a flag used in bootm_process_cmdline_env() call to be either true or false. With this flag value, condition in bootm_process_cmdline() `if (flags & BOOTM_CL_SUBST)` is never true and process_subst() is never called.
Add a simple test to verify if substitution works OK.
Signed-off-by: Piotr Kubik piotr.kubik@iopsys.eu --- boot/bootm.c | 6 +++++- test/py/tests/test_fit.py | 8 ++++++++ 2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/boot/bootm.c b/boot/bootm.c index cb61485c22..31b0431e04 100644 --- a/boot/bootm.c +++ b/boot/bootm.c @@ -852,7 +852,11 @@ int do_bootm_states(struct cmd_tbl *cmdtp, int flag, int argc, if (!ret && (states & BOOTM_STATE_OS_BD_T)) ret = boot_fn(BOOTM_STATE_OS_BD_T, argc, argv, images); if (!ret && (states & BOOTM_STATE_OS_PREP)) { - ret = bootm_process_cmdline_env(images->os.os == IH_OS_LINUX); + int flags = 0; + /* For Linux OS do all substitutions at console processing */ + if (images->os.os == IH_OS_LINUX) + flags = BOOTM_CL_ALL; + ret = bootm_process_cmdline_env(flags); if (ret) { printf("Cmdline setup failed (err=%d)\n", ret); ret = CMD_RET_FAILURE; diff --git a/test/py/tests/test_fit.py b/test/py/tests/test_fit.py index f45848484e..fed8cd58ce 100755 --- a/test/py/tests/test_fit.py +++ b/test/py/tests/test_fit.py @@ -339,6 +339,14 @@ def test_fit(u_boot_console): 'U-Boot loaded FDT from offset %#x, FDT is actually at %#x' % (fit_offset, real_fit_offset))
+ # Check if bootargs strings substitution works + output = cons.run_command_list([ + 'env set bootargs \"'my_boot_var=${foo}'\"', + 'env set foo bar', + 'bootm prep', + 'env print bootargs']) + assert 'bootargs="my_boot_var=bar"' in output, "Bootargs strings not substituted" + # Now a kernel and an FDT with cons.log.section('Kernel + FDT load'): params['fdt_load'] = 'load = <%#x>;' % params['fdt_addr']
On Fri, 24 Nov 2023 at 09:35, Piotr Kubik piotr.kubik@iopsys.eu wrote:
Commit 51bb33846ad2 ("bootm: Support string substitution in bootargs") introduced a feature of bootargs string substitution and changed a flag used in bootm_process_cmdline_env() call to be either true or false. With this flag value, condition in bootm_process_cmdline() `if (flags & BOOTM_CL_SUBST)` is never true and process_subst() is never called.
Add a simple test to verify if substitution works OK.
Signed-off-by: Piotr Kubik piotr.kubik@iopsys.eu
boot/bootm.c | 6 +++++- test/py/tests/test_fit.py | 8 ++++++++ 2 files changed, 13 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
Thank you
On Fri, Nov 24, 2023 at 05:30:46PM +0100, Piotr Kubik wrote:
Commit 51bb33846ad2 ("bootm: Support string substitution in bootargs") introduced a feature of bootargs string substitution and changed a flag used in bootm_process_cmdline_env() call to be either true or false. With this flag value, condition in bootm_process_cmdline() `if (flags & BOOTM_CL_SUBST)` is never true and process_subst() is never called.
Add a simple test to verify if substitution works OK.
Signed-off-by: Piotr Kubik piotr.kubik@iopsys.eu Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/next, thanks!
participants (3)
-
Piotr Kubik -
Simon Glass -
Tom Rini