[PATCH v2 0/2] env: setenv add resolve value option

[PATCH v2 0/2] env: setenv add resolve value option

[Artem Lapkin]

Add possibility setup env variable with additional resolving vars inside
value.

Usage examples:

=> setenv a hello; setenv b world; setenv c '${a} ${b}'
=> setenv -r d '${c}! ${a}...'
=> printenv d
d=hello world! hello...

/* internal usage example */
env_resolve("d", "${c}! ${a}...");
/* d="hello world! hello..." */

Artem Lapkin (2):
  env: setenv add resolve value option
  test: env: deep resolve value testing

---
V2 changes:
_ fix comments style
_ add comment include/exports.h
_ remake strcpy to strdup
_ env_resolve minimize
_ test added: test/py/tests/test_env.py: test_env_resovle
---

 cmd/nvedit.c              | 43 ++++++++++++++++++++++++++++++++++++++-
 include/_exports.h        |  1 +
 include/env.h             | 11 ++++++++++
 include/exports.h         |  2 ++
 test/py/tests/test_env.py | 24 ++++++++++++++++++++++
 5 files changed, 80 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH v2 1/2] env: setenv add resolve value option

[Artem Lapkin]

Add possibility setup env variable with additional resolving vars inside
value.

Usage examples:

=> setenv a hello; setenv b world; setenv c '${a} ${b}'
=> setenv -r d '${c}! ${a}...'
=> printenv d
d=hello world! hello...

/* internal usage example */
env_resolve("d", "${c}! ${a}...");
/* d="hello world! hello..." */

Signed-off-by: Artem Lapkin <art@khadas.com>
---
V2 changes:
_ fix comments style
_ add comment include/exports.h
_ remake strcpy to strdup
_ env_resolve minimize
---
 cmd/nvedit.c       | 43 ++++++++++++++++++++++++++++++++++++++++++-
 include/_exports.h |  1 +
 include/env.h      | 11 +++++++++++
 include/exports.h  |  2 ++
 4 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 3bb6e764c0..6e1237df81 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -229,6 +229,7 @@ static int _do_env_set(int flag, int argc, char *const argv[], int env_flag)
 	int   i, len;
 	char  *name, *value, *s;
 	struct env_entry e, *ep;
+	bool  resolve = 0;
 
 	debug("Initial value for argc=%d\n", argc);
 
@@ -246,6 +247,9 @@ static int _do_env_set(int flag, int argc, char *const argv[], int env_flag)
 			case 'f':		/* force */
 				env_flag |= H_FORCE;
 				break;
+			case 'r':		/* resolve */
+				resolve = 1;
+				break;
 			default:
 				return CMD_RET_USAGE;
 			}
@@ -291,6 +295,29 @@ static int _do_env_set(int flag, int argc, char *const argv[], int env_flag)
 	if (s != value)
 		*--s = '\0';
 
+	/* deep resolve value vars */
+	if (resolve) {
+		int max_loop = 32;
+		char value2[CONFIG_SYS_CBSIZE];
+		char *v = NULL;
+
+		do {
+			cli_simple_process_macros(value, value2, CONFIG_SYS_CBSIZE);
+
+			if (!strcmp(value, value2))
+				break;
+
+			v = strdup(value2);
+			if (!v) {
+				printf("## Can't allocate memory\n");
+				return 1;
+			}
+
+			free(value);
+			value = v;
+		} while (max_loop--);
+	}
+
 	e.key	= name;
 	e.data	= value;
 	hsearch_r(e, ENV_ENTER, &ep, &env_htab, env_flag);
@@ -304,6 +331,18 @@ static int _do_env_set(int flag, int argc, char *const argv[], int env_flag)
 	return 0;
 }
 
