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

From: Tom Rini <trini@konsulko.com>
To: Wolfgang Denk <wd@denx.de>
Cc: "Simon Glass" <sjg@chromium.org>,
	"Marek Behún" <marek.behun@nic.cz>,
	"U-Boot Mailing List" <u-boot@lists.denx.de>,
	"Rasmus Villemoes" <rasmus.villemoes@prevas.dk>,
	"Heinrich Schuchardt" <xypron.glpk@gmx.de>,
	"Joe Hershberger" <joe.hershberger@ni.com>
Subject: Re: [PATCH v9 3/7] env: Allow U-Boot scripts to be placed in a .env file
Date: Fri, 22 Oct 2021 10:50:12 -0400	[thread overview]
Message-ID: <20211022145012.GH3577824@bill-the-cat> (raw)
In-Reply-To: <3763165.1634890015@gemini.denx.de>

[-- Attachment #1: Type: text/plain, Size: 4032 bytes --]

On Fri, Oct 22, 2021 at 10:06:55AM +0200, Wolfgang Denk wrote:
> Dear Simon,
> 
> In message <CAPnjgZ1BuAJC6Vhp06HrifU9jZqbOEuC+zRauV1DH3rY1qZp3Q@mail.gmail.com> you wrote:
> >
> > > > i.e.
> > > >   var+=value
> > > > appends value to var, while
> > > >   var\+=value
> > > > sets variable with name "var+"
> >
> > My first preference is to disallow + at the end of an end var. Perhaps
> > we can start printing a warning if people do it, for a few releases.
> 
> This might seem to be a harmless change, but it is actually a
> fundamental one.  And it breaks backward compatiility.  And all this
> without need, as a list of alternatives have been suggested.
> 
> > My distance second preference is what Marek has here, using a
> > backslash to escape the + character.
> 
> Actually this has the same problem, as the backslash is also a legal
> character in a variable name:
> 
> 	=> setenv foo\\+ bar
> 	=> printenv foo\\+  
> 	foo\+=bar
> 
> 
> Yes, it was probably not a good idea not to restrict the allowed
> character set when I implemented this stuff 21 years ago, but then
> code size was critical - we had U-Boot running from 128 kB EPROM
> (you remember these huge chips which were erased under UV light?).
> 
> The fact is, '=' and NUL are the only characters that cannot be used
> in a variable name.
> 
> 
> > As for =+ ...while I can see how people might parse it (we are setting
> > the var equal to what it has with an appending string) I think it is a
> > terrible idea as it is just not what people expect.
> 
> What do people expect? This is a totally new feature, so people will
> use what they find in the documentation and in example code.
> 
> > Also, putting the
> > + after the = places (similarly unlikely) restrictions on the
> > expression.
> 
> There is a fundamental difference here.
> 
> For the '+=' case, there is no way to escape the '+', as all
> commonly used escapes are valid characters in the variable name,
> too.
> 
> With '=+', the '=' defines where the variable name ends, and from
> here you can define your own rules as where the value part begins -
> this is just a matter how you implement your parser.
> 
> > The current format is basically the same as 'print'. So if I can't
> > have the first preference, we could ensure that it prints a \ in the
> > case that the var ends with +
> 
> But '\' is a legal character in the variable name, too. Anything but
> '=' and NUL is a legal char. And this makes escaping impossible:
> 
> 	=> setenv \'foo\\-\' foobar
> 	=> printenv \'foo\\-\'
> 	'foo\-'=foobar
> 
> > > Also, I think that it would be better if spaces and tabs were allowed
> > > to indent the .env file, i.e.
> > >
> > >         var_a   =       3
> > >         var_bcd =       7
> > >
> > > should set "var_a" to "3", "var_bcd" to "7".
> > >
> > > If special character are needed in either name or value, they could be
> > > escaped and/or quoted.
> >
> > They are allowed in the value but are reduced to a single space in the
> > front. We need this for multi-line strings (but I'm a bit worried
> > about it).
> 
> You mean this automatically insert a newline between parts? ugh...
> I didn't realize this. Did I miss it in the documentation?
> 
> > We could update it to skip any leading space after the = I think.
> 
> So what if you need a leading space?
> 
> 
> > I don't like spaces before the = though. It doesn't match the 'print'
> > output (which has no space) and it is confusing:
> 
> env print also does not add any spaces after the '='.
> 
> > I think we need strict rules so it is easy for people to get exactly
> > the env they want.
> 
> Strict rules, proper documentation, and a set of examples.

And sanity and restrictions introduced to our environment variables.
The amount of "fun" things that were allowed by disallowing only NUL and
= from names, and also allowing us to stay crazy tiny are just not
relevant to where we are now.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]