[U-Boot] [PATCH 0/4] Add option -r to env import to allow import of text files with CRLF as line endings
Hello,
time passed by, the usage of uEnv.txt is now more widespread and some maintainers might be more experienced. So now maybe they understand why it might be a good idea to let Windows users create a *working* uEnv.txt too.
So here is my second and last attempt. I've added patches for some board configs too.
Regards,
Alexander Holler
When this option is enabled, CRLF is treated like LF when importing environments from text files, which means CRs ('\r') in front of LFs ('\n') are just ignored.
Drawback of enabling this option is that (maybe exported) variables which have a trailing CR in their content will get imported without that CR. But this drawback is very unlikely and the big advantage of letting Windows user create a *working* uEnv.txt too is likely more welcome.
Signed-off-by: Alexander Holler holler@ahsoftware.de --- common/cmd_nvedit.c | 19 +++++++++++++++---- common/env_common.c | 6 +++--- include/search.h | 3 ++- lib/hashtable.c | 17 ++++++++++++++++- 4 files changed, 36 insertions(+), 9 deletions(-)
diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c index e6c3395..855808c 100644 --- a/common/cmd_nvedit.c +++ b/common/cmd_nvedit.c @@ -950,11 +950,15 @@ sep_err:
#ifdef CONFIG_CMD_IMPORTENV /* - * env import [-d] [-t | -b | -c] addr [size] + * env import [-d] [-t [-r] | -b | -c] addr [size] * -d: delete existing environment before importing; * otherwise overwrite / append to existion definitions * -t: assume text format; either "size" must be given or the * text data must be '\0' terminated + * -r: handle CRLF like LF, that means exported variables with + * a content which ends with \r won't get imported. Used + * to import text files created with editors which are using CRLF + * for line endings. Only effective in addition to -t. * -b: assume binary format ('\0' separated, "\0\0" terminated) * -c: assume checksum protected environment format * addr: memory address to read from @@ -970,6 +974,7 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, int chk = 0; int fmt = 0; int del = 0; + int crlf_is_lf = 0; size_t size;
cmd = *argv; @@ -994,6 +999,9 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, goto sep_err; sep = '\n'; break; + case 'r': /* handle CRLF like LF */ + crlf_is_lf = 1; + break; case 'd': del = 1; break; @@ -1009,6 +1017,9 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, if (!fmt) printf("## Warning: defaulting to text format\n");
+ if (sep != '\n' && crlf_is_lf ) + crlf_is_lf = 0; + addr = simple_strtoul(argv[0], NULL, 16); ptr = map_sysmem(addr, 0);
@@ -1050,8 +1061,8 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, ptr = (char *)ep->data; }
- if (himport_r(&env_htab, ptr, size, sep, del ? 0 : H_NOCLEAR, 0, - NULL) == 0) { + if (himport_r(&env_htab, ptr, size, sep, del ? 0 : H_NOCLEAR, + crlf_is_lf, 0, NULL) == 0) { error("Environment import failed: errno = %d\n", errno); return 1; } @@ -1180,7 +1191,7 @@ static char env_help_text[] = #endif #endif #if defined(CONFIG_CMD_IMPORTENV) - "env import [-d] [-t | -b | -c] addr [size] - import environment\n" + "env import [-d] [-t [-r] | -b | -c] addr [size] - import environment\n" #endif "env print [-a | name ...] - print environment\n" #if defined(CONFIG_CMD_RUN) diff --git a/common/env_common.c b/common/env_common.c index cd7b4cd..af372a6 100644 --- a/common/env_common.c +++ b/common/env_common.c @@ -120,7 +120,7 @@ void set_default_env(const char *s) }
if (himport_r(&env_htab, (char *)default_environment, - sizeof(default_environment), '\0', flags, + sizeof(default_environment), '\0', flags, 0, 0, NULL) == 0) error("Environment import failed: errno = %d\n", errno);
@@ -137,7 +137,7 @@ int set_default_vars(int nvars, char * const vars[]) */ return himport_r(&env_htab, (const char *)default_environment, sizeof(default_environment), '\0', - H_NOCLEAR | H_INTERACTIVE, nvars, vars); + H_NOCLEAR | H_INTERACTIVE, 0, nvars, vars); }
#ifdef CONFIG_ENV_AES @@ -214,7 +214,7 @@ int env_import(const char *buf, int check) return ret; }
- if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0, + if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0, 0, 0, NULL)) { gd->flags |= GD_FLG_ENV_READY; return 1; diff --git a/include/search.h b/include/search.h index ae3efc4..9701efb 100644 --- a/include/search.h +++ b/include/search.h @@ -102,7 +102,8 @@ extern ssize_t hexport_r(struct hsearch_data *__htab, */ extern int himport_r(struct hsearch_data *__htab, const char *__env, size_t __size, const char __sep, - int __flag, int nvars, char * const vars[]); + int __flag, int __crlf_is_lf, int nvars, + char * const vars[]);
/* Walk the whole table calling the callback on each element */ extern int hwalk_r(struct hsearch_data *__htab, int (*callback)(ENTRY *)); diff --git a/lib/hashtable.c b/lib/hashtable.c index 4356b23..18ed590 100644 --- a/lib/hashtable.c +++ b/lib/hashtable.c @@ -776,7 +776,7 @@ static int drop_var_from_set(const char *name, int nvars, char * vars[])
int himport_r(struct hsearch_data *htab, const char *env, size_t size, const char sep, int flag, - int nvars, char * const vars[]) + int crlf_is_lf, int nvars, char * const vars[]) { char *data, *sp, *dp, *name, *value; char *localvars[nvars]; @@ -841,6 +841,21 @@ int himport_r(struct hsearch_data *htab, } }
+ if(!size) + return 1; /* everything OK */ + if(crlf_is_lf) { + /* Remove Carriage Returns in front of Line Feeds */ + unsigned ignored_crs = 0; + for(;dp < data + size && *dp; ++dp) { + if(*dp == '\r' && + dp < data + size - 1 && *(dp+1) == '\n') + ++ignored_crs; + else + *(dp-ignored_crs) = *dp; + } + size -= ignored_crs; + dp = data; + } /* Parse environment; allow for '\0' and 'sep' as separators */ do { ENTRY e, *rv;
On Mon, Jul 14, 2014 at 05:49:55PM +0200, Alexander Holler wrote:
When this option is enabled, CRLF is treated like LF when importing environments from text files, which means CRs ('\r') in front of LFs ('\n') are just ignored.
Drawback of enabling this option is that (maybe exported) variables which have a trailing CR in their content will get imported without that CR. But this drawback is very unlikely and the big advantage of letting Windows user create a *working* uEnv.txt too is likely more welcome.
Signed-off-by: Alexander Holler holler@ahsoftware.de
Applied to u-boot/master, thanks!
On 07/14/2014 09:49 AM, Alexander Holler wrote:
When this option is enabled, CRLF is treated like LF when importing environments from text files, which means CRs ('\r') in front of LFs ('\n') are just ignored.
Drawback of enabling this option is that (maybe exported) variables which have a trailing CR in their content will get imported without that CR. But this drawback is very unlikely and the big advantage of letting Windows user create a *working* uEnv.txt too is likely more welcome.
This patch doesn't seem to have been applied, yet patches 2..4 in the series were. This means that various boot scripts use "env import -t -r ..." which fails due to the unknown option -r...
On Wed, Jul 30, 2014 at 04:47:57PM -0600, Stephen Warren wrote:
On 07/14/2014 09:49 AM, Alexander Holler wrote:
When this option is enabled, CRLF is treated like LF when importing environments from text files, which means CRs ('\r') in front of LFs ('\n') are just ignored.
Drawback of enabling this option is that (maybe exported) variables which have a trailing CR in their content will get imported without that CR. But this drawback is very unlikely and the big advantage of letting Windows user create a *working* uEnv.txt too is likely more welcome.
This patch doesn't seem to have been applied, yet patches 2..4 in the series were. This means that various boot scripts use "env import -t -r ..." which fails due to the unknown option -r...
commit ecd1446fe1df00d7f7b9de286dba309d93b51870 Author: Alexander Holler holler@ahsoftware.de Date: Mon Jul 14 17:49:55 2014 +0200
Add option -r to env import to allow import of text files with CRLF as line
is what I have.
On 07/31/2014 01:51 PM, Tom Rini wrote:
On Wed, Jul 30, 2014 at 04:47:57PM -0600, Stephen Warren wrote:
On 07/14/2014 09:49 AM, Alexander Holler wrote:
When this option is enabled, CRLF is treated like LF when importing environments from text files, which means CRs ('\r') in front of LFs ('\n') are just ignored.
Drawback of enabling this option is that (maybe exported) variables which have a trailing CR in their content will get imported without that CR. But this drawback is very unlikely and the big advantage of letting Windows user create a *working* uEnv.txt too is likely more welcome.
This patch doesn't seem to have been applied, yet patches 2..4 in the series were. This means that various boot scripts use "env import -t -r ..." which fails due to the unknown option -r...
commit ecd1446fe1df00d7f7b9de286dba309d93b51870 Author: Alexander Holler holler@ahsoftware.de Date: Mon Jul 14 17:49:55 2014 +0200
Add option -r to env import to allow import of text files with CRLF as line
is what I have.
Huh, I do see that now. I must have been looking at the content of common/cmd_nvedit.c from the wrong branch, which didn't include that patch. I could have sworn I checked git history too, but evidently not. It is indeed clearly there right before the patches which use it. Sorry for the noise.
Am 31.07.2014 21:57, schrieb Stephen Warren:
Huh, I do see that now. I must have been looking at the content of common/cmd_nvedit.c from the wrong branch, which didn't include that patch. I could have sworn I checked git history too, but evidently not. It is indeed clearly there right before the patches which use it. Sorry for the noise.
As I've just remembered where I did see your name before, the config for the rpi (as found in 2004.04) misses the uenvcmd. That's necessary to execute commands when using uEnv.txt.
It's easily done with something like the following:
"env import -t -r $loadaddr $filesize;" \ "if test -n "$uenvcmd"; then " \ "echo "Running uenvcmd ...";" \ "run uenvcmd;" \ "fi;" \
Regards,
Alexander Holler
On 08/14/2014 02:25 AM, Alexander Holler wrote:
Am 31.07.2014 21:57, schrieb Stephen Warren:
Huh, I do see that now. I must have been looking at the content of common/cmd_nvedit.c from the wrong branch, which didn't include that patch. I could have sworn I checked git history too, but evidently not. It is indeed clearly there right before the patches which use it. Sorry for the noise.
As I've just remembered where I did see your name before, the config for the rpi (as found in 2004.04) misses the uenvcmd. That's necessary to execute commands when using uEnv.txt.
It's easily done with something like the following:
"env import -t -r $loadaddr $filesize;" \ "if test -n \"$uenvcmd\"; then " \ "echo \"Running uenvcmd ...\";" \ "run uenvcmd;" \ "fi;" \
My intention was that uEnv.txt be used to set up environment variables, not to allow its use for custom scripts.
On Thu, Aug 14, 2014 at 10:49 AM, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/14/2014 02:25 AM, Alexander Holler wrote:
Am 31.07.2014 21:57, schrieb Stephen Warren:
Huh, I do see that now. I must have been looking at the content of common/cmd_nvedit.c from the wrong branch, which didn't include that patch. I could have sworn I checked git history too, but evidently not. It is indeed clearly there right before the patches which use it. Sorry for the noise.
As I've just remembered where I did see your name before, the config for the rpi (as found in 2004.04) misses the uenvcmd. That's necessary to execute commands when using uEnv.txt.
It's easily done with something like the following:
"env import -t -r $loadaddr $filesize;" \ "if test -n \"$uenvcmd\"; then " \ "echo \"Running uenvcmd ...\";" \ "run uenvcmd;" \ "fi;" \My intention was that uEnv.txt be used to set up environment variables, not to allow its use for custom scripts.
The check for if uenvcmd is set then run uenvcmd syntax, should really be pushed into the distro default stuff. As that syntax is used by default for a lot of different targets in u-boot. Most users who deal with u-boot (even if they don't want to) seem to understand it.
Regards,
On Thu, Aug 14, 2014 at 01:41:16PM -0500, Robert Nelson wrote:
On Thu, Aug 14, 2014 at 10:49 AM, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/14/2014 02:25 AM, Alexander Holler wrote:
Am 31.07.2014 21:57, schrieb Stephen Warren:
Huh, I do see that now. I must have been looking at the content of common/cmd_nvedit.c from the wrong branch, which didn't include that patch. I could have sworn I checked git history too, but evidently not. It is indeed clearly there right before the patches which use it. Sorry for the noise.
As I've just remembered where I did see your name before, the config for the rpi (as found in 2004.04) misses the uenvcmd. That's necessary to execute commands when using uEnv.txt.
It's easily done with something like the following:
"env import -t -r $loadaddr $filesize;" \ "if test -n \"$uenvcmd\"; then " \ "echo \"Running uenvcmd ...\";" \ "run uenvcmd;" \ "fi;" \My intention was that uEnv.txt be used to set up environment variables, not to allow its use for custom scripts.
The check for if uenvcmd is set then run uenvcmd syntax, should really be pushed into the distro default stuff. As that syntax is used by default for a lot of different targets in u-boot. Most users who deal with u-boot (even if they don't want to) seem to understand it.
Right. The intention was to provide a "just do what I want you to do" hook to the end user.
On 08/14/2014 12:41 PM, Robert Nelson wrote:
On Thu, Aug 14, 2014 at 10:49 AM, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/14/2014 02:25 AM, Alexander Holler wrote:
Am 31.07.2014 21:57, schrieb Stephen Warren:
Huh, I do see that now. I must have been looking at the content of common/cmd_nvedit.c from the wrong branch, which didn't include that patch. I could have sworn I checked git history too, but evidently not. It is indeed clearly there right before the patches which use it. Sorry for the noise.
As I've just remembered where I did see your name before, the config for the rpi (as found in 2004.04) misses the uenvcmd. That's necessary to execute commands when using uEnv.txt.
It's easily done with something like the following:
"env import -t -r $loadaddr $filesize;" \ "if test -n \"$uenvcmd\"; then " \ "echo \"Running uenvcmd ...\";" \ "run uenvcmd;" \ "fi;" \My intention was that uEnv.txt be used to set up environment variables, not to allow its use for custom scripts.
The check for if uenvcmd is set then run uenvcmd syntax, should really be pushed into the distro default stuff. As that syntax is used by default for a lot of different targets in u-boot. Most users who deal with u-boot (even if they don't want to) seem to understand it.
I don't think this is anything to do with distro defaults.
Distro defaults are intended to provide a single common interface between the bootloader and Linux/... distro. As such, locating and loading extlinux.conf fits the bill there. The whole idea is that distros/OSs wouldn't have to know anything about the bootloader at all; command script formats, etc.
uenv.txt is the opposite; it's very U-Boot specific, and more about internal implementation details of U-Boot. In particular, I only see a use-case for uenv.txt on systems that have nowhere to store the U-Boot environment other than in some filesystem. That's the reason the RPi port loads uenv.txt, so the environment can be modified somehow. Perhaps there's an ENV_IS_IN_FAT that could be used instead on the Pi? For example, none of the Tegra boards use uEnv.txt since "saveenv" to flash works there.
So, perhaps I could see providing include/common_bootenv.h to make the definition you wrote above common between boards, but I certainly would not expect that opting in to distro defaults automatically added support for uEnv.txt.
On Thu, Aug 14, 2014 at 01:50:31PM -0600, Stephen Warren wrote:
[snip]
uenv.txt is the opposite; it's very U-Boot specific, and more about internal implementation details of U-Boot. In particular, I only see a use-case for uenv.txt on systems that have nowhere to store the U-Boot environment other than in some filesystem. That's the reason the RPi port loads uenv.txt, so the environment can be modified somehow. Perhaps there's an ENV_IS_IN_FAT that could be used instead on the Pi? For example, none of the Tegra boards use uEnv.txt since "saveenv" to flash works there.
Even with ENV_IS_IN_FAT you need to be in U-Boot to modify the environment (fw_setenv/printenv should be adaptable easily enough I would hope, but aren't today). uEnv.txt is the way for a user to pop the SD card into their PC, tweak the env as needed (or fiddle some bits), eject the card and boot their target.
Am 14.08.2014 22:53, schrieb Tom Rini:
On Thu, Aug 14, 2014 at 01:50:31PM -0600, Stephen Warren wrote:
[snip]
uenv.txt is the opposite; it's very U-Boot specific, and more about internal implementation details of U-Boot. In particular, I only see a use-case for uenv.txt on systems that have nowhere to store the U-Boot environment other than in some filesystem. That's the reason the RPi port loads uenv.txt, so the environment can be modified somehow. Perhaps there's an ENV_IS_IN_FAT that could be used instead on the Pi? For example, none of the Tegra boards use uEnv.txt since "saveenv" to flash works there.
Even with ENV_IS_IN_FAT you need to be in U-Boot to modify the environment (fw_setenv/printenv should be adaptable easily enough I would hope, but aren't today). uEnv.txt is the way for a user to pop the SD card into their PC, tweak the env as needed (or fiddle some bits), eject the card and boot their target.
Yes, many "developers" today (those which do buy development boards) are having problems to use a serial which most of the time is needed to access the u-boot command line. The reasons are various, most devices people do use don't have a serial anymore, the voltage of the serial changes every few years (12, 5, 3.3 and now 1.8 Volt), sometimes a nullmodem (just 3 wires) is needed, ...
Whatever the reason is, sometimes it can be very hard to access the u-boot command line. But most are able to modifying or create a file on disk. ;)
Regards,
Alexander Holler
On 08/14/2014 02:53 PM, Tom Rini wrote:
On Thu, Aug 14, 2014 at 01:50:31PM -0600, Stephen Warren wrote:
[snip]
uenv.txt is the opposite; it's very U-Boot specific, and more about internal implementation details of U-Boot. In particular, I only see a use-case for uenv.txt on systems that have nowhere to store the U-Boot environment other than in some filesystem. That's the reason the RPi port loads uenv.txt, so the environment can be modified somehow. Perhaps there's an ENV_IS_IN_FAT that could be used instead on the Pi? For example, none of the Tegra boards use uEnv.txt since "saveenv" to flash works there.
Even with ENV_IS_IN_FAT you need to be in U-Boot to modify the environment (fw_setenv/printenv should be adaptable easily enough I would hope, but aren't today). uEnv.txt is the way for a user to pop the SD card into their PC, tweak the env as needed (or fiddle some bits), eject the card and boot their target.
What, you don't link binary-editing the file to fix the CRC? :-P
Am 14.08.2014 23:35, schrieb Stephen Warren:
On 08/14/2014 02:53 PM, Tom Rini wrote:
On Thu, Aug 14, 2014 at 01:50:31PM -0600, Stephen Warren wrote:
[snip]
uenv.txt is the opposite; it's very U-Boot specific, and more about internal implementation details of U-Boot. In particular, I only see a use-case for uenv.txt on systems that have nowhere to store the U-Boot environment other than in some filesystem. That's the reason the RPi port loads uenv.txt, so the environment can be modified somehow. Perhaps there's an ENV_IS_IN_FAT that could be used instead on the Pi? For example, none of the Tegra boards use uEnv.txt since "saveenv" to flash works there.
Even with ENV_IS_IN_FAT you need to be in U-Boot to modify the environment (fw_setenv/printenv should be adaptable easily enough I would hope, but aren't today). uEnv.txt is the way for a user to pop the SD card into their PC, tweak the env as needed (or fiddle some bits), eject the card and boot their target.
What, you don't link binary-editing the file to fix the CRC? :-P
That reminds me that I thought about adding uEnv.sha1 (and CMD_SHA1) to have the same protection (or even better through sha1) as boot.cmd for environments where it makes sense. So in regard to "untrustworthy" sd-cards most boards. ;)
Regards,
Alexander Holler
Am 14.08.2014 17:49, schrieb Stephen Warren:
On 08/14/2014 02:25 AM, Alexander Holler wrote:
As I've just remembered where I did see your name before, the config for the rpi (as found in 2004.04) misses the uenvcmd. That's necessary to execute commands when using uEnv.txt.
It's easily done with something like the following:
"env import -t -r $loadaddr $filesize;" \ "if test -n \"$uenvcmd\"; then " \ "echo \"Running uenvcmd ...\";" \ "run uenvcmd;" \ "fi;" \My intention was that uEnv.txt be used to set up environment variables, not to allow its use for custom scripts.
Sure. In most cases changing the predefined available variables is enough. But it's a very hand option if someone wants or needs to do stuff which can't be done by just changing some environment variables (one never knows what ideas people will have).
And as it now seems to be possible to write the environment back to disk too, one should be make sure that uenvcmd will be cleared before writing the environment back to disk. Personally I prefer to not let the bootloader write to any (real) filesystem, but just in case someone does so ...
Regards,
Alexander Holler
On 08/14/2014 01:38 PM, Alexander Holler wrote:
Am 14.08.2014 17:49, schrieb Stephen Warren:
On 08/14/2014 02:25 AM, Alexander Holler wrote:
As I've just remembered where I did see your name before, the config for the rpi (as found in 2004.04) misses the uenvcmd. That's necessary to execute commands when using uEnv.txt.
It's easily done with something like the following:
"env import -t -r $loadaddr$filesize;" \ "if test -n "$uenvcmd"; then " \ "echo "Running uenvcmd ...";" \ "run uenvcmd;" \ "fi;" \
My intention was that uEnv.txt be used to set up environment variables, not to allow its use for custom scripts.
Sure. In most cases changing the predefined available variables is enough. But it's a very hand option if someone wants or needs to do stuff which can't be done by just changing some environment variables (one never knows what ideas people will have).
For such presumably non-standard things, why can't the user simply edit $bootcmd, and pre-pend whatever they want?
Am 14.08.2014 21:51, schrieb Stephen Warren:
On 08/14/2014 01:38 PM, Alexander Holler wrote:
Am 14.08.2014 17:49, schrieb Stephen Warren:
On 08/14/2014 02:25 AM, Alexander Holler wrote:
As I've just remembered where I did see your name before, the config for the rpi (as found in 2004.04) misses the uenvcmd. That's necessary to execute commands when using uEnv.txt.
It's easily done with something like the following:
"env import -t -r $loadaddr$filesize;" \ "if test -n "$uenvcmd"; then " \ "echo "Running uenvcmd ...";" \ "run uenvcmd;" \ "fi;" \
My intention was that uEnv.txt be used to set up environment variables, not to allow its use for custom scripts.
Sure. In most cases changing the predefined available variables is enough. But it's a very hand option if someone wants or needs to do stuff which can't be done by just changing some environment variables (one never knows what ideas people will have).
For such presumably non-standard things, why can't the user simply edit $bootcmd, and pre-pend whatever they want?
Depends on when the bootcmd will be constructed. Usually that is done after having read uEnv.txt to include variables defined in uEnv.txt in bootcmd. So whatever bootcmd one sets in uEnv.txt, it just will be overwritten.
Regards,
Alexander Holler
On 08/14/2014 01:59 PM, Alexander Holler wrote:
Am 14.08.2014 21:51, schrieb Stephen Warren:
On 08/14/2014 01:38 PM, Alexander Holler wrote:
Am 14.08.2014 17:49, schrieb Stephen Warren:
On 08/14/2014 02:25 AM, Alexander Holler wrote:
As I've just remembered where I did see your name before, the config for the rpi (as found in 2004.04) misses the uenvcmd. That's necessary to execute commands when using uEnv.txt.
It's easily done with something like the following:
"env import -t -r $loadaddr$filesize;" \ "if test -n "$uenvcmd"; then " \ "echo "Running uenvcmd ...";" \ "run uenvcmd;" \ "fi;" \
My intention was that uEnv.txt be used to set up environment variables, not to allow its use for custom scripts.
Sure. In most cases changing the predefined available variables is enough. But it's a very hand option if someone wants or needs to do stuff which can't be done by just changing some environment variables (one never knows what ideas people will have).
For such presumably non-standard things, why can't the user simply edit $bootcmd, and pre-pend whatever they want?
Depends on when the bootcmd will be constructed. Usually that is done after having read uEnv.txt to include variables defined in uEnv.txt in bootcmd. So whatever bootcmd one sets in uEnv.txt, it just will be overwritten.
What would over-write bootcmd? None of the boards I've looked at auto-generates bootcmd. bootargs perhaps (which is a string passed to the kernel) but not bootargs (which is a U-Boot command sequence that U-Boot executes automatically at boot).
If some board does auto-generate bootcmd, I'd suggest that it not. The static bootcmd could execute some kind of user-(or uenv-)set variable and/or the auto-generation of bootcmd could happen before uenv.txt was pulled in, so that whatever was in uenv.txt would have ultimate "power".
Am 14.08.2014 22:08, schrieb Stephen Warren:
On 08/14/2014 01:59 PM, Alexander Holler wrote:
Am 14.08.2014 21:51, schrieb Stephen Warren:
On 08/14/2014 01:38 PM, Alexander Holler wrote:
Am 14.08.2014 17:49, schrieb Stephen Warren:
On 08/14/2014 02:25 AM, Alexander Holler wrote:
As I've just remembered where I did see your name before, the config for the rpi (as found in 2004.04) misses the uenvcmd. That's necessary to execute commands when using uEnv.txt.
It's easily done with something like the following:
"env import -t -r $loadaddr$filesize;" \ "if test -n "$uenvcmd"; then " \ "echo "Running uenvcmd ...";" \ "run uenvcmd;" \ "fi;" \
My intention was that uEnv.txt be used to set up environment variables, not to allow its use for custom scripts.
Sure. In most cases changing the predefined available variables is enough. But it's a very hand option if someone wants or needs to do stuff which can't be done by just changing some environment variables (one never knows what ideas people will have).
For such presumably non-standard things, why can't the user simply edit $bootcmd, and pre-pend whatever they want?
Depends on when the bootcmd will be constructed. Usually that is done after having read uEnv.txt to include variables defined in uEnv.txt in bootcmd. So whatever bootcmd one sets in uEnv.txt, it just will be overwritten.
What would over-write bootcmd? None of the boards I've looked at auto-generates bootcmd. bootargs perhaps (which is a string passed to the kernel) but not bootargs (which is a U-Boot command sequence that U-Boot executes automatically at boot).
If some board does auto-generate bootcmd, I'd suggest that it not. The static bootcmd could execute some kind of user-(or uenv-)set variable and/or the auto-generation of bootcmd could happen before uenv.txt was pulled in, so that whatever was in uenv.txt would have ultimate "power".
Ah, yes. Sorry, I confused bootcmd with bootargs (I don't live in u-boot and just fiddle once a year or such with it).
But overwriting bootcmd needs to read uEnv.txt in PREBOOT (or how it is named). I originally have read uEnv.txt in the bootcmd itself, so overwriting it didn't work. But I don't want to dive too deep into that discussion, as I think it's up to the board-maintainers to write the config however they want and seem to fit for there users. I've just mentioned the uenvcmd, because it was the first, I've added to my u-boot for the rpi (to have the same interface I use with my other boards). ;)
Regards,
Alexander Holler
Dear Alexander Holler,
In message 1405352998-7707-2-git-send-email-holler@ahsoftware.de you wrote:
When this option is enabled, CRLF is treated like LF when importing environments from text files, which means CRs ('\r') in front of LFs ('\n') are just ignored.
Drawback of enabling this option is that (maybe exported) variables which have a trailing CR in their content will get imported without that CR. But this drawback is very unlikely and the big advantage of letting Windows user create a *working* uEnv.txt too is likely more welcome.
Should we not, for reasons of symmetry, then also extend "env export" by such a "-r" option?
Best regards,
Wolfgang Denk
Am 01.08.2014 14:08, schrieb Wolfgang Denk:
Dear Alexander Holler,
In message 1405352998-7707-2-git-send-email-holler@ahsoftware.de you wrote:
When this option is enabled, CRLF is treated like LF when importing environments from text files, which means CRs ('\r') in front of LFs ('\n') are just ignored.
Drawback of enabling this option is that (maybe exported) variables which have a trailing CR in their content will get imported without that CR. But this drawback is very unlikely and the big advantage of letting Windows user create a *working* uEnv.txt too is likely more welcome.
Should we not, for reasons of symmetry, then also extend "env export" by such a "-r" option?
Sorry, but I don't follow the new features of u-boot that closely.
Is it already possible to save an exported environment as (text-)file to some storage? Such wasn't possible when I've implemented that -r for "env import" and it doesn't make much sense if an exported environment never reaches users.
Another problem would be to decide in which format to save the environment. Same magic (e.g. an env var set by "env import") would be necessary to decided in which format to save the environment.
Regards,
Alexander Holler
Am 02.08.2014 23:09, schrieb Alexander Holler:
Am 01.08.2014 14:08, schrieb Wolfgang Denk:
Should we not, for reasons of symmetry, then also extend "env export" by such a "-r" option?
Sorry, but I don't follow the new features of u-boot that closely.
Is it already possible to save an exported environment as (text-)file to some storage? Such wasn't possible when I've implemented that -r for "env import" and it doesn't make much sense if an exported environment never reaches users.
Just to clarify: I see uEnv.txt (which only was possible through your env import implementation) as a read-only configuration file for u-boot, mainly used to easily configure the kernel-command-line from userspace. Something like grub.cfg or any config for other bootloaders. The (simple) trick with uenvcmd to execute commands is just a handy addition.
And I don't think all the necessary stuff to save a file in all the possible filesystems should end up in u-boot. Modifying filesystems is dangerous.
So from a u-boot point of view uEnv.txt is a read-only mechanism and I'm happy with it as such.
I just did the patch in the subject because it ended up with extremly hard to diagnose problems when someone created an uEnv.txt with CRLF using Windows. E.g. foo=bar in such an uEnv.txt was in fact foo=bar<CR> which was feeded to the kernel command line as foo=bar<CR> too, and the Linux kernel usually treads carriage returns as a normal character. So it treats bar<CR> as something different than bar, leading to various failures. And that underlying problem is almost impossible to see because everything (what a user pastes, kernel output, ...) looks good.
Regards,
Alexander Holler
Dear Alexander,
In message 53DE658F.5010703@ahsoftware.de you wrote:
Just to clarify: I see uEnv.txt (which only was possible through your env import implementation) as a read-only configuration file for u-boot,
This is just one of the many possible usages.
And I don't think all the necessary stuff to save a file in all the possible filesystems should end up in u-boot. Modifying filesystems is dangerous.
Thius has nothing to do with exporting an environment. The export operation and the writing to the file system are two separate steps. If a file system driver contains write support or not depends on the file system code. For the environment it does not matter. If we have write support, we just use it.
So from a u-boot point of view uEnv.txt is a read-only mechanism and I'm happy with it as such.
As mentioned, this is but one usage.
I think that "env import" / "env export" should be kept symmetric.
Best regards,
Wolfgang Denk
Am 03.08.2014 19:51, schrieb Wolfgang Denk:
Dear Alexander,
In message 53DE658F.5010703@ahsoftware.de you wrote:
Just to clarify: I see uEnv.txt (which only was possible through your env import implementation) as a read-only configuration file for u-boot,
This is just one of the many possible usages.
And I don't think all the necessary stuff to save a file in all the possible filesystems should end up in u-boot. Modifying filesystems is dangerous.
Thius has nothing to do with exporting an environment. The export operation and the writing to the file system are two separate steps. If a file system driver contains write support or not depends on the file system code. For the environment it does not matter. If we have write support, we just use it.
So from a u-boot point of view uEnv.txt is a read-only mechanism and I'm happy with it as such.
As mentioned, this is but one usage.
I think that "env import" / "env export" should be kept symmetric.
Using a \r\n instead of \n when -r is used for env export should be something like 4 liner or such.
But it would not be really symmetric. The -r for "env import" makes "env import" eat both formats, which means it can be used almost always, but using -r with "env export" would be a decision which always would be wrong for many people.
Of course, adding the possibility to export the environment in a system-foreign format (Assuming nobody boots windows using u-boot) doesn't really make a harm, it just adds the danger that people will use -r for "env export" because it is used for "env import" too, which most likely would be wrong for most usage scenarios.
Anyway, I don't have any other objections agains a -r for "env export", maybe it could be added to the TODO-list which contains documentation for "env *" too. ;)
Regards,
Alexander Holler
Am 04.08.2014 08:47, schrieb Alexander Holler:
But it would not be really symmetric. The -r for "env import" makes "env import" eat both formats, which means it can be used almost always, but using -r with "env export" would be a decision which always would be wrong for many people.
Of course, adding the possibility to export the environment in a system-foreign format (Assuming nobody boots windows using u-boot) doesn't really make a harm, it just adds the danger that people will use -r for "env export" because it is used for "env import" too, which most likely would be wrong for most usage scenarios.
Anyway, I don't have any other objections agains a -r for "env export", maybe it could be added to the TODO-list which contains documentation for "env *" too. ;)
My slow brain (therefor this second message) now suggests to add e.g. a -w to "env export", where the -w would be mutually exclusive to -t. This would make it clear that it doesn't mean the same as '-t -r' for env import and that using -w means a decision which might be wrong.
Regards,
Alexander Holler
Alexander Holler holler@ahsoftware.de writes:
Am 03.08.2014 19:51, schrieb Wolfgang Denk:
Dear Alexander,
In message 53DE658F.5010703@ahsoftware.de you wrote:
Just to clarify: I see uEnv.txt (which only was possible through your env import implementation) as a read-only configuration file for u-boot,
This is just one of the many possible usages.
And I don't think all the necessary stuff to save a file in all the possible filesystems should end up in u-boot. Modifying filesystems is dangerous.
Thius has nothing to do with exporting an environment. The export operation and the writing to the file system are two separate steps. If a file system driver contains write support or not depends on the file system code. For the environment it does not matter. If we have write support, we just use it.
So from a u-boot point of view uEnv.txt is a read-only mechanism and I'm happy with it as such.
As mentioned, this is but one usage.
I think that "env import" / "env export" should be kept symmetric.
Using a \r\n instead of \n when -r is used for env export should be something like 4 liner or such.
But it would not be really symmetric. The -r for "env import" makes "env import" eat both formats, which means it can be used almost always, but using -r with "env export" would be a decision which always would be wrong for many people.
Of course, adding the possibility to export the environment in a system-foreign format (Assuming nobody boots windows using u-boot) doesn't really make a harm, it just adds the danger that people will use -r for "env export" because it is used for "env import" too, which most likely would be wrong for most usage scenarios.
Why not just make "env import" treat \r like any other whitespace? It would be a slight change from current behaviour, but the chance that someone actually relies on a trailing \r being part of the value is vanishingly small.
Am 04.08.2014 12:00, schrieb Måns Rullgård:
Alexander Holler holler@ahsoftware.de writes:
Why not just make "env import" treat \r like any other whitespace? It would be a slight change from current behaviour, but the chance that someone actually relies on a trailing \r being part of the value is vanishingly small.
There was an objection about such wheh I've first posted the patch whichg already has had the same strong check as the current patch. But I think nobody still has a problem with that, and whatever was said long time ago was based on some misunderstandings about what the patch is for, and how it really works and how the new feature is used.
And currently we are only talking if and how enhancing "env export".
At least I've understood it such that nobody still has an objection against the new feature for "env import -t" (-r).
Regards,
Alexander Holler
Dear Alexander,
In message 53DFDC99.2020206@ahsoftware.de you wrote:
At least I've understood it such that nobody still has an objection=20 against the new feature for "env import -t" (-r).
I object if it would be added without maintaining symmetry with "env export".
Best regards,
Wolfgang Denk
Am 06.08.2014 08:43, schrieb Wolfgang Denk:
Dear Alexander,
In message 53DFDC99.2020206@ahsoftware.de you wrote:
At least I've understood it such that nobody still has an objection=20 against the new feature for "env import -t" (-r).
I object if it would be added without maintaining symmetry with "env export".
As explained I don't know how to add symmetry. It's impossible to export text concurrently in both formats.
And I will not write a patch (I don't need), if I have to assume I'm wasting my time because it will never reach any possible user. So without consensus about what such a symmetry feature for "env export" should be and how it will look like I will not spend the (little) time to write a patch, nor the much large time to discuss the patch afterwards.
Regards,
Alexander Holler
Am 06.08.2014 12:02, schrieb Alexander Holler:
Am 06.08.2014 08:43, schrieb Wolfgang Denk:
Dear Alexander,
In message 53DFDC99.2020206@ahsoftware.de you wrote:
At least I've understood it such that nobody still has an objection=20 against the new feature for "env import -t" (-r).
I object if it would be added without maintaining symmetry with "env export".
As explained I don't know how to add symmetry. It's impossible to export text concurrently in both formats.
And I will not write a patch (I don't need), if I have to assume I'm wasting my time because it will never reach any possible user. So without consensus about what such a symmetry feature for "env export" should be and how it will look like I will not spend the (little) time to write a patch, nor the much large time to discuss the patch afterwards.
Maybe it helps to explain my motivation to write the patch in the subject:
I've run into the problem once, when I had to use a Windows box to write an uEnv.txt, and it happens extremly seldom that I'm using Windows. But then I've seen several other people running into the same problem (which is extremly hard to identify as <CR> is usually invisible). So I though I'm nice and write a patch to help other people (because I can write such a patch and I don't need to spend much time to do so) and everyone will be happy about.
So to conclude I don't need the -r for "env import" myself and it just ended up with around a dozen mails where I had to defend and explain the patch. That isn't what motivates to spend time writing "maintainer friendly" patches and to submit them.
Regards,
Alexander Holler
Alexander Holler holler@ahsoftware.de writes:
Am 06.08.2014 08:43, schrieb Wolfgang Denk:
Dear Alexander,
In message 53DFDC99.2020206@ahsoftware.de you wrote:
At least I've understood it such that nobody still has an objection=20 against the new feature for "env import -t" (-r).
I object if it would be added without maintaining symmetry with "env export".
As explained I don't know how to add symmetry. It's impossible to export text concurrently in both formats.
What is the problem with ignoring \r on input and not writing it on output?
Am 06.08.2014 12:44, schrieb Måns Rullgård:
Alexander Holler holler@ahsoftware.de writes:
Am 06.08.2014 08:43, schrieb Wolfgang Denk:
Dear Alexander,
In message 53DFDC99.2020206@ahsoftware.de you wrote:
At least I've understood it such that nobody still has an objection=20 against the new feature for "env import -t" (-r).
I object if it would be added without maintaining symmetry with "env export".
As explained I don't know how to add symmetry. It's impossible to export text concurrently in both formats.
What is the problem with ignoring \r on input and not writing it on output?
None. Please read the mails before.
Am 06.08.2014 13:18, schrieb Alexander Holler:
Am 06.08.2014 12:44, schrieb Måns Rullgård:
Alexander Holler holler@ahsoftware.de writes:
Am 06.08.2014 08:43, schrieb Wolfgang Denk:
Dear Alexander,
In message 53DFDC99.2020206@ahsoftware.de you wrote:
At least I've understood it such that nobody still has an objection=20 against the new feature for "env import -t" (-r).
I object if it would be added without maintaining symmetry with "env export".
As explained I don't know how to add symmetry. It's impossible to export text concurrently in both formats.
What is the problem with ignoring \r on input and not writing it on output?
None. Please read the mails before.
And just in case it has become difficult to follow this thread because the subject should have changed to something else, I think what Wolfgang Denk wants is an option to extent "env export" to export the environment to text files with CRLF.
I've made a suggestion here:
http://lists.denx.de/pipermail/u-boot/2014-August/185272.html
But as there was no response until now (I'm not impatient, I'm just mentioning it again because I think you've missed about what Wolfgang Denk started to discuss). it's just an assumption from me, I don't have any clue what he means with symmetry in regard to that "-r".
And without any consensus I will not spend the time to write the few lines of source to implement that (I don't need it). As written before, I don't even care much if -r for "env export -t" ends up in mainline U-Boot.
So there is no problem, Wolfgang Denk and I are just discussing if and how to extend "env export". That is at least how I do now understand this thread about the simple patch I've posted some years ago.
Regards,
Alexander Holler
Dear Alexander,
In message 53DD53A5.3010502@ahsoftware.de you wrote:
Should we not, for reasons of symmetry, then also extend "env export" by such a "-r" option?
Sorry, but I don't follow the new features of u-boot that closely.
This is not a new feature.
Is it already possible to save an exported environment as (text-)file to some storage? Such wasn't possible when I've implemented that -r for "env import" and it doesn't make much sense if an exported environment never reaches users.
"env import" and "env export" have always been symmetric. In the same way you can (and could) import the environment from a memory reagion (not directoly form a file) you can export it to one. So all you need is the capability to read a file into a memory reagion resp. to write a file from one. Eventually file write support was not present yet when you looked at this, but now at least (V)FAT and ext4 include write support (fatwrite, ext4write).
Another problem would be to decide in which format to save the environment. Same magic (e.g. an env var set by "env import") would be necessary to decided in which format to save the environment.
This is a decision made by the user, ha has to know what he wants to do resp. which format the file he is reading has or the file he is writing shall have.
Best regards,
Wolfgang Denk
Use the new option -r for env import.
Signed-off-by: Alexander Holler holler@ahsoftware.de --- include/configs/omap3_beagle.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/configs/omap3_beagle.h b/include/configs/omap3_beagle.h index 3782049..9ba031d 100644 --- a/include/configs/omap3_beagle.h +++ b/include/configs/omap3_beagle.h @@ -195,7 +195,7 @@ "bootenv=uEnv.txt\0" \ "loadbootenv=fatload mmc ${mmcdev} ${loadaddr} ${bootenv}\0" \ "importbootenv=echo Importing environment from mmc ...; " \ - "env import -t $loadaddr $filesize\0" \ + "env import -t -r $loadaddr $filesize\0" \ "ramargs=setenv bootargs console=${console} " \ "${optargs} " \ "mpurate=${mpurate} " \
On Mon, Jul 14, 2014 at 05:49:56PM +0200, Alexander Holler wrote:
Use the new option -r for env import.
Signed-off-by: Alexander Holler holler@ahsoftware.de
Applied to u-boot/master, thanks!
Use the new option -r for env import.
Signed-off-by: Alexander Holler holler@ahsoftware.de --- include/configs/am335x_evm.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/configs/am335x_evm.h b/include/configs/am335x_evm.h index a48b386..34d27c6 100644 --- a/include/configs/am335x_evm.h +++ b/include/configs/am335x_evm.h @@ -117,7 +117,7 @@ "bootenv=uEnv.txt\0" \ "loadbootenv=load mmc ${mmcdev} ${loadaddr} ${bootenv}\0" \ "importbootenv=echo Importing environment from mmc ...; " \ - "env import -t $loadaddr $filesize\0" \ + "env import -t -r $loadaddr $filesize\0" \ "ramargs=setenv bootargs console=${console} " \ "${optargs} " \ "root=${ramroot} " \
On Mon, Jul 14, 2014 at 05:49:57PM +0200, Alexander Holler wrote:
Use the new option -r for env import.
Signed-off-by: Alexander Holler holler@ahsoftware.de
Applied to u-boot/master, thanks!
Use the new option -r for env import.
Signed-off-by: Alexander Holler holler@ahsoftware.de --- include/configs/rpi_b.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/configs/rpi_b.h b/include/configs/rpi_b.h index ff48598..60f2489 100644 --- a/include/configs/rpi_b.h +++ b/include/configs/rpi_b.h @@ -98,7 +98,7 @@ #define CONFIG_SYS_CONSOLE_IS_IN_ENV #define CONFIG_PREBOOT \ "if load mmc 0:1 ${loadaddr} /uEnv.txt; then " \ - "env import -t ${loadaddr} ${filesize}; " \ + "env import -t -r ${loadaddr} ${filesize}; " \ "fi"
#define ENV_DEVICE_SETTINGS \
On Mon, Jul 14, 2014 at 05:49:58PM +0200, Alexander Holler wrote:
Use the new option -r for env import.
Signed-off-by: Alexander Holler holler@ahsoftware.de
Applied to u-boot/master, thanks!
participants (6)
-
Alexander Holler -
Måns Rullgård -
Robert Nelson -
Stephen Warren -
Tom Rini -
Wolfgang Denk