+int env_resolve(const char *varname, const char *varvalue)
+{
+	const char * const argv[5] = { "setenv", "-r", varname, varvalue, NULL };
+
+	/* before import into hashtable */
+	if (!(gd->flags & GD_FLG_ENV_READY))
+		return 1;
+
+	return _do_env_set(0, !varvalue || varvalue[0]  == '\0' ? 3 : 4,
+		(char * const *)argv, H_PROGRAMMATIC);
+}
+
 int env_set(const char *varname, const char *varvalue)
 {
 	const char * const argv[4] = { "setenv", varname, varvalue, NULL };
@@ -1371,7 +1410,9 @@ U_BOOT_CMD_COMPLETE(
 	"setenv [-f] name value ...\n"
 	"    - [forcibly] set environment variable 'name' to 'value ...'\n"
 	"setenv [-f] name\n"
-	"    - [forcibly] delete environment variable 'name'",
+	"    - [forcibly] delete environment variable 'name'\n"
+	"setenv [-r] name value ...\n"
+	"    - [resolve] resolve 'value ...' to environment variable\n",
 	var_complete
 );
 
diff --git a/include/_exports.h b/include/_exports.h
index 8030d70c0b..86bc07f051 100644
--- a/include/_exports.h
+++ b/include/_exports.h
@@ -32,6 +32,7 @@
 	EXPORT_FUNC(do_reset, int, do_reset, struct cmd_tbl *,
 		    int , int , char * const [])
 	EXPORT_FUNC(env_get, char  *, env_get, const char*)
+	EXPORT_FUNC(env_resolve, int, env_resolve, const char *, const char *)
 	EXPORT_FUNC(env_set, int, env_set, const char *, const char *)
 	EXPORT_FUNC(simple_strtoul, unsigned long, simple_strtoul,
 		    const char *, char **, unsigned int)
diff --git a/include/env.h b/include/env.h
index ee5e30d036..73d1aca9ac 100644
--- a/include/env.h
+++ b/include/env.h
@@ -133,6 +133,17 @@ int env_get_f(const char *name, char *buf, unsigned int len);
  */
 int env_get_yesno(const char *var);
 
+/**
+ * env_resolve() - resolve to environment variable
+ *
+ * Same as env_set but make deep resolve for value
+ *
+ * @varname: Variable to adjust
+ * @value: Value to resolve for the variable, or NULL or "" to delete the variable
+ * @return 0 if OK, 1 on error
+ */
+int env_resolve(const char *varname, const char *value);
+
 /**
  * env_set() - set an environment variable
  *
diff --git a/include/exports.h b/include/exports.h
index 550cafdc7a..02aac8047c 100644
--- a/include/exports.h
+++ b/include/exports.h
@@ -44,6 +44,8 @@ int vprintf(const char *, va_list);
 unsigned long simple_strtoul(const char *cp, char **endp, unsigned int base);
 int strict_strtoul(const char *cp, unsigned int base, unsigned long *res);
 char *env_get(const char *name);
+/* deep resolve value to environment variable */
+int env_resolve(const char *varname, const char *value);
 int env_set(const char *varname, const char *value);
 long simple_strtol(const char *cp, char **endp, unsigned int base);
 int strcmp(const char *cs, const char *ct);
-- 
2.25.1

Re: [PATCH v2 1/2] env: setenv add resolve value option

[Simon Glass]

On Thu, 18 Nov 2021 at 21:37, Artem Lapkin <email2tema@gmail.com> wrote:
>
> Add possibility setup env variable with additional resolving vars inside
> value.
>
> Usage examples:
>
> => setenv a hello; setenv b world; setenv c '${a} ${b}'
> => setenv -r d '${c}! ${a}...'
> => printenv d
> d=hello world! hello...
>
> /* internal usage example */
> env_resolve("d", "${c}! ${a}...");
> /* d="hello world! hello..." */
>
> Signed-off-by: Artem Lapkin <art@khadas.com>
> ---
> V2 changes:
> _ fix comments style
> _ add comment include/exports.h
> _ remake strcpy to strdup
> _ env_resolve minimize
> ---
>  cmd/nvedit.c       | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  include/_exports.h |  1 +
>  include/env.h      | 11 +++++++++++
>  include/exports.h  |  2 ++
>  4 files changed, 56 insertions(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 1/2] env: setenv add resolve value option

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 603 bytes --]

