Re: [PATCH v4 3/5] env: Allow U-Boot scripts to be placed in a .env file

[Wolfgang Denk <wd@denx.de>]

Dear Simon,

In message <20210919125937.v4.3.If789ba3e2667c46c03eda3386ca84a863baeda55@changeid> you wrote:
>
...
> +It is also possible to create an environment file with the name
> +`board/<vendor>/env/<board>.env` for your board. If that file is not present
> +then U-Boot will look for `oard/<vendor>/env/common.env` so that you can

a/oard/board/

> +have a common environment for all vendor boards.

Actually it would be nice to look for `board/<vendor>/env/common.env`
first, and then for `board/<vendor>/env/<board>.env` - and if both
exost, they should be concatenated, so a vendor can keep all common
definitions in the common.env, and have only board specific
definitions in the <board>.env files - otherwise he would have to
repeat everything in the board files, and the common file makes
little sense.

> +This is a plain text file where you can type your environment variables in
> +the form `var=value`. Blank lines and multi-line variables are supported.
> +The conversion script looks for a line that starts with a letter or number
> +and has an equals sign immediately afterwards. Spaces before the = are not
> +permitted. It is a good idea to indent your scripts so that only the 'var='
> +appears at the start of a line.

This is not correct.  Variable names can be more complex:

	=> setenv _foo 1
	=> setenv ,bar 2
	=> setenv /baz 3

etc.

> +For example, for snapper9260 you would create a text file called
> +`board/bluewater/env/snapper9260.env` containing the environment text.
> +
> +Example::
> +
> +    stdout=serial
> +    #ifdef CONFIG_LCD
> +    stdout+=,lcd

Is that a new feature?  I didn't see it documented anywhere?

> +    #endif
> +    bootcmd=
> +        /* U-Boot script for booting */
> +
> +        if [ -z ${tftpserverip} ]; then
> +            echo "Use 'setenv tftpserverip a.b.c.d' to set IP address."
> +        fi
> +
> +        usb start; setenv autoload n; bootp;
> +        tftpboot ${tftpserverip}:
> +        bootm
> +    failed=
> +        /* Print a message when boot fails */
> +        echo CONFIG_SYS_BOARD boot failed - please check your image
> +        echo Load address is CONFIG_SYS_LOAD_ADDR

You _must_ define a clear syntax for the file, including indentation
rules.  Otherwise there is plenty chance for incorrect arsing.


> +	# Is this the start of a new environment variable?
> +	if (match($0, "^([^ =][^ =]*)=(.*)", arr)) {

This is not what you document above.

> +		if (length(env) != 0) {
> +			# Record the value of the variable now completed
> +			vars[var] = env
> +		}

What in case of length == 0 ?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Imitation is the sincerest form of plagarism.