[PATCH v4 0/4] cmd: env: add option for quiet output on env info
Hi,
It is a V4 for [1] serie.
I add the -q option for 'env info' command and I also add pytest for this command.
Test for ENV_IS_IN_DEVICE is included in separate serie [2] (I will activate ENV_IS_IN_EXT4 support in sandbox)
To avoid compilation warning, I add prototype for env_get_location for the patch 3/7 "cmd: env: check real location for env info command" as it is done in [3].
[1] http://patchwork.ozlabs.org/project/uboot/list/?series=158105 [2] http://patchwork.ozlabs.org/project/uboot/list/?series=158160 [3] http://patchwork.ozlabs.org/patch/1230200/
Changes in v4: - rebase on master branch - move 5/7 stm32mp1: configs: activate CMD_ERASEENV in a new serie 183380 - move 2/7 and 4/7 in a new serie 183387
Changes in v3: - update commit message (sub-commandi) - rename test_env_info_test to test_env_info_quiet
Changes in v2: - update prototype in env_internal.h as done in "env: add prototypes for weak function" - remove comment change in env.c (implementation information) - move env_location declaration - activate env info command in sandbox (new) - add pytest test_env_info and test_env_info_test (new)
Patrick Delaunay (4): cmd: env: add option for quiet output on env info cmd: env: check real location for env info command configs: sandbox: Enable sub command 'env info' test: env: add test for env info sub-command
cmd/Kconfig | 1 + cmd/nvedit.c | 37 +++++++++++++++++++------ configs/sandbox64_defconfig | 1 + configs/sandbox_defconfig | 1 + configs/sandbox_flattree_defconfig | 1 + configs/sandbox_spl_defconfig | 1 + include/env_internal.h | 11 ++++++++ test/py/tests/test_env.py | 44 ++++++++++++++++++++++++++++++ 8 files changed, 89 insertions(+), 8 deletions(-)
The "env info" can be use for test with -d and -p parameter, in scripting case the output of the command is not needed.
This patch allows to deactivate this output with a new option "-q".
For example, we can save the environment if default environment is used and persistent storage is managed with: if env info -p -d -q; then env save; fi
Without the quiet option, I have the unnecessary traces First boot: Default environment is used Environment can be persisted Saving Environment to EXT4... File System is consistent
Next boot: Environment was loaded from persistent storage Environment can be persisted
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com Reviewed-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
cmd/Kconfig | 1 + cmd/nvedit.c | 26 +++++++++++++++++++------- 2 files changed, 20 insertions(+), 7 deletions(-)
diff --git a/cmd/Kconfig b/cmd/Kconfig index 192b3b262f..1de57988ae 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -607,6 +607,7 @@ config CMD_NVEDIT_INFO This command can be optionally used for evaluation in scripts: [-d] : evaluate whether default environment is used [-p] : evaluate whether environment can be persisted + [-q] : quiet output The result of multiple evaluations will be combined with AND.
endmenu diff --git a/cmd/nvedit.c b/cmd/nvedit.c index 49338b4d36..68cb1a4a8f 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -1224,12 +1224,15 @@ static int print_env_info(void) * env info - display environment information * env info [-d] - evaluate whether default environment is used * env info [-p] - evaluate whether environment can be persisted + * Add [-q] - quiet mode, use only for command result, for test by example: + * test env info -p -d -q */ static int do_env_info(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { int eval_flags = 0; int eval_results = 0; + bool quiet = false;
/* display environment information */ if (argc <= 1) @@ -1247,6 +1250,9 @@ static int do_env_info(struct cmd_tbl *cmdtp, int flag, case 'p': eval_flags |= ENV_INFO_IS_PERSISTED; break; + case 'q': + quiet = true; + break; default: return CMD_RET_USAGE; } @@ -1256,20 +1262,24 @@ static int do_env_info(struct cmd_tbl *cmdtp, int flag, /* evaluate whether default environment is used */ if (eval_flags & ENV_INFO_IS_DEFAULT) { if (gd->flags & GD_FLG_ENV_DEFAULT) { - printf("Default environment is used\n"); + if (!quiet) + printf("Default environment is used\n"); eval_results |= ENV_INFO_IS_DEFAULT; } else { - printf("Environment was loaded from persistent storage\n"); + if (!quiet) + printf("Environment was loaded from persistent storage\n"); } }
/* evaluate whether environment can be persisted */ if (eval_flags & ENV_INFO_IS_PERSISTED) { #if defined(CONFIG_CMD_SAVEENV) && defined(ENV_IS_IN_DEVICE) - printf("Environment can be persisted\n"); + if (!quiet) + printf("Environment can be persisted\n"); eval_results |= ENV_INFO_IS_PERSISTED; #else - printf("Environment cannot be persisted\n"); + if (!quiet) + printf("Environment cannot be persisted\n"); #endif }
@@ -1326,7 +1336,7 @@ static struct cmd_tbl cmd_env_sub[] = { U_BOOT_CMD_MKENT(import, 5, 0, do_env_import, "", ""), #endif #if defined(CONFIG_CMD_NVEDIT_INFO) - U_BOOT_CMD_MKENT(info, 2, 0, do_env_info, "", ""), + U_BOOT_CMD_MKENT(info, 3, 0, do_env_info, "", ""), #endif U_BOOT_CMD_MKENT(print, CONFIG_SYS_MAXARGS, 1, do_env_print, "", ""), #if defined(CONFIG_CMD_RUN) @@ -1405,8 +1415,10 @@ static char env_help_text[] = #endif #if defined(CONFIG_CMD_NVEDIT_INFO) "env info - display environment information\n" - "env info [-d] - whether default environment is used\n" - "env info [-p] - whether environment can be persisted\n" + "env info [-d] [-p] [-q] - evaluate environment information\n" + " "-d": default environment is used\n" + " "-p": environment can be persisted\n" + " "-q": quiet output\n" #endif "env print [-a | name ...] - print environment\n" #if defined(CONFIG_CMD_NVEDIT_EFI)
Check the current ENV location, dynamically provided by the weak function env_get_location to be sure that the environment can be persistent.
The compilation flag ENV_IS_IN_DEVICE is not enough when the board dynamically select the available storage location (according boot device for example).
This patch solves issue for stm32mp1 platform, when the boot device is USB.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com Reviewed-by: Simon Glass sjg@chromium.org ---
(no changes since v2)
Changes in v2: - update prototype in env_internal.h as done in "env: add prototypes for weak function" - remove comment change in env.c (implementation information) - move env_location declaration
cmd/nvedit.c | 15 ++++++++++++--- include/env_internal.h | 11 +++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c index 68cb1a4a8f..0f9cea96f3 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -1233,6 +1233,9 @@ static int do_env_info(struct cmd_tbl *cmdtp, int flag, int eval_flags = 0; int eval_results = 0; bool quiet = false; +#if defined(CONFIG_CMD_SAVEENV) && defined(ENV_IS_IN_DEVICE) + enum env_location loc; +#endif
/* display environment information */ if (argc <= 1) @@ -1274,9 +1277,15 @@ static int do_env_info(struct cmd_tbl *cmdtp, int flag, /* evaluate whether environment can be persisted */ if (eval_flags & ENV_INFO_IS_PERSISTED) { #if defined(CONFIG_CMD_SAVEENV) && defined(ENV_IS_IN_DEVICE) - if (!quiet) - printf("Environment can be persisted\n"); - eval_results |= ENV_INFO_IS_PERSISTED; + loc = env_get_location(ENVOP_SAVE, gd->env_load_prio); + if (ENVL_NOWHERE != loc && ENVL_UNKNOWN != loc) { + if (!quiet) + printf("Environment can be persisted\n"); + eval_results |= ENV_INFO_IS_PERSISTED; + } else { + if (!quiet) + printf("Environment cannot be persisted\n"); + } #else if (!quiet) printf("Environment cannot be persisted\n"); diff --git a/include/env_internal.h b/include/env_internal.h index e89fbdb1b7..66550434c3 100644 --- a/include/env_internal.h +++ b/include/env_internal.h @@ -211,6 +211,17 @@ struct env_driver {
extern struct hsearch_data env_htab;
+/** + * env_get_location()- Provide the best location for the U-Boot environment + * + * It is a weak function allowing board to overidde the environment location + * + * @op: operations performed on the environment + * @prio: priority between the multiple environments, 0 being the + * highest priority + * @return an enum env_location value on success, or -ve error code. + */ +enum env_location env_get_location(enum env_operation op, int prio); #endif /* DO_DEPS_ONLY */
#endif /* _ENV_INTERNAL_H_ */
Enable support for sub command 'env info' in sandbox with CONFIG_CMD_NVEDIT_INFO. This is aimed primarily at adding unit test.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com Reviewed-by: Simon Glass sjg@chromium.org ---
(no changes since v2)
Changes in v2: - activate env info command in sandbox (new)
configs/sandbox64_defconfig | 1 + configs/sandbox_defconfig | 1 + configs/sandbox_flattree_defconfig | 1 + configs/sandbox_spl_defconfig | 1 + 4 files changed, 4 insertions(+)
diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig index a3f049e124..a2410b3368 100644 --- a/configs/sandbox64_defconfig +++ b/configs/sandbox64_defconfig @@ -30,6 +30,7 @@ CONFIG_CMD_GREPENV=y CONFIG_CMD_ENV_CALLBACK=y CONFIG_CMD_ENV_FLAGS=y CONFIG_CMD_NVEDIT_EFI=y +CONFIG_CMD_NVEDIT_INFO=y CONFIG_LOOPW=y CONFIG_CMD_MD5SUM=y CONFIG_CMD_MEMINFO=y diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig index 5b7569319b..0c9e0b9f1f 100644 --- a/configs/sandbox_defconfig +++ b/configs/sandbox_defconfig @@ -34,6 +34,7 @@ CONFIG_CMD_GREPENV=y CONFIG_CMD_ENV_CALLBACK=y CONFIG_CMD_ENV_FLAGS=y CONFIG_CMD_NVEDIT_EFI=y +CONFIG_CMD_NVEDIT_INFO=y CONFIG_LOOPW=y CONFIG_CMD_MD5SUM=y CONFIG_CMD_MEMINFO=y diff --git a/configs/sandbox_flattree_defconfig b/configs/sandbox_flattree_defconfig index 21f9047046..bc4819f998 100644 --- a/configs/sandbox_flattree_defconfig +++ b/configs/sandbox_flattree_defconfig @@ -24,6 +24,7 @@ CONFIG_CMD_BOOTEFI_HELLO=y # CONFIG_CMD_ELF is not set CONFIG_CMD_ASKENV=y CONFIG_CMD_GREPENV=y +CONFIG_CMD_NVEDIT_INFO=y CONFIG_LOOPW=y CONFIG_CMD_MD5SUM=y CONFIG_CMD_MEMINFO=y diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig index fc8b26e88c..64f2ed5102 100644 --- a/configs/sandbox_spl_defconfig +++ b/configs/sandbox_spl_defconfig @@ -34,6 +34,7 @@ CONFIG_CMD_ASKENV=y CONFIG_CMD_GREPENV=y CONFIG_CMD_ENV_CALLBACK=y CONFIG_CMD_ENV_FLAGS=y +CONFIG_CMD_NVEDIT_INFO=y CONFIG_LOOPW=y CONFIG_CMD_MD5SUM=y CONFIG_CMD_MEMINFO=y
Add a pytest for testing the env info sub-command:
test_env_info: test command with several option
test_env_info_quiet: test the result of the sub-command with quiet option, '-q' as used for support in shell test; for example: if env info -p -d -q; then env save; fi
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com ---
Changes in v4: - rebase on master branch - move 5/7 stm32mp1: configs: activate CMD_ERASEENV in a new serie 183380 - move 2/7 and 4/7 in a new serie 183387
Changes in v3: - update commit message (sub-commandi) - rename test_env_info_test to test_env_info_quiet
Changes in v2: - add pytest test_env_info and test_env_info_test (new)
test/py/tests/test_env.py | 44 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)
diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py index 6ff38f1020..cbdb41031c 100644 --- a/test/py/tests/test_env.py +++ b/test/py/tests/test_env.py @@ -336,3 +336,47 @@ def test_env_import_whitelist_delete(state_test_env): unset_var(state_test_env, 'foo2') unset_var(state_test_env, 'foo3') unset_var(state_test_env, 'foo4') + +@pytest.mark.boardspec('sandbox') +@pytest.mark.buildconfigspec('cmd_nvedit_info') +def test_env_info(state_test_env): + + """Test 'env info' command with several options. + """ + c = state_test_env.u_boot_console + + response = c.run_command('env info') + assert 'env_valid = invalid' in response + assert 'env_ready = true' in response + assert 'env_use_default = true' in response + + response = c.run_command('env info -p -d') + assert 'Default environment is used' in response + assert 'Environment cannot be persisted' in response + + response = c.run_command('env info -p -d -q') + assert response == "" + +@pytest.mark.boardspec('sandbox') +@pytest.mark.buildconfigspec('cmd_nvedit_info') +@pytest.mark.buildconfigspec('cmd_echo') +def test_env_info_quiet(state_test_env): + + """Test 'env info' quiet command result with several options for test. + """ + c = state_test_env.u_boot_console + + response = c.run_command('env info -d -q') + assert response == "" + response = c.run_command('echo $?') + assert response == "0" + + response = c.run_command('env info -p -q') + assert response == "" + response = c.run_command('echo $?') + assert response == "1" + + response = c.run_command('env info -d -p -q') + assert response == "" + response = c.run_command('echo $?') + assert response == "1"
On 6/15/20 8:01 AM, Patrick Delaunay wrote:
Add a pytest for testing the env info sub-command:
test_env_info: test command with several option
test_env_info_quiet: test the result of the sub-command with quiet option, '-q' as used for support in shell test; for example: if env info -p -d -q; then env save; fi
diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py
+@pytest.mark.boardspec('sandbox') +@pytest.mark.buildconfigspec('cmd_nvedit_info') +def test_env_info(state_test_env):
The body of these tests doesn't look like it tests something that's specific to sandbox, so I'm not sure why the test function is marked to only run on sandbox. Is it simply because other boards may store the environment differently and/or have valid saved environment in flash, so the responses to e.g. "env info" aren't the same everywhere? If so, I imagine that test_env_info_quiet() doesn't need to be sandbox-only, since there's no output in that case.
Hi Stephen,
From: Stephen Warren swarren@wwwdotorg.org Sent: mardi 16 juin 2020 00:09
On 6/15/20 8:01 AM, Patrick Delaunay wrote:
Add a pytest for testing the env info sub-command:
test_env_info: test command with several option
test_env_info_quiet: test the result of the sub-command with quiet option, '-q' as used for support in shell test; for example: if env info -p -d -q; then env save; fi
diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py
+@pytest.mark.boardspec('sandbox') +@pytest.mark.buildconfigspec('cmd_nvedit_info') +def test_env_info(state_test_env):
The body of these tests doesn't look like it tests something that's specific to sandbox, so I'm not sure why the test function is marked to only run on sandbox. Is it simply because other boards may store the environment differently and/or have valid saved environment in flash, so the responses to e.g. "env info" aren't the same everywhere? If so, I imagine that test_env_info_quiet() doesn't need to be sandbox-only, since there's no output in that case.
The test is not really sandbox specific but I don't have easy way to know on real board the ENV configuration (for the resut of command env info -p -d).
In the test, I assume that at least CONFIG_ENV_IS_.... is activated (for persistent storage) and if this target is selected in the weak function env_get_location. And "env save" as be not be executed (default environment is used).
And with quiet option, the test the environment if is persistent (result of "env -p -q" is 0) or using default ("env -d -q" result is 0).
And in the next patch http://patchwork.ozlabs.org/project/uboot/patch/20200616074048.7898-10-patri...
As the command "env erase" is not always supported according he environment target.
I could test on real hardware but I need to check if I test all the possible result.
Patrick
On 6/16/20 2:01 AM, Patrick DELAUNAY wrote:
Hi Stephen,
From: Stephen Warren swarren@wwwdotorg.org Sent: mardi 16 juin 2020 00:09
On 6/15/20 8:01 AM, Patrick Delaunay wrote:
Add a pytest for testing the env info sub-command:
test_env_info: test command with several option
test_env_info_quiet: test the result of the sub-command with quiet option, '-q' as used for support in shell test; for example: if env info -p -d -q; then env save; fi
diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py
+@pytest.mark.boardspec('sandbox') +@pytest.mark.buildconfigspec('cmd_nvedit_info') +def test_env_info(state_test_env):
The body of these tests doesn't look like it tests something that's specific to sandbox, so I'm not sure why the test function is marked to only run on sandbox. Is it simply because other boards may store the environment differently and/or have valid saved environment in flash, so the responses to e.g. "env info" aren't the same everywhere? If so, I imagine that test_env_info_quiet() doesn't need to be sandbox-only, since there's no output in that case.
The test is not really sandbox specific but I don't have easy way to know on real board the ENV configuration (for the resut of command env info -p -d).
In the test, I assume that at least CONFIG_ENV_IS_.... is activated (for persistent storage) and if this target is selected in the weak function env_get_location. And "env save" as be not be executed (default environment is used).
And with quiet option, the test the environment if is persistent (result of "env -p -q" is 0) or using default ("env -d -q" result is 0).
And in the next patch http://patchwork.ozlabs.org/project/uboot/patch/20200616074048.7898-10-patri...
As the command "env erase" is not always supported according he environment target.
I could test on real hardware but I need to check if I test all the possible result.
OK, I guess that makes sense for a start.
For testing on real HW, the typical approach would be to require that the board's test configuration define some env__xxx variables that define its capabilities. Then, the test can be made to depend on those values, or whether those variables are set at all.
Hi Stephen,
From: Stephen Warren swarren@wwwdotorg.org Sent: jeudi 18 juin 2020 00:32
On 6/16/20 2:01 AM, Patrick DELAUNAY wrote:
Hi Stephen,
From: Stephen Warren swarren@wwwdotorg.org Sent: mardi 16 juin 2020 00:09
On 6/15/20 8:01 AM, Patrick Delaunay wrote:
Add a pytest for testing the env info sub-command:
test_env_info: test command with several option
test_env_info_quiet: test the result of the sub-command with quiet option, '-q' as used for support in shell test; for example: if env info -p -d -q; then env save; fi
diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py
+@pytest.mark.boardspec('sandbox') +@pytest.mark.buildconfigspec('cmd_nvedit_info') +def test_env_info(state_test_env):
The body of these tests doesn't look like it tests something that's specific to sandbox, so I'm not sure why the test function is marked to only run
on sandbox.
Is it simply because other boards may store the environment differently and/or have valid saved environment in flash, so the responses to e.g. "env info" aren't the same everywhere? If so, I imagine that test_env_info_quiet() doesn't need to be sandbox-only, since
there's no output in that case.
The test is not really sandbox specific but I don't have easy way to know on real board the ENV configuration (for the resut of command env info -p
-d).
In the test, I assume that at least CONFIG_ENV_IS_.... is activated (for persistent storage) and if this target is selected in the weak function
env_get_location.
And "env save" as be not be executed (default environment is used).
And with quiet option, the test the environment if is persistent (result of "env -p -q" is 0) or using default ("env -d -q" result is 0).
And in the next patch http://patchwork.ozlabs.org/project/uboot/patch/20200616074048.7898-10 -patrick.delaunay@st.com/
As the command "env erase" is not always supported according he environment
target.
I could test on real hardware but I need to check if I test all the possible result.
OK, I guess that makes sense for a start.
But I will propose a V5 to check command on real hardware Just modify test_env_info to check the all possible strings.
For testing on real HW, the typical approach would be to require that the board's test configuration define some env__xxx variables that define its capabilities. Then, the test can be made to depend on those values, or whether those variables are set at all.
For the next steps, I need to thinks about tests because ENV location is not only impacted by CONFIG_ENV_IS_IN_.... or CONFIG_ENV_IS_NOWHERE
These defined can be activated simultaneously and env location is detected at run time: so it is difficult to predict the 'env info' result on real hardware.
On sandbox it is fixed because ENVL_NOWHERE is selected by default.
Patrick
participants (3)
-
Patrick DELAUNAY -
Patrick Delaunay
-
Stephen Warren