On Fri, Nov 19, 2021 at 12:36:46PM +0800, Artem Lapkin wrote:
> Add possibility setup env variable with additional resolving vars inside

> value.
> 
> Usage examples:
> 
> => setenv a hello; setenv b world; setenv c '${a} ${b}'
> => setenv -r d '${c}! ${a}...'
> => printenv d
> d=hello world! hello...
> 
> /* internal usage example */
> env_resolve("d", "${c}! ${a}...");
> /* d="hello world! hello..." */
> 
> Signed-off-by: Artem Lapkin <art@khadas.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

This break building on a number of platforms such as am335x_evm.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 1/2] env: setenv add resolve value option

[Sean Anderson]

On 4/7/22 2:05 PM, Tom Rini wrote:
> On Fri, Nov 19, 2021 at 12:36:46PM +0800, Artem Lapkin wrote:
>> Add possibility setup env variable with additional resolving vars inside
> 
>> value.
>>
>> Usage examples:
>>
>> => setenv a hello; setenv b world; setenv c '${a} ${b}'
>> => setenv -r d '${c}! ${a}...'
>> => printenv d
>> d=hello world! hello...
>>
>> /* internal usage example */
>> env_resolve("d", "${c}! ${a}...");
>> /* d="hello world! hello..." */
>>
>> Signed-off-by: Artem Lapkin <art@khadas.com>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> This break building on a number of platforms such as am335x_evm.
> 

Should this even be applied in the first place? I agree with Wolfgang's
objections. This should be done by the shell (if anything).

--Sean

Re: [PATCH v2 1/2] env: setenv add resolve value option

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 964 bytes --]

On Fri, Apr 08, 2022 at 09:09:41AM -0400, Sean Anderson wrote:
> On 4/7/22 2:05 PM, Tom Rini wrote:
> > On Fri, Nov 19, 2021 at 12:36:46PM +0800, Artem Lapkin wrote:
> > > Add possibility setup env variable with additional resolving vars inside
> > 
> > > value.
> > > 
> > > Usage examples:
> > > 
> > > => setenv a hello; setenv b world; setenv c '${a} ${b}'
> > > => setenv -r d '${c}! ${a}...'
> > > => printenv d
> > > d=hello world! hello...
> > > 
> > > /* internal usage example */
> > > env_resolve("d", "${c}! ${a}...");
> > > /* d="hello world! hello..." */
> > > 
> > > Signed-off-by: Artem Lapkin <art@khadas.com>
> > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > 
> > This break building on a number of platforms such as am335x_evm.
> 
> Should this even be applied in the first place? I agree with Wolfgang's
> objections. This should be done by the shell (if anything).

Ah true, thanks for reminding me.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH v2 2/2] test: env: deep resolve value testing

[Artem Lapkin]

Add new tests for `setenv -r` options (setup env variable with additional
resolving vars inside value).

test.py -k test_env

Signed-off-by: Artem Lapkin <art@khadas.com>
---
 test/py/tests/test_env.py | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py
index 9bed2f48d7..a2f02f6f67 100644
--- a/test/py/tests/test_env.py
+++ b/test/py/tests/test_env.py
@@ -201,6 +201,30 @@ def test_env_unset_non_existent(state_test_env):
     unset_var(state_test_env, var)
     validate_empty(state_test_env, var)
 
