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

[Simon Glass]

Hi Ravi,

On Fri, 9 Aug 2024 at 13:36, Ravi Minnikanti <rminnikanti at 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 at 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 at 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 at 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