[PATCH] env: remove vars that are not in default env

current env_set_default_vars() doesn't delete var that are not in the imported env. hashtable removes vars that are not in the imported env but present in the current env only if H_NOCLEAR flag is not set.
This change is to avoid passing H_NOCLEAR flag if specific vars are passed to env_set_default_vars()
Test:
Without this change: Marvell>> env default boot_mode Marvell>>
With the change: Marvell>> env default boot_mode WARNING: 'boot_mode' not in imported env, deleting it!
Signed-off-by: rminnikanti rminnikanti@marvell.com --- env/common.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/env/common.c b/env/common.c index 8d47d72605..2f783e3a69 100644 --- a/env/common.c +++ b/env/common.c @@ -401,7 +401,15 @@ int env_set_default_vars(int nvars, char * const vars[], int flags) * Special use-case: import from default environment * (and use \0 as a separator) */ - flags |= H_NOCLEAR | H_DEFAULT; + + /* + * When vars are passed remove variables that are not in + * the default environment. + */ + if (!nvars) + flags |= H_NOCLEAR; + + flags |= H_DEFAULT; return himport_r(&env_htab, default_environment, sizeof(default_environment), '\0', flags, 0, nvars, vars);

Hi Ravi,
On Fri, 9 Aug 2024 at 09:38, Ravi Minnikanti rminnikanti@marvell.com wrote:
current env_set_default_vars() doesn't delete var that are not in the imported env. hashtable removes vars that are not in the imported env but present in the current env only if H_NOCLEAR flag is not set.
This change is to avoid passing H_NOCLEAR flag if specific vars are passed to env_set_default_vars()
Test:
Without this change: Marvell>> env default boot_mode Marvell>>
With the change: Marvell>> env default boot_mode WARNING: 'boot_mode' not in imported env, deleting it!
Signed-off-by: rminnikanti rminnikanti@marvell.com
env/common.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
Could you add to usage/environment.rst and also update the tests for this - test/env/ - I suppose your patch fixes a bug, but it is hard to figure out what it is supposed to do from the docs.
See for example dm_test_acpi_cmd_set() for how to check output from a command.
diff --git a/env/common.c b/env/common.c index 8d47d72605..2f783e3a69 100644 --- a/env/common.c +++ b/env/common.c @@ -401,7 +401,15 @@ int env_set_default_vars(int nvars, char * const vars[], int flags) * Special use-case: import from default environment * (and use \0 as a separator) */
flags |= H_NOCLEAR | H_DEFAULT;
/*
* When vars are passed remove variables that are not in
* the default environment.
*/
if (!nvars)
flags |= H_NOCLEAR;
flags |= H_DEFAULT; return himport_r(&env_htab, default_environment, sizeof(default_environment), '\0', flags, 0, nvars, vars);
-- 2.25.1
Regards, Simon