+def test_env_resovle(state_test_env):
+    """Test deep resolve variable."""
+    c = state_test_env.u_boot_console
+    c.run_command("setenv a hello; setenv b world; setenv c '${a} ${b}'")
+    assert c.run_command('printenv a') == 'a=hello'
+    assert c.run_command('printenv b') == 'b=world'
+    assert c.run_command('printenv c') == 'c=${a} ${b}'
+    c.run_command("setenv d '${c}'")
+    assert c.run_command('printenv d') == 'd=${c}'
+    c.run_command("setenv -r e '${a} ${b} ${c}'")
+    assert c.run_command('printenv e') == 'e=hello world hello world'
+    c.run_command("setenv not_exist; setenv -r e '${not_exist}OK'")
+    assert c.run_command('printenv e') == 'e=OK'
+    c.run_command("setenv a 'A${b}'; setenv b 'B${a}'")
+    assert c.run_command('printenv a') == 'a=A${b}'
+    c.run_command("setenv e ''")
+    assert c.run_command('printenv e') == 'e='
+    c.run_command("setenv -r e '${b}'")
+    #assert c.run_command('printenv e') == 'e=BABABABABABABABABABABABABABABABAB${a}'
+    assert c.run_command('printenv e') != 'e='
+    c.run_command("setenv -r e")
+    with c.disable_check('error_notification'):
+        assert c.run_command('printenv e') == '## Error: "e" not defined'
+
 def test_env_set_non_existent(state_test_env):
     """Test set a non-existant variable."""
 
-- 
2.25.1

Re: [PATCH v2 2/2] test: env: deep resolve value testing

[Simon Glass]

On Thu, 18 Nov 2021 at 21:37, Artem Lapkin <email2tema@gmail.com> wrote:
>
> Add new tests for `setenv -r` options (setup env variable with additional
> resolving vars inside value).
>
> test.py -k test_env
>
> Signed-off-by: Artem Lapkin <art@khadas.com>
> ---
>  test/py/tests/test_env.py | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 0/2] env: setenv add resolve value option

[Wolfgang Denk]

Dear Artem,

In message <20211119043647.1251416-1-art@khadas.com> you wrote:
> Add possibility setup env variable with additional resolving vars inside
> value.

Hm... if you want to evaluate variables, you should not prevent the
shell to do that by enclosing them in apostrophes?

> Usage examples:
>
> => setenv a hello; setenv b world; setenv c '${a} ${b}'
> => setenv -r d '${c}! ${a}...'
> => printenv d
> d=hello world! hello...

Without any new code added:

=> setenv a hello; setenv b world; setenv c ${a} ${b}
=> setenv d ${c}! ${a}...
=> printenv d
d=hello world! hello...


I know very well that this does not cover all use cases you might
have in mind, as you speak of "deep resolve". But then, I'm first
missing an explanation (and documentation) of what "deep resolve"
actually means in this context, i. e. how many levels down you
evaluat.   Oh...  the code has "int max_loop = 32;" - this is a
limitation mentioned nowhere.  And are you really sure this is a
clever idea?  I am not convinced.  If we do something like this, we
should not invent a new non-standard way.  We should implement
standard shell behaviour instead (i. e. for example something like
the eval command).

> Artem Lapkin (2):
>   env: setenv add resolve value option
>   test: env: deep resolve value testing

This is one more of the patches that actually try to fix existing
problems with our ancient verion of the hush shell.  As far as I
understand, Francis (added to Cc:) has started working on an update
of hush to a current version.  We should rather help him with that
instead of implementing non-standard workarounds.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
When choosing between two evils, I always like to take the  one  I've
never tried before.                     -- Mae West, "Klondike Annie"

Re: [PATCH v2 0/2] env: setenv add resolve value option

[Art Nikpal]

hi Wolfgang
tnx for your comments

On Fri, Nov 19, 2021 at 3:49 PM Wolfgang Denk <wd@denx.de> wrote:
>
> Dear Artem,
>
> In message <20211119043647.1251416-1-art@khadas.com> you wrote:
> > Add possibility setup env variable with additional resolving vars inside
> > value.
>
> Hm... if you want to evaluate variables, you should not prevent the
> shell to do that by enclosing them in apostrophes?
>

next examples just demonstrate how its works for already defined env
variables which contain other variables (like storred env variables)

> > Usage examples:
> >
> > => setenv a hello; setenv b world; setenv c '${a} ${b}'
> > => setenv -r d '${c}! ${a}...'
> > => printenv d
> > d=hello world! hello...
>
> Without any new code added:
>

sure I know about this ! see my prev message please .

