[PATCH v3] env: remove vars that are not in default env
Ravi Minnikanti
rminnikanti at marvell.com
Sun Aug 11 20:44:15 CEST 2024
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 at 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);
--
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 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
>
More information about the U-Boot
mailing list