[PATCH] env: Allow string CONFIG options in the text environment

[Simon Glass]

Hi Tom,


On Mon, 7 Nov 2022 at 10:29, Tom Rini <trini at konsulko.com> wrote:
>
> On Mon, Nov 07, 2022 at 04:33:28PM +0000, Holger Brunck wrote:
> > Hi Tom,
> >
> > > > > On Fri, 4 Nov 2022 at 08:20, Holger Brunck
> > > > > <holger.brunck at hitachienergy.com> wrote:
> > > > >>
> > > > >> Hi Simon,
> > > > >> I got no time to try it yet but I have a general comment.
> > > > >>
> > > > >>>
> > > > >>> Sometimes it is useful to include a CONFIG option that contains a string.
> > > > >>> This is hard to do in general, since in many cases it is useful to
> > > > >>> have the quotes around the string so that, for example:
> > > > >>>
> > > > >>
> > > > >> wouldn't it be cleaner to always convert a Kconfig option which is
> > > > >> defined as a string to a string without the double quotes? If
> > > > >> someone needs them he could explicitly add them with
> > > > >>
> > > > >> bootcmd=run "CONFIG_BOARD_CMD"
> > > > >>
> > > > >> Because  in my case I have some options I use them to build
> > > > >> together the kernel command line I pass to the kernel.  Ok I could
> > > > >> store them before in an own variable and them use them with
> > > > >> ${variable} in the command line. But I think it would be cleaner to
> > > > >> always convert a string defined in Kconfig in a string without the quotes.
> > > What do you think?
> > > > >
> > > > > Yes I would prefer that to. I'm not sure how to implement it though.
> > > > > Any thoughts?
> > > >
> > > > I agree that special-casing the RHS containing a single qouted string
> > > > is a bad idea, it's really hard to understand and explain what the rules are.
> > > >
> > > > Unfortunately, I don't think we can just create a separate version of
> > > > the config.h header where the quotes are removed and then as Holger
> > > > suggests rely on including the double quotes when needed, because then
> > > > the C preprocessor would see "CONFIG_BOARD_CMD" as a string literal,
> > > > inside which macro expansion is not done.
> > > >
> > > > What we really want is to separate the two uses of the config values:
> > > > "control" and "data". One use is on conditional lines
> > > >
> > > > #if IS_ENABLED(CONFIG_FOO)
> > > >
> > > > and another is the case of substituting values into RHS values.
> > > >
> > > > It is really convenient to use the C preprocessor for the former. But
> > > > for the latter, it's kind of overkill, and we could probably just as
> > > > well implement a simple perl script (or python or whatnot) that would
> > > > do what we want, including stripping of double quotes from string
> > > > items before substitution. But adding that as a pre-pre-processor step
> > > > (only doing substitution on lines not beginning with a #) would break
> > > > down if anybody uses #include directives, and it's also an annoying
> > > > extra step and extra script to maintain.
> > > >
> > > > tl;dr: no, I don't have any good ideas.
> > >
> > > This frankly also gets back to an ongoing discussion I'm having with Pali which is
> > > that having a Kconfig option to set a string used in the environment is well, not a
> > > great way to use the tool.
> > >
> > > What I kind of want, or at least keep leaning towards, is that nothing that sets
> > > the environment directly should be in Kconfig, and things should be pulled from
> > > the text based .env files.
> > >
> > > I don't have a complete idea / solution right now, but I'm not sure entering more
> > > strings in Kconfig, that end up in the environment is moving in the right direction.
> > >
> >
> > Ok. I was not aware that a Kconfig string should not be used in the environment
> > header. If this is the case I agree that we maybe then should not support it.
> >
> > But currently there are several usecases for it e.g.:
> > cmd/Kconfig:config MTDPARTS_DEFAULT
> > this is a string ends up in
> > include/env_default.h:  "mtdparts="     CONFIG_MTDPARTS_DEFAULT         "\0"
> >
> > So all these get obsolete in the future and should be defined in a board_env.txt
> > file?
> >
> > If this is the case I will go into this direction and remove the strings I have for my
> > boards coming from Kconfig and move them into a board_env.txt
>
> What I was trying to say was that yes, we do it in a ton of places
> today, and it's problematic. Pali's example was that nokia_rx51 had both
> preboot= in CONFIG_EXTRA_ENV_SETTINGS and then from how CONFIG_PREBOOT
> is set. I'd gone through and tried to find and fix all of the cases
> where preboot, mtdparts, etc, were both set in CONFIG_EXTRA_ENV_SETTINGS
> and then from env_default.h but missed at least that one case. And
> nothing but manual review will catch new cases of this issue from
> happening in the future.  What I'm not sure of yet is how to solve this
> in a good way going forward.

If you are warning to stop the use of CONFIG strings in the
environment, the current implementation probably helps with that.

We could also grep for CONFIG options and make sure none of them
evaluates to a string?

Regards,
Simon