> => setenv a hello; setenv b world; setenv c ${a} ${b}
> => setenv d ${c}! ${a}...
> => printenv d
> d=hello world! hello...
>
>
> I know very well that this does not cover all use cases you might

Why not have this new opportunity ?

> have in mind, as you speak of "deep resolve". But then, I'm first
> missing an explanation (and documentation) of what "deep resolve"

recurrent  resolving for variables

> actually means in this context, i. e. how many levels down you
> evaluat.   Oh...  the code has "int max_loop = 32;" - this is a

i think its will be enough

> limitation mentioned nowhere.  And are you really sure this is a
> clever idea?  I am not convinced.  If we do something like this, we
> should not invent a new non-standard way.  We should implement

I believe it's a good idea ;-)

> standard shell behaviour instead (i. e. for example something like
> the eval command).

1) this option did not broke any exist compatibilities
2) there we talk not only about uboot shell, same time will be useful
to have env_resolve for internal c usage, because env_set dont have
this feature

>
> > Artem Lapkin (2):
> >   env: setenv add resolve value option
> >   test: env: deep resolve value testing
>
> This is one more of the patches that actually try to fix existing
> problems with our ancient verion of the hush shell.  As far as I
> understand, Francis (added to Cc:) has started working on an update
> of hush to a current version.  We should rather help him with that
> instead of implementing non-standard workarounds.

yes i'm informed  about this plans  (and think its happens  not  so
soon - but i provide some simple elegant solution already)
but again we dont have env_resolve for internal c usage which must be
very useful

for example: pxe_utils.c: and some other places
env_set("bootargs", finalbootargs);
can be replaced by env_resolve("bootargs", ..... )
will be easy get useful features via simple solution ( deep resolve
all vars by one line )

>
> Best regards,
>

Big tnx again ! and waiting any other ideas about this

> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> When choosing between two evils, I always like to take the  one  I've
> never tried before.                     -- Mae West, "Klondike Annie"

Re: [PATCH v2 0/2] env: setenv add resolve value option

[Wolfgang Denk]

Dear Artem,

In message <CAKaHn9KZg13r=pGCo=Lv69pBP-s-EW4uxjBVuH9veT+o5j9v+g@mail.gmail.com> you wrote:
>
> next examples just demonstrate how its works for already defined env
> variables which contain other variables (like storred env variables)

Which next examples?

> sure I know about this ! see my prev message please .

Which exact message are you referring to here?

> Why not have this new opportunity ?

I think the suggested code is adding more problems than it solves.

> > have in mind, as you speak of "deep resolve". But then, I'm first
> > missing an explanation (and documentation) of what "deep resolve"
>
> recurrent  resolving for variables

Your implementation of recursion has an arbiotrary and undocumented
depth limit. Also, I cannot see a way to prevent resolving in case I
want to keep something like "$foo" in the result.

But that's to be expected from such a non-standard way.

Why don't you stick with what "eval" in a standard shell does?

> > actually means in this context, i. e. how many levels down you
> > evaluat.   Oh...  the code has "int max_loop = 32;" - this is a
>
> i think its will be enough

It is a reallybad habt to implement code with arbitrary limits, as
it will blow into your face (or more likely that of an innocent
user) rather sooner than later.  It's even worse that this limit is
nowhere documented.

> 1) this option did not broke any exist compatibilities
> 2) there we talk not only about uboot shell, same time will be useful
> to have env_resolve for internal c usage, because env_set dont have
> this feature

I did not say that an "eval" like construct would not be useful.
But uncontrolled recursion with an undocumented depth limit is
a problem.

> yes i'm informed  about this plans  (and think its happens  not  so
> soon - but i provide some simple elegant solution already)
> but again we dont have env_resolve for internal c usage which must be
> very useful

On the CLI, we use the "run" command to get the desired effect. Yes,
this is neither perfect nor elegant. But you can use that in C code
as well.

> will be easy get useful features via simple solution ( deep resolve
> all vars by one line )

