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

-- 2.25.1 Thanks, Ravi On 8/11/24 07:50, Simon Glass wrote:> > > 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 ... > >> + 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; >> + >> + /* > > 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 >