[PATCH v4 0/4] cmd: env: add option for quiet output on env info

[PATCH v4 0/4] cmd: env: add option for quiet output on env info

[Patrick Delaunay]

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(-)

-- 
2.17.1

[PATCH v4 1/4] cmd: env: add option for quiet output on env info

[Patrick Delaunay]

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)
-- 
2.17.1

[PATCH v4 2/4] cmd: env: check real location for env info command

[Patrick Delaunay]

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_ */
-- 
2.17.1

[PATCH v4 3/4] configs: sandbox: Enable sub command 'env info'

[Patrick Delaunay]

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
-- 
2.17.1

[PATCH v4 4/4] test: env: add test for env info sub-command

[Patrick Delaunay]

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')
+
+ at pytest.mark.boardspec('sandbox')
+ at 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 == ""
+
+ at pytest.mark.boardspec('sandbox')
+ at pytest.mark.buildconfigspec('cmd_nvedit_info')
+ at 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"
-- 
2.17.1

[PATCH v4 4/4] test: env: add test for env info sub-command

[Stephen Warren]

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

> + at pytest.mark.boardspec('sandbox')
> + at 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.

[PATCH v4 4/4] test: env: add test for env info sub-command

[Patrick DELAUNAY]

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
> 
> > + at pytest.mark.boardspec('sandbox')
> > + at 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 at 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.

Patrick

[PATCH v4 4/4] test: env: add test for env info sub-command

[Stephen Warren]

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
>>
>>> + at pytest.mark.boardspec('sandbox')
>>> + at 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 at 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.

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.

[PATCH v4 4/4] test: env: add test for env info sub-command

[Patrick DELAUNAY]

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
> >>
> >>> + at pytest.mark.boardspec('sandbox')
> >>> + at 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 at 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