I understand what you want, but this is not a good way to solve the
problem.  I'd really rather see such efforts invested in helping
Francis with the hush update - which will make such code unnecessary.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Success in marriage is not so much finding the right person as it  is
being the right person.

Re: [PATCH v2 0/2] env: setenv add resolve value option

[Art Nikpal]

On Sat, Nov 20, 2021 at 8:36 PM Wolfgang Denk <wd@denx.de> wrote:
>
> Dear Artem,
>
> In message <CAKaHn9KZg13r=pGCo=Lv69pBP-s-EW4uxjBVuH9veT+o5j9v+g@mail.gmail.com> you wrote:
> >
> > next examples just demonstrate how its works for already defined env
> > variables which contain other variables (like storred env variables)
>
> Which next examples?
>
> > sure I know about this ! see my prev message please .
>
> Which exact message are you referring to here?
>
> > Why not have this new opportunity ?
>
> I think the suggested code is adding more problems than it solves.
>
> > > have in mind, as you speak of "deep resolve". But then, I'm first
> > > missing an explanation (and documentation) of what "deep resolve"
> >
> > recurrent  resolving for variables
>
> Your implementation of recursion has an arbiotrary and undocumented
> depth limit.

we can improve it if it will be really necessary

> Also, I cannot see a way to prevent resolving in case I
> want to keep something like "$foo" in the result.
>

`setenv -r` resolve only bracked vars like ${VAR} if u need keen some
$VAR just use it without brackets
or just use same setenv without -r option - whats a problem ?

> But that's to be expected from such a non-standard way.
>

we can use setenv with -r or without this option (default standard way)
again didn't see problems  (setenv -r is special option especially for
non standard way )

> Why don't you stick with what "eval" in a standard shell does?
>

show me please how can i do it via standard way maybe i miss something ?

> > > actually means in this context, i. e. how many levels down you
> > > evaluat.   Oh...  the code has "int max_loop = 32;" - this is a
> >
> > i think its will be enough
>
> It is a reallybad habt to implement code with arbitrary limits, as
> it will blow into your face (or more likely that of an innocent
> user) rather sooner than later.  It's even worse that this limit is
> nowhere documented.
>
> > 1) this option did not broke any exist compatibilities
> > 2) there we talk not only about uboot shell, same time will be useful
> > to have env_resolve for internal c usage, because env_set dont have
> > this feature
>
> I did not say that an "eval" like construct would not be useful.
> But uncontrolled recursion with an undocumented depth limit is
> a problem.
>
> > yes i'm informed  about this plans  (and think its happens  not  so
> > soon - but i provide some simple elegant solution already)
> > but again we dont have env_resolve for internal c usage which must be
> > very useful
>
> On the CLI, we use the "run" command to get the desired effect. Yes,
> this is neither perfect nor elegant. But you can use that in C code
> as well.
>
> > will be easy get useful features via simple solution ( deep resolve
> > all vars by one line )
>
> I understand what you want, but this is not a good way to solve the
> problem.  I'd really rather see such efforts invested in helping
> Francis with the hush update - which will make such code unnecessary.
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> Success in marriage is not so much finding the right person as it  is
> being the right person.

Re: [PATCH v2 0/2] env: setenv add resolve value option

[Art Nikpal]

On Sat, Nov 20, 2021 at 8:36 PM Wolfgang Denk <wd@denx.de> wrote:
>
> Dear Artem,
>
> In message <CAKaHn9KZg13r=pGCo=Lv69pBP-s-EW4uxjBVuH9veT+o5j9v+g@mail.gmail.com> you wrote:
> >
> > next examples just demonstrate how its works for already defined env
> > variables which contain other variables (like storred env variables)
>
> Which next examples?

Usage examples (from commit message):

=> setenv a hello; setenv b world; setenv c '${a} ${b}'
=> setenv -r d '${c}! ${a}...'
=> printenv d
d=hello world! hello...

