[U-Boot] [RFC PATCH v2 0/7] "Transient" (= export-restricted) environment vars
This series evolved around the idea of introducing a new env_op type to control (and possibly restrict) the export of variables. This is especially useful if one wants to prevent dynamic configuration information from ending up in a saved environment - the 'classic' example being network setup with "dhcp" followed by the "saveenv" command.
(The networking case is even further complicated by the fact that users may actually wish to setup and save a static IP configuration manually, which means that "locking up" the corresponding variables right away isn't a viable solution.)
See also: http://lists.denx.de/pipermail/u-boot/2015-September/227611.html http://lists.denx.de/pipermail/u-boot/2016-April/250237.html Pointed out by Joe Hershberger: http://lists.denx.de/pipermail/u-boot/2010-May/071315.html http://lists.denx.de/pipermail/u-boot/2010-June/073031.html
Regards, B. Nortmann
Changes in v2: - Add "Reviewed-by" sjg - Removed too narrow (flag & H_FORCE) expression, use "flag" directly - Fixed outdated "env_flags_varaccess_lock" to the correct "env_flags_varaccess_system"
Bernhard Nortmann (7): env: Allow unconditional access if H_PROGRAMMATIC is set net: dm: Ignore unknown env_op_* constants env: Introduce "export" operation and (access flag) restriction env: Introduce "transient" and "system" access flags sunxi: env: flag fel_* environment vars as "system" env: Introduce setenv_transient() helper function env: Automatically mark dynamic configuration info as "do not export"
cmd/net.c | 34 +++++++++++++++++----------------- cmd/nvedit.c | 24 +++++++++++++++++++++++- common/env_flags.c | 21 ++++++++++++++++++--- include/common.h | 1 + include/configs/sunxi-common.h | 8 ++++++++ include/env_flags.h | 5 ++++- include/search.h | 1 + lib/hashtable.c | 3 +++ net/eth-uclass.c | 2 ++ 9 files changed, 77 insertions(+), 22 deletions(-)
This patch modifies env_flags_validate() in such a way that any operation will *always* be allowed if H_PROGRAMMATIC is present.
Without this change, programmatically changing environment vars may fail depending on their flags, e.g. when trying to setenv*() a variable that is marked "read-only". http://lists.denx.de/pipermail/u-boot/2016-April/250237.html
Notes: H_FORCE may be insufficient for this purpose, as it can be disabled by CONFIG_ENV_ACCESS_IGNORE_FORCE. H_PROGRAMMATIC indicates "U-Boot internal" use. By contrast, any user interaction (from U-Boot prompt or scripts) is expected to be marked H_INTERACTIVE.
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de
---
Changes in v2: None
common/env_flags.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/common/env_flags.c b/common/env_flags.c index 921d377..1087f4e 100644 --- a/common/env_flags.c +++ b/common/env_flags.c @@ -523,6 +523,8 @@ int env_flags_validate(const ENTRY *item, const char *newval, enum env_op op, }
/* check for access permission */ + if (flag & H_PROGRAMMATIC) + return 0; #ifndef CONFIG_ENV_ACCESS_IGNORE_FORCE if (flag & H_FORCE) return 0;
On 16 November 2016 at 03:29, Bernhard Nortmann bernhard.nortmann@web.de wrote:
This patch modifies env_flags_validate() in such a way that any operation will *always* be allowed if H_PROGRAMMATIC is present.
Without this change, programmatically changing environment vars may fail depending on their flags, e.g. when trying to setenv*() a variable that is marked "read-only". http://lists.denx.de/pipermail/u-boot/2016-April/250237.html
Notes: H_FORCE may be insufficient for this purpose, as it can be disabled by CONFIG_ENV_ACCESS_IGNORE_FORCE. H_PROGRAMMATIC indicates "U-Boot internal" use. By contrast, any user interaction (from U-Boot prompt or scripts) is expected to be marked H_INTERACTIVE.
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de
Changes in v2: None
common/env_flags.c | 2 ++ 1 file changed, 2 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
This prevents a possible compiler warning similar to "net/eth-uclass.c:<line>:<pos>: warning: enumeration value 'env_op_*' not handled in switch [-Wswitch]".
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de Reviewed-by: Simon Glass sjg@chromium.org
---
Changes in v2: - Add "Reviewed-by" sjg
net/eth-uclass.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/net/eth-uclass.c b/net/eth-uclass.c index 1d13011..50e37da 100644 --- a/net/eth-uclass.c +++ b/net/eth-uclass.c @@ -231,6 +231,8 @@ static int on_ethaddr(const char *name, const char *value, enum env_op op, break; case env_op_delete: memset(pdata->enetaddr, 0, 6); + default: + break; /* ignore */ } }
On Wed, Nov 16, 2016 at 4:29 AM, Bernhard Nortmann bernhard.nortmann@web.de wrote:
This prevents a possible compiler warning similar to "net/eth-uclass.c:<line>:<pos>: warning: enumeration value 'env_op_*' not handled in switch [-Wswitch]".
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de Reviewed-by: Simon Glass sjg@chromium.org
Acked-by: Joe Hershberger joe.hershberger@ni.com
This patch introduces a new "export" environment operation (env_op_export) and the corresponding access flag ENV_FLAGS_VARACCESS_PREVENT_EXPORT; so that env_flags_validate() may now check requests to export specific variables.
In turn, hexport_r() makes uses of this ability to suppress the export of variables that are flagged accordingly.
Note that env_flags_validate() and hexport_r() will respect H_FORCE and H_PROGRAMMATIC flags, allowing to bypass the export filtering. H_PROGRAMMATIC gets used within env_print() to make sure all variables are listed. This is necessary because env_print() is essentially an "export to text" operation.
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de
---
Changes in v2: - Removed too narrow (flag & H_FORCE) expression, use "flag" directly
cmd/nvedit.c | 3 ++- common/env_flags.c | 8 +++++++- include/env_flags.h | 3 ++- include/search.h | 1 + lib/hashtable.c | 3 +++ 5 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c index 9ca5cb5..9a78e1d 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -101,7 +101,8 @@ static int env_print(char *name, int flag) }
/* print whole list */ - len = hexport_r(&env_htab, '\n', flag, &res, 0, 0, NULL); + len = hexport_r(&env_htab, '\n', + flag | H_PROGRAMMATIC, &res, 0, 0, NULL);
if (len > 0) { puts(res); diff --git a/common/env_flags.c b/common/env_flags.c index 1087f4e..f39d952 100644 --- a/common/env_flags.c +++ b/common/env_flags.c @@ -510,7 +510,7 @@ int env_flags_validate(const ENTRY *item, const char *newval, enum env_op op, newval = newval ? : "";
/* validate the value to match the variable type */ - if (op != env_op_delete) { + if (op != env_op_delete && op != env_op_export) { enum env_flags_vartype type = (enum env_flags_vartype) (ENV_FLAGS_VARTYPE_BIN_MASK & item->flags);
@@ -560,6 +560,12 @@ int env_flags_validate(const ENTRY *item, const char *newval, enum env_op op, return 1; } break; + case env_op_export: + if (item->flags & ENV_FLAGS_VARACCESS_PREVENT_EXPORT) { + printf("## Don't export "%s"\n", name); + return 1; + } + break; }
return 0; diff --git a/include/env_flags.h b/include/env_flags.h index 0dcec06..7e2362a 100644 --- a/include/env_flags.h +++ b/include/env_flags.h @@ -173,6 +173,7 @@ int env_flags_validate(const ENTRY *item, const char *newval, enum env_op op, #define ENV_FLAGS_VARACCESS_PREVENT_CREATE 0x00000010 #define ENV_FLAGS_VARACCESS_PREVENT_OVERWR 0x00000020 #define ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR 0x00000040 -#define ENV_FLAGS_VARACCESS_BIN_MASK 0x00000078 +#define ENV_FLAGS_VARACCESS_PREVENT_EXPORT 0x00000080 +#define ENV_FLAGS_VARACCESS_BIN_MASK 0x000000F8
#endif /* __ENV_FLAGS_H__ */ diff --git a/include/search.h b/include/search.h index 402dfd8..3df4d09 100644 --- a/include/search.h +++ b/include/search.h @@ -23,6 +23,7 @@ enum env_op { env_op_create, env_op_delete, env_op_overwrite, + env_op_export, };
/* Action which shall be performed in the call to hsearch. */ diff --git a/lib/hashtable.c b/lib/hashtable.c index f088477..399c4bb 100644 --- a/lib/hashtable.c +++ b/lib/hashtable.c @@ -621,6 +621,9 @@ ssize_t hexport_r(struct hsearch_data *htab, const char sep, int flag, if ((flag & H_HIDE_DOT) && ep->key[0] == '.') continue;
+ if (env_flags_validate(ep, NULL, env_op_export, flag)) + continue; /* don't export */ + list[n++] = ep;
totlen += strlen(ep->key) + 2;
On 16 November 2016 at 03:29, Bernhard Nortmann bernhard.nortmann@web.de wrote:
This patch introduces a new "export" environment operation (env_op_export) and the corresponding access flag ENV_FLAGS_VARACCESS_PREVENT_EXPORT; so that env_flags_validate() may now check requests to export specific variables.
In turn, hexport_r() makes uses of this ability to suppress the export of variables that are flagged accordingly.
Note that env_flags_validate() and hexport_r() will respect H_FORCE and H_PROGRAMMATIC flags, allowing to bypass the export filtering. H_PROGRAMMATIC gets used within env_print() to make sure all variables are listed. This is necessary because env_print() is essentially an "export to text" operation.
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de
Changes in v2:
- Removed too narrow (flag & H_FORCE) expression, use "flag" directly
cmd/nvedit.c | 3 ++- common/env_flags.c | 8 +++++++- include/env_flags.h | 3 ++- include/search.h | 1 + lib/hashtable.c | 3 +++ 5 files changed, 15 insertions(+), 3 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
"transient" (='t') is like "any", but requests that a variable should not be exported (ENV_FLAGS_VARACCESS_PREVENT_EXPORT).
"system" (='S') is meant for 'internal' variables that aren't supposed to be changed by the user. It corresponds to "transient" plus "read-only".
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de
---
Changes in v2: - Fixed outdated "env_flags_varaccess_lock" to the correct "env_flags_varaccess_system"
common/env_flags.c | 11 +++++++++-- include/env_flags.h | 2 ++ 2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/common/env_flags.c b/common/env_flags.c index f39d952..2c30c7f 100644 --- a/common/env_flags.c +++ b/common/env_flags.c @@ -28,7 +28,7 @@ #endif
static const char env_flags_vartype_rep[] = "sdxb" ENV_FLAGS_NET_VARTYPE_REPS; -static const char env_flags_varaccess_rep[] = "aroc"; +static const char env_flags_varaccess_rep[] = "aroctS"; static const int env_flags_varaccess_mask[] = { 0, ENV_FLAGS_VARACCESS_PREVENT_DELETE | @@ -37,7 +37,12 @@ static const int env_flags_varaccess_mask[] = { ENV_FLAGS_VARACCESS_PREVENT_DELETE | ENV_FLAGS_VARACCESS_PREVENT_OVERWR, ENV_FLAGS_VARACCESS_PREVENT_DELETE | - ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR}; + ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR, + ENV_FLAGS_VARACCESS_PREVENT_EXPORT, + ENV_FLAGS_VARACCESS_PREVENT_DELETE | + ENV_FLAGS_VARACCESS_PREVENT_CREATE | + ENV_FLAGS_VARACCESS_PREVENT_OVERWR | + ENV_FLAGS_VARACCESS_PREVENT_EXPORT};
#ifdef CONFIG_CMD_ENV_FLAGS static const char * const env_flags_vartype_names[] = { @@ -55,6 +60,8 @@ static const char * const env_flags_varaccess_names[] = { "read-only", "write-once", "change-default", + "transient", /* do not export/save */ + "system", /* = "transient" plus "read-only" */ };
/* diff --git a/include/env_flags.h b/include/env_flags.h index 7e2362a..9d66706 100644 --- a/include/env_flags.h +++ b/include/env_flags.h @@ -25,6 +25,8 @@ enum env_flags_varaccess { env_flags_varaccess_readonly, env_flags_varaccess_writeonce, env_flags_varaccess_changedefault, + env_flags_varaccess_transient, + env_flags_varaccess_system, env_flags_varaccess_end };
Hi Bernhard,
On 16 November 2016 at 03:29, Bernhard Nortmann bernhard.nortmann@web.de wrote:
"transient" (='t') is like "any", but requests that a variable should not be exported (ENV_FLAGS_VARACCESS_PREVENT_EXPORT).
"system" (='S') is meant for 'internal' variables that aren't supposed to be changed by the user. It corresponds to "transient" plus "read-only".
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de
Changes in v2:
- Fixed outdated "env_flags_varaccess_lock" to the correct "env_flags_varaccess_system"
common/env_flags.c | 11 +++++++++-- include/env_flags.h | 2 ++ 2 files changed, 11 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Please see below.
diff --git a/common/env_flags.c b/common/env_flags.c index f39d952..2c30c7f 100644 --- a/common/env_flags.c +++ b/common/env_flags.c @@ -28,7 +28,7 @@ #endif
static const char env_flags_vartype_rep[] = "sdxb" ENV_FLAGS_NET_VARTYPE_REPS; -static const char env_flags_varaccess_rep[] = "aroc"; +static const char env_flags_varaccess_rep[] = "aroctS"; static const int env_flags_varaccess_mask[] = { 0, ENV_FLAGS_VARACCESS_PREVENT_DELETE | @@ -37,7 +37,12 @@ static const int env_flags_varaccess_mask[] = { ENV_FLAGS_VARACCESS_PREVENT_DELETE | ENV_FLAGS_VARACCESS_PREVENT_OVERWR, ENV_FLAGS_VARACCESS_PREVENT_DELETE |
ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR};
ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR,ENV_FLAGS_VARACCESS_PREVENT_EXPORT,ENV_FLAGS_VARACCESS_PREVENT_DELETE |
Can these flags be shortened? This is not Java :-) Also it might be helpful to use the
[index] = value
syntax so you can see which value it corresponds to?
ENV_FLAGS_VARACCESS_PREVENT_CREATE |ENV_FLAGS_VARACCESS_PREVENT_OVERWR |ENV_FLAGS_VARACCESS_PREVENT_EXPORT};#ifdef CONFIG_CMD_ENV_FLAGS static const char * const env_flags_vartype_names[] = { @@ -55,6 +60,8 @@ static const char * const env_flags_varaccess_names[] = { "read-only", "write-once", "change-default",
"transient", /* do not export/save */"system", /* = "transient" plus "read-only" */};
/* diff --git a/include/env_flags.h b/include/env_flags.h index 7e2362a..9d66706 100644 --- a/include/env_flags.h +++ b/include/env_flags.h @@ -25,6 +25,8 @@ enum env_flags_varaccess { env_flags_varaccess_readonly, env_flags_varaccess_writeonce, env_flags_varaccess_changedefault,
env_flags_varaccess_transient,env_flags_varaccess_system, env_flags_varaccess_end};
-- 2.7.3
Regards, Simon
Hi Simon!
Am 19.11.2016 um 14:47 schrieb Simon Glass:
Hi Bernhard,
On 16 November 2016 at 03:29, Bernhard Nortmann bernhard.nortmann@web.de wrote:
"transient" (='t') is like "any", but requests that a variable should not be exported (ENV_FLAGS_VARACCESS_PREVENT_EXPORT).
"system" (='S') is meant for 'internal' variables that aren't supposed to be changed by the user. It corresponds to "transient" plus "read-only".
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de
Changes in v2:
Fixed outdated "env_flags_varaccess_lock" to the correct "env_flags_varaccess_system"
common/env_flags.c | 11 +++++++++-- include/env_flags.h | 2 ++ 2 files changed, 11 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Please see below.
[...] Can these flags be shortened? This is not Java :-) Also it might be helpful to use the
[index] = value
syntax so you can see which value it corresponds to?
[...] Regards, Simon
I like the [index] suggestion, which already gives a version that I find a lot easier to read:
diff --git a/common/env_flags.c b/common/env_flags.c index f39d952..6dea70c 100644 --- a/common/env_flags.c +++ b/common/env_flags.c @@ -28,16 +28,22 @@ #endif
static const char env_flags_vartype_rep[] = "sdxb" ENV_FLAGS_NET_VARTYPE_REPS; -static const char env_flags_varaccess_rep[] = "aroc"; +static const char env_flags_varaccess_rep[] = "aroctS"; static const int env_flags_varaccess_mask[] = { - 0, - ENV_FLAGS_VARACCESS_PREVENT_DELETE | - ENV_FLAGS_VARACCESS_PREVENT_CREATE | - ENV_FLAGS_VARACCESS_PREVENT_OVERWR, - ENV_FLAGS_VARACCESS_PREVENT_DELETE | - ENV_FLAGS_VARACCESS_PREVENT_OVERWR, - ENV_FLAGS_VARACCESS_PREVENT_DELETE | - ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR}; + [0] = 0, /* no restrictions */ + [1] = ENV_FLAGS_VARACCESS_PREVENT_DELETE + | ENV_FLAGS_VARACCESS_PREVENT_CREATE + | ENV_FLAGS_VARACCESS_PREVENT_OVERWR, + [2] = ENV_FLAGS_VARACCESS_PREVENT_DELETE + | ENV_FLAGS_VARACCESS_PREVENT_OVERWR, + [3] = ENV_FLAGS_VARACCESS_PREVENT_DELETE + | ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR, + [4] = ENV_FLAGS_VARACCESS_PREVENT_EXPORT, + [5] = ENV_FLAGS_VARACCESS_PREVENT_DELETE + | ENV_FLAGS_VARACCESS_PREVENT_CREATE + | ENV_FLAGS_VARACCESS_PREVENT_OVERWR + | ENV_FLAGS_VARACCESS_PREVENT_EXPORT + };
#ifdef CONFIG_CMD_ENV_FLAGS static const char * const env_flags_vartype_names[] = {
As for the shortening of the various flags: The only one I'm introducing is ENV_FLAGS_VARACCESS_PREVENT_EXPORT (in patch 3/7), following along the spirit of existing ones - so I might not be the right person to bust them all?
Regards, B. Nortmann
Hi Bernhard,
On 22 November 2016 at 11:54, Bernhard Nortmann bernhard.nortmann@web.de wrote:
Hi Simon!
Am 19.11.2016 um 14:47 schrieb Simon Glass:
Hi Bernhard,
On 16 November 2016 at 03:29, Bernhard Nortmann bernhard.nortmann@web.de wrote:
"transient" (='t') is like "any", but requests that a variable should not be exported (ENV_FLAGS_VARACCESS_PREVENT_EXPORT).
"system" (='S') is meant for 'internal' variables that aren't supposed to be changed by the user. It corresponds to "transient" plus "read-only".
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de
Changes in v2:
Fixed outdated "env_flags_varaccess_lock" to the correct "env_flags_varaccess_system"
common/env_flags.c | 11 +++++++++-- include/env_flags.h | 2 ++ 2 files changed, 11 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Please see below.
[...] Can these flags be shortened? This is not Java :-) Also it might be helpful to use the
[index] = value
syntax so you can see which value it corresponds to?
[...] Regards, Simon
I like the [index] suggestion, which already gives a version that I find a lot easier to read:
diff --git a/common/env_flags.c b/common/env_flags.c index f39d952..6dea70c 100644 --- a/common/env_flags.c +++ b/common/env_flags.c @@ -28,16 +28,22 @@ #endif
static const char env_flags_vartype_rep[] = "sdxb" ENV_FLAGS_NET_VARTYPE_REPS; -static const char env_flags_varaccess_rep[] = "aroc"; +static const char env_flags_varaccess_rep[] = "aroctS"; static const int env_flags_varaccess_mask[] = {
0,ENV_FLAGS_VARACCESS_PREVENT_DELETE |ENV_FLAGS_VARACCESS_PREVENT_CREATE |ENV_FLAGS_VARACCESS_PREVENT_OVERWR,ENV_FLAGS_VARACCESS_PREVENT_DELETE |ENV_FLAGS_VARACCESS_PREVENT_OVERWR,ENV_FLAGS_VARACCESS_PREVENT_DELETE |ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR};
[0] = 0, /* no restrictions */[1] = ENV_FLAGS_VARACCESS_PREVENT_DELETE| ENV_FLAGS_VARACCESS_PREVENT_CREATE| ENV_FLAGS_VARACCESS_PREVENT_OVERWR,[2] = ENV_FLAGS_VARACCESS_PREVENT_DELETE| ENV_FLAGS_VARACCESS_PREVENT_OVERWR,[3] = ENV_FLAGS_VARACCESS_PREVENT_DELETE| ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR,[4] = ENV_FLAGS_VARACCESS_PREVENT_EXPORT,[5] = ENV_FLAGS_VARACCESS_PREVENT_DELETE| ENV_FLAGS_VARACCESS_PREVENT_CREATE| ENV_FLAGS_VARACCESS_PREVENT_OVERWR| ENV_FLAGS_VARACCESS_PREVENT_EXPORT};#ifdef CONFIG_CMD_ENV_FLAGS static const char * const env_flags_vartype_names[] = {
As for the shortening of the various flags: The only one I'm introducing is ENV_FLAGS_VARACCESS_PREVENT_EXPORT (in patch 3/7), following along the spirit of existing ones - so I might not be the right person to bust them all?
Well you could add a separate patch before this one which renames everything. I don't think anyone else is working in this area.
Regards, Simon
Hi Simon,
Am 23.11.2016 um 00:08 schrieb Simon Glass:
Hi Bernhard,
[...] Well you could add a separate patch before this one which renames everything. I don't think anyone else is working in this area.
Regards, Simon
Doing so, I have arrived at the (additional) commit attached below. With that added to the series, do you think this has matured enough to promote it from "RFC" to an actual PATCH when submitting v3?
Regards, B. Nortmann
On 25 November 2016 at 03:48, Bernhard Nortmann bernhard.nortmann@web.de wrote:
Hi Simon,
Am 23.11.2016 um 00:08 schrieb Simon Glass:
Hi Bernhard,
[...] Well you could add a separate patch before this one which renames everything. I don't think anyone else is working in this area.
Regards, Simon
Doing so, I have arrived at the (additional) commit attached below. With that added to the series, do you think this has matured enough to promote it from "RFC" to an actual PATCH when submitting v3?
SGTM
On Wed, Nov 16, 2016 at 4:29 AM, Bernhard Nortmann bernhard.nortmann@web.de wrote:
"transient" (='t') is like "any", but requests that a variable should not be exported (ENV_FLAGS_VARACCESS_PREVENT_EXPORT).
"system" (='S') is meant for 'internal' variables that
The flags are positional, so 's' is not in use. It seems it would be cleaner to use a lower-case 's'.
aren't supposed to be changed by the user. It corresponds to "transient" plus "read-only".
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de
Changes in v2:
- Fixed outdated "env_flags_varaccess_lock" to the correct "env_flags_varaccess_system"
common/env_flags.c | 11 +++++++++-- include/env_flags.h | 2 ++ 2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/common/env_flags.c b/common/env_flags.c index f39d952..2c30c7f 100644 --- a/common/env_flags.c +++ b/common/env_flags.c @@ -28,7 +28,7 @@ #endif
static const char env_flags_vartype_rep[] = "sdxb" ENV_FLAGS_NET_VARTYPE_REPS; -static const char env_flags_varaccess_rep[] = "aroc"; +static const char env_flags_varaccess_rep[] = "aroctS"; static const int env_flags_varaccess_mask[] = { 0, ENV_FLAGS_VARACCESS_PREVENT_DELETE | @@ -37,7 +37,12 @@ static const int env_flags_varaccess_mask[] = { ENV_FLAGS_VARACCESS_PREVENT_DELETE | ENV_FLAGS_VARACCESS_PREVENT_OVERWR, ENV_FLAGS_VARACCESS_PREVENT_DELETE |
ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR};
ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR,ENV_FLAGS_VARACCESS_PREVENT_EXPORT,ENV_FLAGS_VARACCESS_PREVENT_DELETE |ENV_FLAGS_VARACCESS_PREVENT_CREATE |ENV_FLAGS_VARACCESS_PREVENT_OVERWR |ENV_FLAGS_VARACCESS_PREVENT_EXPORT};#ifdef CONFIG_CMD_ENV_FLAGS static const char * const env_flags_vartype_names[] = { @@ -55,6 +60,8 @@ static const char * const env_flags_varaccess_names[] = { "read-only", "write-once", "change-default",
"transient", /* do not export/save */"system", /* = "transient" plus "read-only" */
I'm not sure why you are adding "transient" or "volatile" to the varaccess. It is an orthogonal property of a variable. This is obvious from the fact that you need to add yet another to compose varaccess with varlifetime (or something).
I worked on something similar years ago, but never posted an RFC.
http://lists.denx.de/pipermail/u-boot/2010-June/073027.html
};
/* diff --git a/include/env_flags.h b/include/env_flags.h index 7e2362a..9d66706 100644 --- a/include/env_flags.h +++ b/include/env_flags.h @@ -25,6 +25,8 @@ enum env_flags_varaccess { env_flags_varaccess_readonly, env_flags_varaccess_writeonce, env_flags_varaccess_changedefault,
env_flags_varaccess_transient,env_flags_varaccess_system, env_flags_varaccess_end};
-- 2.7.3
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Hi Joe!
Thanks for chiming in, especially seeing that you have previously worked on something very similar.
Am 27.11.2016 um 19:53 schrieb Joe Hershberger:
On Wed, Nov 16, 2016 at 4:29 AM, Bernhard Nortmann bernhard.nortmann@web.de wrote:
"transient" (='t') is like "any", but requests that a variable should not be exported (ENV_FLAGS_VARACCESS_PREVENT_EXPORT).
"system" (='S') is meant for 'internal' variables that
The flags are positional, so 's' is not in use. It seems it would be cleaner to use a lower-case 's'.
You're probably right. I think back then I deliberately picked 'S' to avoid potential confusion (e.g. users specifying "s" when they actuall mean/want "ss"), as trying to set flags to just "S" would result in an error message.
For precisely that reason I'd actually prefer to find letter(s) that would not conflict with existing type or access flags, but 'v'olatile seemed ambiguous / too broad at that time.
[...]
I'm not sure why you are adding "transient" or "volatile" to the varaccess. It is an orthogonal property of a variable. This is obvious from the fact that you need to add yet another to compose varaccess with varlifetime (or something).
I worked on something similar years ago, but never posted an RFC.
That's a very good point. This 'orthogonality' is what actually caused me to come up with that "transient" vs. "system" idea. I think I got misled by the way that U-Boot already combined various "access" bit flags, and it never occured to me to introduce another property with a third flag character. Actually "?av" (any-access, volatile) and "?rv" (read-only, volatile) would represent my intent perfectly well.
I also like your "auto-volatile" concept a lot (i.e. resetting the volatile nature on setenv by the user). To stay unambiguous ('a' = "any" access), maybe this could be tagged 't'emporary. We'd also need a default letter for the lifesspan flag, possibly 'n'ormal?
The temporary (= auto-volatile) flag would also nicely save use from the need to have users fumble with "setenv .flags", and the quirks involved.
Implementing this means some refactoring / a major overhaul of this RFC series, but I think it could be well worth that. I'll definitely give it a try when I find some time.
Regards, B. Nortmann
On Wed, Nov 30, 2016 at 4:08 AM, Bernhard Nortmann bernhard.nortmann@web.de wrote:
Hi Joe!
Thanks for chiming in, especially seeing that you have previously worked on something very similar.
Sure! Sorry I didn't review this sooner.
Am 27.11.2016 um 19:53 schrieb Joe Hershberger:
On Wed, Nov 16, 2016 at 4:29 AM, Bernhard Nortmann bernhard.nortmann@web.de wrote:
"transient" (='t') is like "any", but requests that a variable should not be exported (ENV_FLAGS_VARACCESS_PREVENT_EXPORT).
"system" (='S') is meant for 'internal' variables that
The flags are positional, so 's' is not in use. It seems it would be cleaner to use a lower-case 's'.
You're probably right. I think back then I deliberately picked 'S' to avoid potential confusion (e.g. users specifying "s" when they actuall mean/want "ss"), as trying to set flags to just "S" would result in an error message.
For precisely that reason I'd actually prefer to find letter(s) that would not conflict with existing type or access flags, but 'v'olatile seemed ambiguous / too broad at that time.
Yes, that is a good point and was actually a consideration when I first chose the letters that are used for the current 2 positions. It's not a strict requirement, but it's less error-prone.
[...]
I'm not sure why you are adding "transient" or "volatile" to the varaccess. It is an orthogonal property of a variable. This is obvious from the fact that you need to add yet another to compose varaccess with varlifetime (or something).
I worked on something similar years ago, but never posted an RFC.
That's a very good point. This 'orthogonality' is what actually caused me to come up with that "transient" vs. "system" idea. I think I got misled by the way that U-Boot already combined various "access" bit flags, and it never occured to me to introduce another property with a third flag character. Actually "?av" (any-access, volatile) and "?rv" (read-only, volatile) would represent my intent perfectly well.
Perfect. That's exactly what I was proposing in 2010 (with the addition of auto-volatile).
I also like your "auto-volatile" concept a lot (i.e. resetting the volatile nature on setenv by the user). To stay unambiguous ('a' = "any" access), maybe this could be tagged 't'emporary.
't'emporary is ok, but I'm thinking something like o'v'erride(able) is more explicit about the behavior.
We'd also need a default letter for the lifesspan flag, possibly 'n'ormal?
I had suggested 'p'recious, but 'n'ormal is ok too. WD wanted a pointer type too, but I never got around to implementing it. Presumably that would conflict with 'p'recious if we went with that.
Again, it's not that important that the letters used be unique across positions, it's just less error prone. At the same time, using the first letter of a meaning more often will be less error-prone also.
The temporary (= auto-volatile) flag would also nicely save use from the need to have users fumble with "setenv .flags", and the quirks involved.
Exactly. It should be the uncommon case to need to touch the .flags.
Implementing this means some refactoring / a major overhaul of this RFC series, but I think it could be well worth that. I'll definitely give it a try when I find some time.
Awesome, thanks. -Joe
"fel_booted" and "fel_scriptaddr" have a special meaning and get used by U-Boot internally.
This patch aims at both preventing user modification of these values, and at excluding them from being stored on "saveenv".
As this is achieved by setting specialized access flags, also enable the "env flags" command.
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de ---
Changes in v2: None
include/configs/sunxi-common.h | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h index 8363414..2a7cf86 100644 --- a/include/configs/sunxi-common.h +++ b/include/configs/sunxi-common.h @@ -405,6 +405,14 @@ extern int soft_i2c_gpio_scl; "fdt ram " FDT_ADDR_R " 0x100000;" \ "ramdisk ram " RAMDISK_ADDR_R " 0x4000000\0"
+/* + * flag environment vars for FEL mode ("usb boot") as special. "system" access + * means they're marked read-only, and shouldn't be written on "saveenv". + */ +#define CONFIG_ENV_FLAGS_LIST_STATIC "fel_booted:bS,fel_scriptaddr:xS" +/* support "env flags" command */ +#define CONFIG_CMD_ENV_FLAGS + #ifdef CONFIG_MMC #define BOOT_TARGET_DEVICES_MMC(func) func(MMC, mmc, 0) #if CONFIG_MMC_SUNXI_SLOT_EXTRA != -1
Like setenv(), but automatically marks the entry as "don't export".
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de ---
Changes in v2: None
cmd/nvedit.c | 21 +++++++++++++++++++++ include/common.h | 1 + 2 files changed, 22 insertions(+)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c index 9a78e1d..fbed3df 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -300,6 +300,27 @@ int setenv(const char *varname, const char *varvalue) }
/** + * Set a "transient" environment variable + * + * Like setenv(), but this automatically marks the + * resulting entry as transient (= "do not export"). + */ +int setenv_transient(const char *varname, const char *varvalue) +{ + int rc = setenv(varname, varvalue); + if (rc == 0) { + ENTRY e, *ep; + + e.key = varname; + e.data = NULL; + hsearch_r(e, FIND, &ep, &env_htab, 0); + if (ep) + ep->flags |= ENV_FLAGS_VARACCESS_PREVENT_EXPORT; + } + return rc; +} + +/** * Set an environment variable to an integer value * * @param varname Environment variable to set diff --git a/include/common.h b/include/common.h index a8d833b..c0fd319 100644 --- a/include/common.h +++ b/include/common.h @@ -392,6 +392,7 @@ ulong getenv_hex(const char *varname, ulong default_val); int getenv_yesno(const char *var); int saveenv (void); int setenv (const char *, const char *); +int setenv_transient(const char *, const char *); int setenv_ulong(const char *varname, ulong value); int setenv_hex(const char *varname, ulong value); /**
Hi Bernhard,
On 16 November 2016 at 03:30, Bernhard Nortmann bernhard.nortmann@web.de wrote:
Like setenv(), but automatically marks the entry as "don't export".
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de
Changes in v2: None
cmd/nvedit.c | 21 +++++++++++++++++++++ include/common.h | 1 + 2 files changed, 22 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
But see below.
diff --git a/cmd/nvedit.c b/cmd/nvedit.c index 9a78e1d..fbed3df 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -300,6 +300,27 @@ int setenv(const char *varname, const char *varvalue) }
/**
- Set a "transient" environment variable
- Like setenv(), but this automatically marks the
- resulting entry as transient (= "do not export").
- */
+int setenv_transient(const char *varname, const char *varvalue) +{
int rc = setenv(varname, varvalue);if (rc == 0) {
I think returning the error right away is better here.
if (rc) return rc;
ENTRY e, *ep;e.key = varname;e.data = NULL;hsearch_r(e, FIND, &ep, &env_htab, 0);if (ep)ep->flags |= ENV_FLAGS_VARACCESS_PREVENT_EXPORT;}return rc;
return 0;
+}
+/**
- Set an environment variable to an integer value
- @param varname Environment variable to set
diff --git a/include/common.h b/include/common.h index a8d833b..c0fd319 100644 --- a/include/common.h +++ b/include/common.h @@ -392,6 +392,7 @@ ulong getenv_hex(const char *varname, ulong default_val); int getenv_yesno(const char *var); int saveenv (void); int setenv (const char *, const char *); +int setenv_transient(const char *, const char *);
Please add a function comment and parameter names.
int setenv_ulong(const char *varname, ulong value); int setenv_hex(const char *varname, ulong value); /** -- 2.7.3
Regards, SImon
Hi Simon!
Am 19.11.2016 um 14:47 schrieb Simon Glass:
Hi Bernhard,
On 16 November 2016 at 03:30, Bernhard Nortmann bernhard.nortmann@web.de wrote:
Like setenv(), but automatically marks the entry as "don't export".
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de
Changes in v2: None
cmd/nvedit.c | 21 +++++++++++++++++++++ include/common.h | 1 + 2 files changed, 22 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
But see below.
[...] I think returning the error right away is better here.
if (rc) return rc;
[...] return 0;
Makes sense, I'll change that.
[...] Please add a function comment and parameter names.
There are inconsistent coding styles here. getenv_hex() has a documentation comment in include/common.h, getenv_yesno() only a standard comment. The inline function setenv_addr() is both documented and implemented there. setenv() has no comments at all. Other functions have their documentation (next to their implementation) in cmd/nvedit.c - which is what I usually prefer over 'inflating' the header, too.
For setenv_transient() I oriented myself at the 'parent' function setenv(), but added a documentation comment in cmd/nvedit.c. I'm fine with adding @param and @return descriptions, and parameter names to include/common.h. If you insist, I could also move the description over there.
Regards, B. Nortmann
This is an attempt to prevent such information from ending up in exported environment data, especially when doing "saveenv". (http://lists.denx.de/pipermail/u-boot/2015-September/227611.html)
The patch makes use of the new setenv_transient() helper for environment variables that get updated via network configuration: BOOTP/DHCP (netboot_update_env), CDP (cdp_update_env) and link-local protocol (do_link_local).
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de ---
Changes in v2: None
cmd/net.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/cmd/net.c b/cmd/net.c index bed76e4..7ad2540 100644 --- a/cmd/net.c +++ b/cmd/net.c @@ -116,23 +116,23 @@ static void netboot_update_env(void)
if (net_gateway.s_addr) { ip_to_string(net_gateway, tmp); - setenv("gatewayip", tmp); + setenv_transient("gatewayip", tmp); }
if (net_netmask.s_addr) { ip_to_string(net_netmask, tmp); - setenv("netmask", tmp); + setenv_transient("netmask", tmp); }
if (net_hostname[0]) - setenv("hostname", net_hostname); + setenv_transient("hostname", net_hostname);
if (net_root_path[0]) - setenv("rootpath", net_root_path); + setenv_transient("rootpath", net_root_path);
if (net_ip.s_addr) { ip_to_string(net_ip, tmp); - setenv("ipaddr", tmp); + setenv_transient("ipaddr", tmp); } #if !defined(CONFIG_BOOTP_SERVERIP) /* @@ -141,32 +141,32 @@ static void netboot_update_env(void) */ if (net_server_ip.s_addr) { ip_to_string(net_server_ip, tmp); - setenv("serverip", tmp); + setenv_transient("serverip", tmp); } #endif if (net_dns_server.s_addr) { ip_to_string(net_dns_server, tmp); - setenv("dnsip", tmp); + setenv_transient("dnsip", tmp); } #if defined(CONFIG_BOOTP_DNS2) if (net_dns_server2.s_addr) { ip_to_string(net_dns_server2, tmp); - setenv("dnsip2", tmp); + setenv_transient("dnsip2", tmp); } #endif if (net_nis_domain[0]) - setenv("domain", net_nis_domain); + setenv_transient("domain", net_nis_domain);
#if defined(CONFIG_CMD_SNTP) && defined(CONFIG_BOOTP_TIMEOFFSET) if (net_ntp_time_offset) { sprintf(tmp, "%d", net_ntp_time_offset); - setenv("timeoffset", tmp); + setenv_transient("timeoffset", tmp); } #endif #if defined(CONFIG_CMD_SNTP) && defined(CONFIG_BOOTP_NTPSERVER) if (net_ntp_server.s_addr) { ip_to_string(net_ntp_server, tmp); - setenv("ntpserverip", tmp); + setenv_transient("ntpserverip", tmp); } #endif } @@ -291,14 +291,14 @@ static void cdp_update_env(void) printf("CDP offered appliance VLAN %d\n", ntohs(cdp_appliance_vlan)); vlan_to_string(cdp_appliance_vlan, tmp); - setenv("vlan", tmp); + setenv_transient("vlan", tmp); net_our_vlan = cdp_appliance_vlan; }
if (cdp_native_vlan != htons(-1)) { printf("CDP offered native VLAN %d\n", ntohs(cdp_native_vlan)); vlan_to_string(cdp_native_vlan, tmp); - setenv("nvlan", tmp); + setenv_transient("nvlan", tmp); net_native_vlan = cdp_native_vlan; } } @@ -423,14 +423,14 @@ static int do_link_local(cmd_tbl_t *cmdtp, int flag, int argc,
net_gateway.s_addr = 0; ip_to_string(net_gateway, tmp); - setenv("gatewayip", tmp); + setenv_transient("gatewayip", tmp);
ip_to_string(net_netmask, tmp); - setenv("netmask", tmp); + setenv_transient("netmask", tmp);
ip_to_string(net_ip, tmp); - setenv("ipaddr", tmp); - setenv("llipaddr", tmp); /* store this for next time */ + setenv_transient("ipaddr", tmp); + setenv_transient("llipaddr", tmp); /* store this for next time */
return CMD_RET_SUCCESS; }
Hi Bernhard,
On 16 November 2016 at 03:30, Bernhard Nortmann bernhard.nortmann@web.de wrote:
This is an attempt to prevent such information from ending up in exported environment data, especially when doing "saveenv". (http://lists.denx.de/pipermail/u-boot/2015-September/227611.html)
The patch makes use of the new setenv_transient() helper for environment variables that get updated via network configuration: BOOTP/DHCP (netboot_update_env), CDP (cdp_update_env) and link-local protocol (do_link_local).
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de
Changes in v2: None
cmd/net.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
How would you make these variables non-transient? Does the transient nature show up in 'printenv'?
Regards, Simon
Hi Simon!
Am 19.11.2016 um 14:47 schrieb Simon Glass:
Hi Bernhard,
On 16 November 2016 at 03:30, Bernhard Nortmann bernhard.nortmann@web.de wrote:
This is an attempt to prevent such information from ending up in exported environment data, especially when doing "saveenv". (http://lists.denx.de/pipermail/u-boot/2015-September/227611.html)
The patch makes use of the new setenv_transient() helper for environment variables that get updated via network configuration: BOOTP/DHCP (netboot_update_env), CDP (cdp_update_env) and link-local protocol (do_link_local).
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de
Changes in v2: None
cmd/net.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
How would you make these variables non-transient? Does the transient nature show up in 'printenv'?
Regards, Simon
I think it's best to consider a practical example. I'm firing up my sunxi-based device with these patches applied on top of U-Boot v2016.11, and interrupt the autoboot.
The standard way to examine variables that have non-default flags is the "env flags" command. In my case this outputs (discarding the explanations near the top):
Static flags: Variable Name Variable Type Variable Access ------------- ------------- --------------- eth\d?addr MAC address write-once ipaddr IP address any gatewayip IP address any netmask IP address any serverip IP address any nvlan decimal any vlan decimal any dnsip IP address any serial# string write-once fel_booted boolean system fel_scriptaddr hexadecimal system
Active flags: Variable Name Variable Type Variable Access ------------- ------------- --------------- serial# string write-once ethaddr MAC address write-once fel_booted boolean system
As you can see, patch 5/7 has sneaked in two new "system" flags and since I've booted using the FEL mechanism, the "fel_booted" one is in effect ("active").
Okay, now I use the command "setenv autoload no; dhcp" to auto-configure networking. Let's check "env flags" again. The "Static flags" section is unchanged, but at the bottom there's now:
Active flags: Variable Name Variable Type Variable Access ------------- ------------- --------------- ipaddr IP address transient serial# string write-once hostname string transient ethaddr MAC address write-once netmask IP address transient serverip IP address transient fel_booted boolean system dnsip IP address transient gatewayip IP address transient
Does the transient nature show up in 'printenv'?
No, it doesn't. "printenv" will give output that looks as usual / perfectly normal. You're also not restricted in modifying "transient" variables, for example I can do "ipaddr 10.11.12.13" with no problems.
But: The differences will become apparent when trying to export the environment with something like "saveenv" or "env export": => env export 0x40000000 ## Don't export "ipaddr" ## Don't export "hostname" ## Don't export "netmask" ## Don't export "serverip" ## Don't export "fel_booted" ## Don't export "dnsip" ## Don't export "gatewayip"
U-Boot refuses to store the 'volatile' information for those variables that have been flagged accordingly, and informs the user about it. This is checked individually per variable, i.e. the other ones will get exported normally.
At this point I decide that I want to set up a *static* IP configuration and make it persistent with "saveenv". Houston, we have a problem!
How would you make these variables non-transient?
Well, again there's a standard U-Boot mechanism for this. The README mentions it in the description of CONFIG_ENV_FLAGS_LIST_STATIC: "To override a setting in the static list, simply add an entry for the same variable name to the '.flags' variable.".
So we could do "setenv .flags ipaddr:i,netmask:i,gatewayip.i" to reset the variables to access level "any", meaning they'll get exported again.
Admittedly, there's a slight inconsistency here. Due to the fact that setenv_transient() only sets a temporary flag, and U-Boot will rebuild the access flags list (from the "static" default one) when assigning ".flags", the "transient" flags for the networking variables will get cleared anyway - even when deleting ".flags".
The alternative would be to mark these variables "transient" from the beginning (in the static flags), but I considered that more awkward - when taking into account that users might want to setup (and "saveenv") the network manually right from the start, and are less likely to override the values retrieved via auto-configuration (DHCP etc.).
Regards, B. Nortmann
Hi Bernhard,
On 22 November 2016 at 15:30, Bernhard Nortmann bernhard.nortmann@web.de wrote:
Hi Simon!
Am 19.11.2016 um 14:47 schrieb Simon Glass:
Hi Bernhard,
On 16 November 2016 at 03:30, Bernhard Nortmann bernhard.nortmann@web.de wrote:
This is an attempt to prevent such information from ending up in exported environment data, especially when doing "saveenv". (http://lists.denx.de/pipermail/u-boot/2015-September/227611.html)
The patch makes use of the new setenv_transient() helper for environment variables that get updated via network configuration: BOOTP/DHCP (netboot_update_env), CDP (cdp_update_env) and link-local protocol (do_link_local).
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de
Changes in v2: None
cmd/net.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
How would you make these variables non-transient? Does the transient nature show up in 'printenv'?
Regards, Simon
I think it's best to consider a practical example. I'm firing up my sunxi-based device with these patches applied on top of U-Boot v2016.11, and interrupt the autoboot.
The standard way to examine variables that have non-default flags is the "env flags" command. In my case this outputs (discarding the explanations near the top):
Static flags: Variable Name Variable Type Variable Access ------------- ------------- --------------- eth\d?addr MAC address write-once ipaddr IP address any gatewayip IP address any netmask IP address any serverip IP address any nvlan decimal any vlan decimal any dnsip IP address any serial# string write-once fel_booted boolean system fel_scriptaddr hexadecimal system
Active flags: Variable Name Variable Type Variable Access ------------- ------------- --------------- serial# string write-once ethaddr MAC address write-once fel_booted boolean system
As you can see, patch 5/7 has sneaked in two new "system" flags and since I've booted using the FEL mechanism, the "fel_booted" one is in effect ("active").
Okay, now I use the command "setenv autoload no; dhcp" to auto-configure networking. Let's check "env flags" again. The "Static flags" section is unchanged, but at the bottom there's now:
Active flags: Variable Name Variable Type Variable Access ------------- ------------- --------------- ipaddr IP address transient serial# string write-once hostname string transient ethaddr MAC address write-once netmask IP address transient serverip IP address transient fel_booted boolean system dnsip IP address transient gatewayip IP address transient
Does the transient nature show up in 'printenv'?
No, it doesn't. "printenv" will give output that looks as usual / perfectly normal. You're also not restricted in modifying "transient" variables, for example I can do "ipaddr 10.11.12.13" with no problems.
But: The differences will become apparent when trying to export the environment with something like "saveenv" or "env export": => env export 0x40000000 ## Don't export "ipaddr" ## Don't export "hostname" ## Don't export "netmask" ## Don't export "serverip" ## Don't export "fel_booted" ## Don't export "dnsip" ## Don't export "gatewayip"
U-Boot refuses to store the 'volatile' information for those variables that have been flagged accordingly, and informs the user about it. This is checked individually per variable, i.e. the other ones will get exported normally.
At this point I decide that I want to set up a *static* IP configuration and make it persistent with "saveenv". Houston, we have a problem!
How would you make these variables non-transient?
Well, again there's a standard U-Boot mechanism for this. The README mentions it in the description of CONFIG_ENV_FLAGS_LIST_STATIC: "To override a setting in the static list, simply add an entry for the same variable name to the '.flags' variable.".
So we could do "setenv .flags ipaddr:i,netmask:i,gatewayip.i" to reset the variables to access level "any", meaning they'll get exported again.
Admittedly, there's a slight inconsistency here. Due to the fact that setenv_transient() only sets a temporary flag, and U-Boot will rebuild the access flags list (from the "static" default one) when assigning ".flags", the "transient" flags for the networking variables will get cleared anyway - even when deleting ".flags".
The alternative would be to mark these variables "transient" from the beginning (in the static flags), but I considered that more awkward - when taking into account that users might want to setup (and "saveenv") the network manually right from the start, and are less likely to override the values retrieved via auto-configuration (DHCP etc.).
Thanks for the explanation. It looks fine to me.
Regards, Simon
participants (3)
-
Bernhard Nortmann -
Joe Hershberger -
Simon Glass