| Message ID | 20200108134247.31443-1-rasmus.villemoes@prevas.dk |
|---|---|
| State | Rejected |
| Delegated to: | Wolfgang Denk |
| Headers | show |
| Series | env: introduce CONFIG_ENV_DOTVARS_TEMPORARY | expand |
Dear Rasmus Villemoes, In message <20200108134247.31443-1-rasmus.villemoes@prevas.dk> you wrote: > The printenv command already by default hides variables beginning with > a dot. It can be useful to take that convention even further, and > prevent such variables from ever being stored persistently (and > ignored if they happen to exist in stable storage). > > This way, one can freely use such variable names in script logic, > without worrying about random temporary variables leaking to > persistent storage and/or to/from another U-boot "session". This is not a good idea. The decision whether a variable shall be stored permanently or not, or wheter it is readonly or writable, and other such properties should never based on their name. There may be many good reasons that some .name variable _shall_ be persistent. Naked-by: Wolfgang Denk <wd@denx.de> Best regards, Wolfgang Denk
On 20/01/2020 17.44, Wolfgang Denk wrote: > Dear Rasmus Villemoes, > > In message <20200108134247.31443-1-rasmus.villemoes@prevas.dk> you wrote: >> The printenv command already by default hides variables beginning with >> a dot. It can be useful to take that convention even further, and >> prevent such variables from ever being stored persistently (and >> ignored if they happen to exist in stable storage). >> >> This way, one can freely use such variable names in script logic, >> without worrying about random temporary variables leaking to >> persistent storage and/or to/from another U-boot "session". > > This is not a good idea. The decision whether a variable shall be > stored permanently or not, or wheter it is readonly or writable, and > other such properties should never based on their name. Sorry, but what other property of the variable could possibly determine those things, then? There may > be many good reasons that some .name variable _shall_ be persistent. Sure, absolutely. Which is why this is entirely opt-in for those who know they won't need that, but do have some semi-complicated script that interacts with various commands that return their output via an environment variable. > Naked-by: Wolfgang Denk <wd@denx.de> Ah, now I see how env_flags_varaccess is actually implemented, involving a .flags special variable. OK, then I can certainly see why one would not want that to be excluded from the environment - I just thought the idea behind "printenv" hiding dot-variables by default was that those were considered temporary, and not special in this way. So, would you accept introducing env_flags_varaccess_temporary, for which I could then add tmp_.*:st ? Thanks, Rasmus
Dear Rasmus, In message <2676be2b-2e4f-7aba-14e6-5659174ad011@prevas.dk> you wrote: > > > This is not a good idea. The decision whether a variable shall be > > stored permanently or not, or wheter it is readonly or writable, and > > other such properties should never based on their name. > > Sorry, but what other property of the variable could possibly determine > those things, then? Such properties are stored in the .flags settings, see env/flags.c > There may > > be many good reasons that some .name variable _shall_ be persistent. > > Sure, absolutely. Which is why this is entirely opt-in for those who > know they won't need that, but do have some semi-complicated script that > interacts with various commands that return their output via an > environment variable. This has been discussed several times before (for example in the context of UEFI persistance handling); it should be implemented using the existing .flags mechanism. > Ah, now I see how env_flags_varaccess is actually implemented, > involving a .flags special variable. OK, then I can certainly see why > one would not want that to be excluded from the environment - I just > thought the idea behind "printenv" hiding dot-variables by default was > that those were considered temporary, and not special in this way. Not only that, .flags is exactly the mechanism that should be used to implement what you want. > So, would you accept introducing env_flags_varaccess_temporary, for > which I could then add tmp_.*:st ? Please look up the last discussion of this topic; see the thread "efi_loader: implementing non-volatile UEFI variables" starting here: https://lists.denx.de/pipermail/u-boot/2019-June/373503.html Best regards, Wolfgang Denk
diff --git a/env/Kconfig b/env/Kconfig index 4661082f0e..69fd2cae03 100644 --- a/env/Kconfig +++ b/env/Kconfig @@ -559,6 +559,16 @@ config SYS_RELOC_GD_ENV_ADDR Relocate the early env_addr pointer so we know it is not inside the binary. Some systems need this and for the rest, it doesn't hurt. +config ENV_DOTVARS_TEMPORARY + bool "Ignore variables beginning with . when saving/loading the environment" + help + If you select this option, environment variable names + beginning with a dot (.) are skipped when writing the + environment to persistent storage. Similarly, should the + persistent storage somehow contain such a variable, it is + ignored (i.e. not added to the runtime environment) when + loading. + config USE_DEFAULT_ENV_FILE bool "Create default environment from file" help diff --git a/env/common.c b/env/common.c index 0da21ee081..c23b490364 100644 --- a/env/common.c +++ b/env/common.c @@ -116,6 +116,7 @@ int env_set_default_vars(int nvars, char * const vars[], int flags) int env_import(const char *buf, int check) { env_t *ep = (env_t *)buf; + int flag = IS_ENABLED(CONFIG_ENV_DOTVARS_TEMPORARY) ? H_HIDE_DOT : 0; if (check) { uint32_t crc; @@ -128,7 +129,7 @@ int env_import(const char *buf, int check) } } - if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0, 0, + if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', flag, 0, 0, NULL)) { gd->flags |= GD_FLG_ENV_READY; return 0; @@ -212,9 +213,10 @@ int env_export(env_t *env_out) { char *res; ssize_t len; + int flag = IS_ENABLED(CONFIG_ENV_DOTVARS_TEMPORARY) ? H_HIDE_DOT : 0; res = (char *)env_out->data; - len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL); + len = hexport_r(&env_htab, '\0', flag, &res, ENV_SIZE, 0, NULL); if (len < 0) { pr_err("Cannot export environment: errno = %d\n", errno); return 1; diff --git a/lib/hashtable.c b/lib/hashtable.c index 907e8a642f..e05a097c75 100644 --- a/lib/hashtable.c +++ b/lib/hashtable.c @@ -928,6 +928,9 @@ int himport_r(struct hsearch_data *htab, if (!drop_var_from_set(name, nvars, localvars)) continue; + if ((flag & H_HIDE_DOT) && *name == '.') + continue; + /* enter into hash table */ e.key = name; e.data = value;
The printenv command already by default hides variables beginning with a dot. It can be useful to take that convention even further, and prevent such variables from ever being stored persistently (and ignored if they happen to exist in stable storage). This way, one can freely use such variable names in script logic, without worrying about random temporary variables leaking to persistent storage and/or to/from another U-boot "session". Shell variables can be used somewhat similarly, but they are not as flexible, since many helper commands (e.g. setexpr, fdt) offer to store their output in an environment variable, not a shell variable. Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- env/Kconfig | 10 ++++++++++ env/common.c | 6 ++++-- lib/hashtable.c | 3 +++ 3 files changed, 17 insertions(+), 2 deletions(-)