>
> > sure I know about this ! see my prev message please .
>
> Which exact message are you referring to here?
>
> > Why not have this new opportunity ?
>
> I think the suggested code is adding more problems than it solves.
>
> > > have in mind, as you speak of "deep resolve". But then, I'm first
> > > missing an explanation (and documentation) of what "deep resolve"
> >
> > recurrent  resolving for variables
>
> Your implementation of recursion has an arbiotrary and undocumented
> depth limit. Also, I cannot see a way to prevent resolving in case I
> want to keep something like "$foo" in the result.
>
> But that's to be expected from such a non-standard way.
>
> Why don't you stick with what "eval" in a standard shell does?
>
> > > actually means in this context, i. e. how many levels down you
> > > evaluat.   Oh...  the code has "int max_loop = 32;" - this is a
> >
> > i think its will be enough
>
> It is a reallybad habt to implement code with arbitrary limits, as
> it will blow into your face (or more likely that of an innocent
> user) rather sooner than later.  It's even worse that this limit is
> nowhere documented.
>
> > 1) this option did not broke any exist compatibilities
> > 2) there we talk not only about uboot shell, same time will be useful
> > to have env_resolve for internal c usage, because env_set dont have
> > this feature
>
> I did not say that an "eval" like construct would not be useful.
> But uncontrolled recursion with an undocumented depth limit is
> a problem.
>
> > yes i'm informed  about this plans  (and think its happens  not  so
> > soon - but i provide some simple elegant solution already)
> > but again we dont have env_resolve for internal c usage which must be
> > very useful
>
> On the CLI, we use the "run" command to get the desired effect. Yes,
> this is neither perfect nor elegant. But you can use that in C code
> as well.
>
> > will be easy get useful features via simple solution ( deep resolve
> > all vars by one line )
>
> I understand what you want, but this is not a good way to solve the
> problem.  I'd really rather see such efforts invested in helping
> Francis with the hush update - which will make such code unnecessary.
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> Success in marriage is not so much finding the right person as it  is
> being the right person.

Re: [PATCH v2 0/2] env: setenv add resolve value option

[Wolfgang Denk]

Dear Artem,

In message <CAKaHn9+yL3naiDqbsQJWQ0Po=g=TmjjcPSKxkx5C4ay1M1AsUA@mail.gmail.com> you wrote:
> > >
> > > next examples just demonstrate how its works for already defined env
> > > variables which contain other variables (like storred env variables)
> >
> > Which next examples?
>
> Usage examples (from commit message):
>
> => setenv a hello; setenv b world; setenv c '${a} ${b}'
> => setenv -r d '${c}! ${a}...'
> => printenv d
> d=hello world! hello...

This is a very simple example, and I showed you how you can solve
this one by just omitting the apostrophes:

=> setenv a hello; setenv b world; setenv c ${a} ${b}
=> setenv d ${c}! ${a}...
=> printenv d
d=hello world! hello...


I _think_ what you actually have in mind is something like this:

=> setenv a hello
=> setenv b world
=> setenv c '${a} ${b}'
=> setenv a goodbye
=> setenv b sunshine

something to set d to: '${c}! ${a}...'

=> printenv d

Here my simple approach does not show what you want to have:

=> setenv a hello
=> setenv b world
=> setenv c ${a} ${b}
=> setenv a goodbye
=> setenv b sunshine
=> setenv d ${c}! ${a}...
=> printenv d
d=hello world! goodbye...

That's because here evaluation takes place at assignment, but you
want it when used - but being recursive here is neither a good idea
nor standard.

How would you do it in a standard posix shell?  You would have to
use "eval", like that:

$ a=hello
$ b=world
$ c='${a} ${b}'
$ a=goodbye
$ b=sunshine
$ d=$(eval echo $c)
$ echo $d
goodbye sunshine

But please note that "eval" is _not_ recursive!!

$ a='$b'
$ eval echo $c
$b sunshine

And this is why I object against this patch.

Oh, and in U-Boot you could write this as:

=> setenv a hello
=> setenv b world
=> setenv c '${a} ${b}'
=> setenv a goodbye
=> setenv b sunshine
=> setenv foo "setenv d ${c}! ${a}..."
=> run foo
=> printenv d                         
d=goodbye sunshine! goodbye...