current env_set_default_vars() doesn't delete var that are not in the imported env. hashtable removes vars that are not in the imported env but present in the current env only if H_NOCLEAR flag is not set.
This change is to avoid passing H_NOCLEAR flag if specific vars are passed to env_set_default_vars()
Without this change: Marvell>> env default boot_mode Marvell>>
With the change: Marvell>> env default boot_mode WARNING: 'boot_mode' not in imported env, deleting it!
Signed-off-by: rminnikanti rminnikanti@marvell.com --- Changes in v2: - Added env ut to test the scenario - Updated doc usage/cmd/env.rst
=> ut env Running 5 environment tests Test: env_test_attrs_lookup: attr.c Test: env_test_attrs_lookup_regex: attr.c Test: env_test_env_cmd: cmd_ut_env.c Test: env_test_htab_deletes: hashtable.c Test: env_test_htab_fill: hashtable.c Failures: 0
doc/usage/cmd/env.rst | 4 +++- env/common.c | 10 +++++++++- test/env/cmd_ut_env.c | 36 ++++++++++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 2 deletions(-)
diff --git a/doc/usage/cmd/env.rst b/doc/usage/cmd/env.rst index 9629f97ffc..9104c88525 100644 --- a/doc/usage/cmd/env.rst +++ b/doc/usage/cmd/env.rst @@ -79,7 +79,8 @@ The *env default* command resets the selected variables in the U-Boot environment to their default values.
var - list of variable name. + list of variable name. If variable is not part of default + environment, it is deleted with a warning message on console. -a all U-Boot environment. -f @@ -309,6 +310,7 @@ Delete environment variable in memory:: Reset environment variable to default value, in memory::
=> env default bootcmd + => env default ipaddr serverip => env default -a
Save current environment in persistent storage:: diff --git a/env/common.c b/env/common.c index 8d47d72605..2f783e3a69 100644 --- a/env/common.c +++ b/env/common.c @@ -401,7 +401,15 @@ int env_set_default_vars(int nvars, char * const vars[], int flags) * Special use-case: import from default environment * (and use \0 as a separator) */ - flags |= H_NOCLEAR | H_DEFAULT; + + /* + * When vars are passed remove variables that are not in + * the default environment. + */ + if (!nvars) + flags |= H_NOCLEAR; + + flags |= H_DEFAULT; return himport_r(&env_htab, default_environment, sizeof(default_environment), '\0', flags, 0, nvars, vars); diff --git a/test/env/cmd_ut_env.c b/test/env/cmd_ut_env.c index 13e0998341..99961311aa 100644 --- a/test/env/cmd_ut_env.c +++ b/test/env/cmd_ut_env.c @@ -9,6 +9,42 @@ #include <test/suites.h> #include <test/ut.h>
+static int env_test_env_cmd(struct unit_test_state *uts) +{ + console_record_reset_enable(); + ut_assertok(run_command("setenv non_default_var1 1", 0)); + ut_assert_console_end(); + + console_record_reset_enable(); + ut_assertok(run_command("setenv non_default_var2 1", 0)); + ut_assert_console_end(); + + console_record_reset_enable(); + ut_assertok(run_command("env print non_default_var1", 0)); + ut_assert_nextline("non_default_var1=1"); + ut_assert_console_end(); + + console_record_reset_enable(); + ut_assertok(run_command("env default non_default_var1 non_default_var2", 0)); + ut_assert_nextline("WARNING: 'non_default_var1' not in imported env, deleting it!"); + ut_assert_nextline("WARNING: 'non_default_var2' not in imported env, deleting it!"); + ut_assert_console_end(); + + console_record_reset_enable(); + ut_asserteq(1, run_command("env exists non_default_var1", 0)); + ut_assert_nextline_empty(); + ut_assert_console_end(); + + console_record_reset_enable(); + ut_asserteq(1, run_command("env exists non_default_var2", 0)); + ut_assert_nextline_empty(); + ut_assert_console_end(); + + return 0; +} + +ENV_TEST(env_test_env_cmd, 0); + int do_ut_env(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { struct unit_test *tests = UNIT_TEST_SUITE_START(env_test);

Hi Ravi,
On Fri, 9 Aug 2024 at 13:36, Ravi Minnikanti rminnikanti@marvell.com wrote:
current env_set_default_vars() doesn't delete var that are not in the imported env. hashtable removes vars that are not in the imported env but present in the current env only if H_NOCLEAR flag is not set.
This change is to avoid passing H_NOCLEAR flag if specific vars are passed to env_set_default_vars()
Without this change: Marvell>> env default boot_mode Marvell>>
With the change: Marvell>> env default boot_mode WARNING: 'boot_mode' not in imported env, deleting it!
Signed-off-by: rminnikanti rminnikanti@marvell.com
That should have your name at the start
Changes in v2:
- Added env ut to test the scenario
- Updated doc usage/cmd/env.rst
=> ut env Running 5 environment tests Test: env_test_attrs_lookup: attr.c Test: env_test_attrs_lookup_regex: attr.c Test: env_test_env_cmd: cmd_ut_env.c Test: env_test_htab_deletes: hashtable.c Test: env_test_htab_fill: hashtable.c Failures: 0
doc/usage/cmd/env.rst | 4 +++- env/common.c | 10 +++++++++- test/env/cmd_ut_env.c | 36 ++++++++++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 2 deletions(-)
Just some nits below
diff --git a/doc/usage/cmd/env.rst b/doc/usage/cmd/env.rst index 9629f97ffc..9104c88525 100644 --- a/doc/usage/cmd/env.rst +++ b/doc/usage/cmd/env.rst @@ -79,7 +79,8 @@ The *env default* command resets the selected variables in the U-Boot environment to their default values.
var
list of variable name.
list of variable name. If variable is not part of default
list of variable names. If a variable is not ...
-a all U-Boot environment. -fenvironment, it is deleted with a warning message on console.
@@ -309,6 +310,7 @@ Delete environment variable in memory:: Reset environment variable to default value, in memory::
=> env default bootcmd
- => env default ipaddr serverip => env default -a
Save current environment in persistent storage:: diff --git a/env/common.c b/env/common.c index 8d47d72605..2f783e3a69 100644 --- a/env/common.c +++ b/env/common.c @@ -401,7 +401,15 @@ int env_set_default_vars(int nvars, char * const vars[], int flags) * Special use-case: import from default environment * (and use \0 as a separator) */
flags |= H_NOCLEAR | H_DEFAULT;
/*
There is a trailing space on the end of that line. Be sure to check your patch with patman/checkpatch.pl
* When vars are passed remove variables that are not in
* the default environment.
*/
if (!nvars)
flags |= H_NOCLEAR;
flags |= H_DEFAULT; return himport_r(&env_htab, default_environment, sizeof(default_environment), '\0', flags, 0, nvars, vars);
diff --git a/test/env/cmd_ut_env.c b/test/env/cmd_ut_env.c index 13e0998341..99961311aa 100644 --- a/test/env/cmd_ut_env.c +++ b/test/env/cmd_ut_env.c @@ -9,6 +9,42 @@ #include <test/suites.h> #include <test/ut.h>
+static int env_test_env_cmd(struct unit_test_state *uts) +{
console_record_reset_enable();
ut_assertok(run_command("setenv non_default_var1 1", 0));
ut_assert_console_end();
console_record_reset_enable();
ut_assertok(run_command("setenv non_default_var2 1", 0));
ut_assert_console_end();
console_record_reset_enable();
ut_assertok(run_command("env print non_default_var1", 0));
ut_assert_nextline("non_default_var1=1");
ut_assert_console_end();
console_record_reset_enable();
ut_assertok(run_command("env default non_default_var1 non_default_var2", 0));
ut_assert_nextline("WARNING: 'non_default_var1' not in imported env, deleting it!");
ut_assert_nextline("WARNING: 'non_default_var2' not in imported env, deleting it!");
ut_assert_console_end();
console_record_reset_enable();
ut_asserteq(1, run_command("env exists non_default_var1", 0));
ut_assert_nextline_empty();
You don't need that line. It means that the next line should exist, but be empty. In fact this exposes a bug in console checking which I will fix.
ut_assert_console_end();
console_record_reset_enable();
ut_asserteq(1, run_command("env exists non_default_var2", 0));
ut_assert_nextline_empty();
same here
ut_assert_console_end();
return 0;
+}
+ENV_TEST(env_test_env_cmd, 0);
Set the flags to UT_TESTF_CONSOLE_REC and you can drop all the console_record_reset_enable() calls above. Unfortunately many of the tests don't do this as the flag was added later.
int do_ut_env(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { struct unit_test *tests = UNIT_TEST_SUITE_START(env_test); -- 2.25.1
Thanks, Ravi
On 8/9/24 08:58, Simon Glass wrote:
Hi Ravi, On Fri, 9 Aug 2024 at 09: 38, Ravi Minnikanti <rminnikanti@ marvell. com> wrote: > > > current env_set_default_vars() doesn't delete > var that are not in the imported env. hashtable > removes vars that are not in
Hi Ravi,
On Fri, 9 Aug 2024 at 09:38, Ravi Minnikanti rminnikanti@marvell.com wrote:
current env_set_default_vars() doesn't delete var that are not in the imported env. hashtable removes vars that are not in the imported env but present in the current env only if H_NOCLEAR flag is not set.
This change is to avoid passing H_NOCLEAR flag if specific vars are passed to env_set_default_vars()
Test:
Without this change: Marvell>> env default boot_mode Marvell>>
With the change: Marvell>> env default boot_mode WARNING: 'boot_mode' not in imported env, deleting it!
Signed-off-by: rminnikanti rminnikanti@marvell.com
env/common.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
Could you add to usage/environment.rst and also update the tests for this - test/env/ - I suppose your patch fixes a bug, but it is hard to figure out what it is supposed to do from the docs.
See for example dm_test_acpi_cmd_set() for how to check output from a command.
diff --git a/env/common.c b/env/common.c index 8d47d72605..2f783e3a69 100644 --- a/env/common.c +++ b/env/common.c @@ -401,7 +401,15 @@ int env_set_default_vars(int nvars, char * const vars[], int flags) * Special use-case: import from default environment * (and use \0 as a separator) */
flags |= H_NOCLEAR | H_DEFAULT;
/*
* When vars are passed remove variables that are not in
* the default environment.
*/
if (!nvars)
flags |= H_NOCLEAR;
flags |= H_DEFAULT; return himport_r(&env_htab, default_environment, sizeof(default_environment), '\0', flags, 0, nvars, vars);
-- 2.25.1
Regards, Simon

current env_set_default_vars() doesn't delete var that are not in the imported env. hashtable removes vars that are not in the imported env but present in the current env only if H_NOCLEAR flag is not set.
This change is to avoid passing H_NOCLEAR flag if specific vars are passed to env_set_default_vars()
Without this change: Marvell>> env default boot_mode Marvell>>
With the change: Marvell>> env default boot_mode WARNING: 'boot_mode' not in imported env, deleting it!
Signed-off-by: Ravi Minnikanti rminnikanti@marvell.com --- Changes in v2: - Added env ut to test the scenario - Updated doc usage/cmd/env.rst
=> ut env Running 5 environment tests Test: env_test_attrs_lookup: attr.c Test: env_test_attrs_lookup_regex: attr.c Test: env_test_env_cmd: cmd_ut_env.c Test: env_test_htab_deletes: hashtable.c Test: env_test_htab_fill: hashtable.c Failures: 0
Changes in v3: - Fixed review comments - Ran checkpatch.pl and fixed trailing whitespaces. - Used UT_TESTF_CONSOLE_REC flag instead of console_record_reset_enable() - Removed unnecessary ut_assert_nextline_empty() check --- doc/usage/cmd/env.rst | 4 +++- env/common.c | 10 +++++++++- test/env/cmd_ut_env.c | 27 +++++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 2 deletions(-)
diff --git a/doc/usage/cmd/env.rst b/doc/usage/cmd/env.rst index 9629f97ffc..b65d85b668 100644 --- a/doc/usage/cmd/env.rst +++ b/doc/usage/cmd/env.rst @@ -79,7 +79,8 @@ The *env default* command resets the selected variables in the U-Boot environment to their default values.
var - list of variable name. + list of variable names. If variable is not part of default + environment, it is deleted with a warning message on console. -a all U-Boot environment. -f @@ -309,6 +310,7 @@ Delete environment variable in memory:: Reset environment variable to default value, in memory::
=> env default bootcmd + => env default ipaddr serverip => env default -a
Save current environment in persistent storage:: diff --git a/env/common.c b/env/common.c index 8d47d72605..6cba7f1c18 100644 --- a/env/common.c +++ b/env/common.c @@ -401,7 +401,15 @@ int env_set_default_vars(int nvars, char * const vars[], int flags) * Special use-case: import from default environment * (and use \0 as a separator) */ - flags |= H_NOCLEAR | H_DEFAULT; + + /* + * When vars are passed remove variables that are not in + * the default environment. + */ + if (!nvars) + flags |= H_NOCLEAR; + + flags |= H_DEFAULT; return himport_r(&env_htab, default_environment, sizeof(default_environment), '\0', flags, 0, nvars, vars); diff --git a/test/env/cmd_ut_env.c b/test/env/cmd_ut_env.c index 13e0998341..238cf31036 100644 --- a/test/env/cmd_ut_env.c +++ b/test/env/cmd_ut_env.c @@ -9,6 +9,33 @@ #include <test/suites.h> #include <test/ut.h>
+static int env_test_env_cmd(struct unit_test_state *uts) +{ + ut_assertok(run_command("setenv non_default_var1 1", 0)); + ut_assert_console_end(); + + ut_assertok(run_command("setenv non_default_var2 1", 0)); + ut_assert_console_end(); + + ut_assertok(run_command("env print non_default_var1", 0)); + ut_assert_nextline("non_default_var1=1"); + ut_assert_console_end(); + + ut_assertok(run_command("env default non_default_var1 non_default_var2", 0)); + ut_assert_nextline("WARNING: 'non_default_var1' not in imported env, deleting it!"); + ut_assert_nextline("WARNING: 'non_default_var2' not in imported env, deleting it!"); + ut_assert_console_end(); + + ut_asserteq(1, run_command("env exists non_default_var1", 0)); + ut_assert_console_end(); + + ut_asserteq(1, run_command("env exists non_default_var2", 0)); + ut_assert_console_end(); + + return 0; +} +ENV_TEST(env_test_env_cmd, UT_TESTF_CONSOLE_REC); + int do_ut_env(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { struct unit_test *tests = UNIT_TEST_SUITE_START(env_test);

On Sun, 11 Aug 2024 at 12:44, Ravi Minnikanti rminnikanti@marvell.com wrote:
current env_set_default_vars() doesn't delete var that are not in the imported env. hashtable removes vars that are not in the imported env but present in the current env only if H_NOCLEAR flag is not set.
This change is to avoid passing H_NOCLEAR flag if specific vars are passed to env_set_default_vars()
Without this change: Marvell>> env default boot_mode Marvell>>
With the change: Marvell>> env default boot_mode WARNING: 'boot_mode' not in imported env, deleting it!
Signed-off-by: Ravi Minnikanti rminnikanti@marvell.com
Changes in v2:
- Added env ut to test the scenario
- Updated doc usage/cmd/env.rst
=> ut env Running 5 environment tests Test: env_test_attrs_lookup: attr.c Test: env_test_attrs_lookup_regex: attr.c Test: env_test_env_cmd: cmd_ut_env.c Test: env_test_htab_deletes: hashtable.c Test: env_test_htab_fill: hashtable.c Failures: 0
Changes in v3:
- Fixed review comments
- Ran checkpatch.pl and fixed trailing whitespaces.
- Used UT_TESTF_CONSOLE_REC flag instead of console_record_reset_enable()
- Removed unnecessary ut_assert_nextline_empty() check
doc/usage/cmd/env.rst | 4 +++- env/common.c | 10 +++++++++- test/env/cmd_ut_env.c | 27 +++++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org

On Sun, 11 Aug 2024 11:44:15 -0700, Ravi Minnikanti wrote:
current env_set_default_vars() doesn't delete var that are not in the imported env. hashtable removes vars that are not in the imported env but present in the current env only if H_NOCLEAR flag is not set.
This change is to avoid passing H_NOCLEAR flag if specific vars are passed to env_set_default_vars()
[...]
Applied to u-boot/next, thanks!
participants (3)
-
Ravi Minnikanti
-
Simon Glass
-
Tom Rini