Add option -r to env import to allow import of text files with CRLF as line endings

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 <[email protected]>

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.

Quoted 3 lines · 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.

Quoted 4 lines

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...

Quoted 4 lines · Stephen Warren

Quoted 3 lines · 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.

Quoted 8 lines

Quoted 4 lines

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 <[email protected]>
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:

Quoted 5 lines

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;" \

Quoted 14 lines · Alexander Holler

Am 31.07.2014 21:57, schrieb Stephen Warren:

Quoted 5 lines

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.

Quoted 7 lines

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.

Quoted 24 lines · Stephen Warren

Quoted 21 lines · Alexander Holler

Am 31.07.2014 21:57, schrieb Stephen Warren:

Quoted 5 lines

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;" \

Quoted 2 lines

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.

Quoted 24 lines · Stephen Warren

Quoted 21 lines · Alexander Holler

Am 31.07.2014 21:57, schrieb Stephen Warren:

Quoted 5 lines

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;" \

Quoted 2 lines

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.

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.

[snip]

Quoted 8 lines

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.

On Thu, Aug 14, 2014 at 01:50:31PM -0600, Stephen Warren wrote:

[snip]

Quoted 8 lines

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.

Quoted 12 lines · Stephen Warren

[snip]

Quoted 8 lines

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.

Quoted 5 lines

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

Quoted 7 lines

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:

Quoted 5 lines

                                "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.

Am 14.08.2014 17:49, schrieb Stephen Warren:

Quoted 16 lines · Alexander Holler

Quoted 14 lines

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;" \

Quoted 3 lines

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).

Quoted 22 lines

Am 14.08.2014 17:49, schrieb Stephen Warren:

Quoted 20 lines · Alexander Holler

Quoted 8 lines

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:

Quoted 7 lines

"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.

Quoted 4 lines

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:

Quoted 28 lines · Alexander Holler

Quoted 26 lines

Am 14.08.2014 17:49, schrieb Stephen Warren:

Quoted 17 lines · Alexander Holler

Quoted 8 lines

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:

Quoted 7 lines

"env import -t -r $loadaddr
$filesize;" \
"if test -n \"$uenvcmd\"; then " \
"echo \"Running uenvcmd

...\";" \
                                        "run uenvcmd;" \
                                "fi;" \

Quoted 3 lines

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).

Quoted 2 lines

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.

Quoted 30 lines

Am 14.08.2014 21:51, schrieb Stephen Warren:

Quoted 29 lines · Alexander Holler

Quoted 27 lines

Am 14.08.2014 17:49, schrieb Stephen Warren:

Quoted 17 lines · Alexander Holler

Quoted 8 lines

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:

Quoted 7 lines

"env import -t -r $loadaddr
$filesize;" \
"if test -n \"$uenvcmd\"; then " \
"echo \"Running uenvcmd

...\";" \
                                        "run uenvcmd;" \
                                "fi;" \

Quoted 4 lines

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).

Quoted 7 lines

Quoted 2 lines

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".

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.

Dear Alexander Holler,

Quoted 3 lines · In message <[email protected]> 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.

Quoted 4 lines

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?

Am 01.08.2014 14:08, schrieb Wolfgang Denk:

Quoted 2 lines

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,

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.

Dear Alexander,

Quoted 3 lines · In message <[email protected]> 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.

Quoted 3 lines

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.

Quoted 2 lines

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.

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. ;)

Am 03.08.2014 19:51, schrieb Wolfgang Denk:

Quoted 19 lines

Dear Alexander,

Quoted 3 lines · In message <[email protected]> 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.

Quoted 3 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

Alexander Holler <[email protected]> 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.

At least I've understood it such that nobody still has an objection=20 against the new feature for "env import -t" (-r).

Dear Alexander,

Quoted 3 lines · In message <[email protected]> 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".

Am 06.08.2014 08:43, schrieb Wolfgang Denk:

Quoted 9 lines

Dear Alexander,

Quoted 3 lines · In message <[email protected]> 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.

Am 06.08.2014 08:43, schrieb Wolfgang Denk:

Quoted 4 lines

Dear Alexander,

Quoted 1 line · In message <[email protected]> you wrote:

Quoted 5 lines

Quoted 2 lines

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.

Quoted 8 lines · Alexander Holler

Am 06.08.2014 08:43, schrieb Wolfgang Denk:

Quoted 7 lines

Dear Alexander,

Quoted 3 lines · In message <[email protected]> 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).

Quoted 5 lines

Quoted 2 lines

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:

Quoted 13 lines

Quoted 11 lines · Alexander Holler

Am 06.08.2014 08:43, schrieb Wolfgang Denk:

Quoted 9 lines

Dear Alexander,

Quoted 3 lines · In message <[email protected]> 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".

Quoted 6 lines

Quoted 2 lines

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.

Quoted 3 lines

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.

Use the new option -r for env import.

Signed-off-by Alexander Holler <[email protected]>

Use the new option -r for env import.

Signed-off-by Alexander Holler <[email protected]>

Use the new option -r for env import.

Signed-off-by Alexander Holler <[email protected]>