And yes, here you have to be careful about using ' or " as there is
no recursion like you might expect.

So yes, it would be nice if we had "eval" (which will ocme with the
hush update), and no, "eval" does not recurse either.



Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Real computer scientists despise the idea of actual  hardware.  Hard-
ware has limitations, software doesn't. It's a real shame that Turing
machines are so poor at I/O.

Re: [PATCH v2 0/2] env: setenv add resolve value option

[Art Nikpal]

On Mon, Nov 22, 2021 at 4:52 PM Wolfgang Denk <wd@denx.de> wrote:
>
> Dear Artem,
>
> In message <CAKaHn9+yL3naiDqbsQJWQ0Po=g=TmjjcPSKxkx5C4ay1M1AsUA@mail.gmail.com> you wrote:
> > > >
> > > > next examples just demonstrate how its works for already defined env
> > > > variables which contain other variables (like storred env variables)
> > >
> > > Which next examples?
> >
> > Usage examples (from commit message):
> >
> > => setenv a hello; setenv b world; setenv c '${a} ${b}'
> > => setenv -r d '${c}! ${a}...'
> > => printenv d
> > d=hello world! hello...
>
> This is a very simple example, and I showed you how you can solve
> this one by just omitting the apostrophes:
>
> => setenv a hello; setenv b world; setenv c ${a} ${b}
> => setenv d ${c}! ${a}...
> => printenv d
> d=hello world! hello...
>

sure i know it and its easy, but we still have some misunderstanding,
and i will try explain again our problem

our problem (TASK)

we have:
some env variables which already defined and we totally dont know
about content ( for example its was improved by `env import` or loaded
from env storage  and  "setenv a hello; setenv b world; setenv c '${a}
${b}'; setenv d '${c}! ${a}...' ;"  just example how emulate this env
state )

we need:
setup/resolve env variable for example from d (and need resolve all
sub included vars)
=> setenv -r e $d
=> printenv e
=> e=hello world! hello...

please explain how we can get it without -r (deep resolve option) via
standard way ?


>
> I _think_ what you actually have in mind is something like this:
>
> => setenv a hello
> => setenv b world
> => setenv c '${a} ${b}'
> => setenv a goodbye
> => setenv b sunshine
>
> something to set d to: '${c}! ${a}...'
>
> => printenv d
>
> Here my simple approach does not show what you want to have:
>
> => setenv a hello
> => setenv b world
> => setenv c ${a} ${b}
> => setenv a goodbye
> => setenv b sunshine
> => setenv d ${c}! ${a}...
> => printenv d
> d=hello world! goodbye...
>
> That's because here evaluation takes place at assignment, but you
> want it when used - but being recursive here is neither a good idea
> nor standard.
>
> How would you do it in a standard posix shell?  You would have to
> use "eval", like that:
>
> $ a=hello
> $ b=world
> $ c='${a} ${b}'
> $ a=goodbye
> $ b=sunshine
> $ d=$(eval echo $c)
> $ echo $d
> goodbye sunshine
>
> But please note that "eval" is _not_ recursive!!
>
> $ a='$b'
> $ eval echo $c
> $b sunshine
>
> And this is why I object against this patch.
>
> Oh, and in U-Boot you could write this as:
>
> => setenv a hello
> => setenv b world
> => setenv c '${a} ${b}'
> => setenv a goodbye
> => setenv b sunshine
> => setenv foo "setenv d ${c}! ${a}..."
> => run foo
> => printenv d
> d=goodbye sunshine! goodbye...
>
> And yes, here you have to be careful about using ' or " as there is
> no recursion like you might expect.
>
> So yes, it would be nice if we had "eval" (which will ocme with the
> hush update), and no, "eval" does not recurse either.
>
>
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> Real computer scientists despise the idea of actual  hardware.  Hard-
> ware has limitations, software doesn't. It's a real shame that Turing
> machines are so poor at I/O.