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

[Simon Glass]

Hi Rasmus,

On Mon, 25 Oct 2021 at 01:06, Rasmus Villemoes
<rasmus.villemoes at prevas.dk> wrote:
>
> On 24/10/2021 21.54, Simon Glass wrote:
> > Hi Rasmus,
> >
> > On Fri, 22 Oct 2021 at 00:41, Rasmus Villemoes
> > <rasmus.villemoes at prevas.dk> wrote:
> >>
> >> On 21/10/2021 18.03, Tom Rini wrote:
> >>> On Thu, Oct 21, 2021 at 09:59:38AM -0600, Simon Glass wrote:
> >>>> Hi Marek,
> >>>>
> >>>> On Thu, 21 Oct 2021 at 07:28, Marek Behún <marek.behun at nic.cz> wrote:
> >>>>>
> >>>>> On Thu, 21 Oct 2021 15:25:37 +0200
> >>>>> Marek Behún <marek.behun at nic.cz> wrote:
> >>>>>
> >>>>>> Hello,
> >>>>>>
> >>>>>> On Thu, 21 Oct 2021 15:06:51 +0200
> >>>>>> Wolfgang Denk <wd at denx.de> wrote:
> >>>>>>
> >>>>>>> I confirm that '+=' looks better.  But '+=" is technically broken.
> >>>>>>
> >>>>>> a bit of my opinion:
> >>>>>> I think =+ will confuse far more people than + as last character of var
> >>>>>> name working weirdly. But I also think that + should be supported as
> >>>>>> last character. Therefore I propose backslash escaping in variable name,
> >>>>>> 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.
> >>>>
> >>>> My distance second preference is what Marek has here, using a
> >>>> backslash to escape the + character.
> >>>
> >>> How bad does it make the parser look if we allow trailing + in variable
> >>> names, by escaping them?  It's seemingly the substantive objection at
> >>> this point.
> >>>
> >>
> >> So I don't understand all the bruhahaha around a + at the end of the
> >> varname. Nobody suggests (that I have seen) changing anything about how
> >> U-Boot at runtime interprets and handles variables, so all variable
> >> names that used to be valid continue to be so.
> >>
> >> It's just that this _new_ method of generating the environment places
> >> certain restrictions on what can be done. The old-fashioned ways
> >> (mkenvimage, good'ol CONFIG_ENV_EXTRA_SETTINGS with all its warts, and
> >> run-time 'env set') continue to allow people to define whatever env vars
> >> they want. This new method is meant for transitioning in-tree boards'
> >> default environment, and no in-tree boards need anything exotic.
> >>
> >> Also, completely independent of whether the subsequent parser is
> >> implemented in awk or python or rust, or what syntax it accepts and the
> >> semantics of that syntax, the fact that we first pass the input through
> >> cpp already means some things just won't work the same way as when given
> >> to mkenvimage, and that applies to both sides of the =:
> >>
> >> printf 'x/* huh */y=/* where did this go ? */\nmsg=I like unix\nfive
> >>  spaces=5spaces\n' | gcc -E -P -x assembler-with-cpp -
> >>
> >> x y=
> >> msg=I like 1
> >> five spaces=5spaces
> >>
> >> [In case its broken by the email, there are actually five spaces in the
> >> printf string between the words "five" and "spaces", the above should
> >> illustrate that cpp collapses those to a single space].
> >>
> >> So, I'd much rather we do a cleaner break, and accept (and ignore)
> >> whitespace on either side of the assignment operator - that's how Make
> >> variable assignments work IIRC. And since a lot of people making use of
> >> this will already be familiar with Yocto, I think we should have two
> >> compound assignment operators, .= and +=. That still allows one to use
> >> any non-whitespace, non-= characters (modulo the restrictions imposed by
> >> the whole thing passing through cpp) in variable names, so
> >>
> >> foo+=abc
> >>
> >> means
> >>
> >> foo+ = abc
> >>
> >> while could append to foo by saying
> >>
> >> foo += abc
> >>
> >> That whitespace around the assignment operators would also make the
> >> input files somewhat more readable - there really is no point in having
> >> human-readable, human-editable text files having a format
> >> almost-but-not-quite be that of the on-disk format.
> >
> > I am OK with this on the name front, as I assume we don't want to
> > allow space in a var name!
>
> Exactly, there's really never any case where that would be sensible. But
> I would probably go a bit further and simply restrict varnames to the
> usual alphanumerics plus [_.+-] - in particular, exclude single and
> double quotes and backslash. That leaves the door open for somebody to
> later support "arbitrary" variable names by defining what it would mean
> to say e.g.
>
> "abc \n'\"\tfoo" = hahaha
>
> or whatever syntax they'd propose, but there's absolutely no point in
> implementing anything like that initially.
>
> > But how do I do this?
> >
> > bootargs=console=fred
> > #ifdef SOMETHING
> > bootargs+= dm-verity=...
> > #endif
> >
> > We need the space between the bootargs.
>
> Exactly like that, and for the case where you want to append something
> _without_ an extra space there's the .= operator I also suggested.

Do you have a link to the docs for that?

Perhaps we should get this initial thing in and we can take it from
there. I expect that as we start to convert more environments we'll
find more things we need.

>
> FWIW, I like the idea that an indented line represents a continuation of
> the value of the current assignment.
>
> I don't think I've seen it addressed, but how do you deal with CONFIG_
> string items? In the config.h file, the macro definition includes the
> double quotes, but I don't think one wants that in the env values.
>
> conf = CONFIG_DEFAULT_CONF
> bootcmd = bootm $loadaddr#$conf
>
> would attempt to boot e.g. 0x12340000#"foo", and while one can do
> de-quotification in U-Boot shell, it's rather cumbersome.

Badly. I really don't like the stringify stuff so we don't have that
problem, but we have this one.

I would much prefer it be automatic, if possible. This needs some
thought...e.g. I wonder what the default behaviour should be?

In your example, conf would end up being set to "fred" with the quote,
for example, which seem like the wrong default behaviour. Perhaps we
do need something like:

conf=_dequote(CONFIG_DEFAULT_CONF)

Regards,
Simon