All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] env: Allow string CONFIG options in the text environment
@ 2022-11-03 20:46 Simon Glass
  2022-11-04 14:20 ` Holger Brunck
  0 siblings, 1 reply; 9+ messages in thread
From: Simon Glass @ 2022-11-03 20:46 UTC (permalink / raw)
  To: U-Boot Mailing List
  Cc: Holger Brunck, Tom Rini, Simon Glass, Heinrich Schuchardt,
	Joe Hershberger, Marek Behún, Wolfgang Denk

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:

   bootcmd=run CONFIG_BOARD_CMD

becomes

   bootcmd=run "boot_board"

But for the special case where there is a single quoted, it seems
reasonable to suppress the quotes, so that:

   board=CONFIG_SYS_BOARD

becomes

   board=sandbox

Update the script, documentation and tests accordingly.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 doc/usage/environment.rst |  7 ++++++-
 scripts/env2string.awk    |  8 ++++++++
 test/py/tests/test_env.py | 10 +++++++++-
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/doc/usage/environment.rst b/doc/usage/environment.rst
index 15897f63dd9..7272c6abbb4 100644
--- a/doc/usage/environment.rst
+++ b/doc/usage/environment.rst
@@ -55,7 +55,12 @@ variable but assigning to a new one::
 
 This file can include C-style comments. Blank lines and multi-line
 variables are supported, and you can use normal C preprocessor directives
-and CONFIG defines from your board config also.
+and CONFIG defines from your board config also. If the CONFIG value consists of
+a string and this is the only thing in the variable, the quotes will be
+dropped::
+
+   something=CONFIG_SYS_BOARD
+   # where CONFIG_SYS_BOARD is "sandbox" this becomes: something=sandbox
 
 For example, for snapper9260 you would create a text file called
 `board/bluewater/snapper9260.env` containing the environment text.
diff --git a/scripts/env2string.awk b/scripts/env2string.awk
index de470a49941..3a0fac47d07 100644
--- a/scripts/env2string.awk
+++ b/scripts/env2string.awk
@@ -43,6 +43,14 @@ NF {
 		var = substr($0, 1, RLENGTH - 1)
 		env = substr($0, RLENGTH + 1)
 
+		# If the env value consists entirely of a quoted string, drop
+		# the quotes. This handles things like fred=CONFIG_SYS_BOARD
+		# which would otherwise result in fred=\"sandbox\" in this
+		# output and fred="sandbox" in the final environment.
+		if (length(env) != 0 && match(env, /^\\"([^"]*)\\"$/)) {
+			env = substr(env, RSTART + 2, RLENGTH - 4)
+		}
+
 		# Deal with += which concatenates the new string to the existing
 		# variable. Again we are careful to use POSIX match()
 		if (length(env) != 0 && match(var, "^(.*)[+]$")) {
diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py
index 6d08565f0b5..9bec12a2269 100644
--- a/test/py/tests/test_env.py
+++ b/test/py/tests/test_env.py
@@ -588,8 +588,16 @@ e=456
 m+= 456''', 'e=456\\0m=123 456\\0')
 
     # contains quotes
+    check_script('''fred=run "my var"
+mary=another"''', 'fred=run \\"my var\\"\\0mary=another\\"\\0')
+
+    # contains only a quoted strings, so quotes are removed
     check_script('''fred="my var"
-mary=another"''', 'fred=\\"my var\\"\\0mary=another\\"\\0')
+mary=another"''', 'fred=my var\\0mary=another\\"\\0')
+
+    # contains more than one quoted string
+    check_script('''fred="my var" or "this var"
+mary=another"''', 'fred=\\"my var\\" or \\"this var\\"\\0mary=another\\"\\0')
 
     # variable name ending in +
     check_script('''fred\\+=my var
-- 
2.38.1.431.g37b22c650d-goog


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* RE: [PATCH] env: Allow string CONFIG options in the text environment
  2022-11-03 20:46 [PATCH] env: Allow string CONFIG options in the text environment Simon Glass
@ 2022-11-04 14:20 ` Holger Brunck
  2022-11-04 19:08   ` Simon Glass
  0 siblings, 1 reply; 9+ messages in thread
From: Holger Brunck @ 2022-11-04 14:20 UTC (permalink / raw)
  To: Simon Glass, U-Boot Mailing List
  Cc: Tom Rini, Heinrich Schuchardt, Joe Hershberger, Marek Behún,
	Wolfgang Denk

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?

>    bootcmd=run CONFIG_BOARD_CMD
> 
> becomes
> 
>    bootcmd=run "boot_board"
>

just out of curiosity as we are also using similar things in our environment, the
double quotes in this case are not needed or?
 
> But for the special case where there is a single quoted, it seems reasonable to
> suppress the quotes, so that:
> 
>    board=CONFIG_SYS_BOARD
> 
> becomes
> 
>    board=sandbox
> 
> Update the script, documentation and tests accordingly.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---

[..]

Best regards
Holger

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] env: Allow string CONFIG options in the text environment
  2022-11-04 14:20 ` Holger Brunck
@ 2022-11-04 19:08   ` Simon Glass
  2022-11-07 10:12     ` Rasmus Villemoes
  0 siblings, 1 reply; 9+ messages in thread
From: Simon Glass @ 2022-11-04 19:08 UTC (permalink / raw)
  To: Holger Brunck
  Cc: U-Boot Mailing List, Tom Rini, Heinrich Schuchardt,
	Joe Hershberger, Marek Behún, Wolfgang Denk

Hi Holger,

On Fri, 4 Nov 2022 at 08:20, Holger Brunck
<holger.brunck@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?

>
> >    bootcmd=run CONFIG_BOARD_CMD
> >
> > becomes
> >
> >    bootcmd=run "boot_board"
> >
>
> just out of curiosity as we are also using similar things in our environment, the
> double quotes in this case are not needed or?

It isn't needed...actually that is a bad example.

>
> > But for the special case where there is a single quoted, it seems reasonable to
> > suppress the quotes, so that:
> >
> >    board=CONFIG_SYS_BOARD
> >
> > becomes
> >
> >    board=sandbox
> >
> > Update the script, documentation and tests accordingly.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
>
> [..]

Regards,
Simon

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] env: Allow string CONFIG options in the text environment
  2022-11-04 19:08   ` Simon Glass
@ 2022-11-07 10:12     ` Rasmus Villemoes
  2022-11-07 12:54       ` Holger Brunck
  2022-11-07 15:20       ` Tom Rini
  0 siblings, 2 replies; 9+ messages in thread
From: Rasmus Villemoes @ 2022-11-07 10:12 UTC (permalink / raw)
  To: Simon Glass, Holger Brunck
  Cc: U-Boot Mailing List, Tom Rini, Heinrich Schuchardt,
	Joe Hershberger, Marek Behún, Wolfgang Denk

On 04/11/2022 20.08, Simon Glass wrote:
> Hi Holger,
> 
> On Fri, 4 Nov 2022 at 08:20, Holger Brunck
> <holger.brunck@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.

Rasmus



^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] env: Allow string CONFIG options in the text environment
  2022-11-07 10:12     ` Rasmus Villemoes
@ 2022-11-07 12:54       ` Holger Brunck
  2022-11-07 15:20       ` Tom Rini
  1 sibling, 0 replies; 9+ messages in thread
From: Holger Brunck @ 2022-11-07 12:54 UTC (permalink / raw)
  To: Rasmus Villemoes, Simon Glass
  Cc: U-Boot Mailing List, Tom Rini, Heinrich Schuchardt,
	Joe Hershberger, Marek Behún, Wolfgang Denk

> > On Fri, 4 Nov 2022 at 08:20, Holger Brunck
> > <holger.brunck@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.
> 

ok if I I understand it correctly that at the time we process the environment.txt files
the CONFIG_* options are already replaced by some generic process and therefore
at the later stage we cannot remove the double quotes as we don't know if they are
intended or due to a Kconfig string.

In this case the only thing that comes in my mind would be to have an additional
tag we can use in the environment.txt file to explicitly tell the parser to remove them.

Something like:
netdev=__plain_string(CONFIG_KM_NETEDEV)

this we could parse later on and remove the double quotes between the braces.
In this case we know we always have them per default, but users would be able
to remove them if wanted. This would at least be consistent.

Best regards
Holger



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] env: Allow string CONFIG options in the text environment
  2022-11-07 10:12     ` Rasmus Villemoes
  2022-11-07 12:54       ` Holger Brunck
@ 2022-11-07 15:20       ` Tom Rini
  2022-11-07 16:33         ` Holger Brunck
  1 sibling, 1 reply; 9+ messages in thread
From: Tom Rini @ 2022-11-07 15:20 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: Simon Glass, Holger Brunck, U-Boot Mailing List,
	Heinrich Schuchardt, Joe Hershberger, Marek Behún,
	Wolfgang Denk, Pali Rohár

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

On Mon, Nov 07, 2022 at 11:12:48AM +0100, Rasmus Villemoes wrote:
> On 04/11/2022 20.08, Simon Glass wrote:
> > Hi Holger,
> > 
> > On Fri, 4 Nov 2022 at 08:20, Holger Brunck
> > <holger.brunck@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.

-- 
Tom

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] env: Allow string CONFIG options in the text environment
  2022-11-07 15:20       ` Tom Rini
@ 2022-11-07 16:33         ` Holger Brunck
  2022-11-07 17:29           ` Tom Rini
  0 siblings, 1 reply; 9+ messages in thread
From: Holger Brunck @ 2022-11-07 16:33 UTC (permalink / raw)
  To: Tom Rini, Rasmus Villemoes
  Cc: Simon Glass, U-Boot Mailing List, Heinrich Schuchardt,
	Joe Hershberger, Marek Behún, Wolfgang Denk,
	Pali Rohár

Hi Tom,

> > > On Fri, 4 Nov 2022 at 08:20, Holger Brunck
> > > <holger.brunck@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

Best regards
Holger


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] env: Allow string CONFIG options in the text environment
  2022-11-07 16:33         ` Holger Brunck
@ 2022-11-07 17:29           ` Tom Rini
  2022-11-10 20:40             ` Simon Glass
  0 siblings, 1 reply; 9+ messages in thread
From: Tom Rini @ 2022-11-07 17:29 UTC (permalink / raw)
  To: Holger Brunck
  Cc: Rasmus Villemoes, Simon Glass, U-Boot Mailing List,
	Heinrich Schuchardt, Joe Hershberger, Marek Behún,
	Wolfgang Denk, Pali Rohár

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

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

-- 
Tom

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] env: Allow string CONFIG options in the text environment
  2022-11-07 17:29           ` Tom Rini
@ 2022-11-10 20:40             ` Simon Glass
  0 siblings, 0 replies; 9+ messages in thread
From: Simon Glass @ 2022-11-10 20:40 UTC (permalink / raw)
  To: Tom Rini
  Cc: Holger Brunck, Rasmus Villemoes, U-Boot Mailing List,
	Heinrich Schuchardt, Joe Hershberger, Marek Behún,
	Wolfgang Denk, Pali Rohár

Hi Tom,


On Mon, 7 Nov 2022 at 10:29, Tom Rini <trini@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@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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-11-10 20:41 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-03 20:46 [PATCH] env: Allow string CONFIG options in the text environment Simon Glass
2022-11-04 14:20 ` Holger Brunck
2022-11-04 19:08   ` Simon Glass
2022-11-07 10:12     ` Rasmus Villemoes
2022-11-07 12:54       ` Holger Brunck
2022-11-07 15:20       ` Tom Rini
2022-11-07 16:33         ` Holger Brunck
2022-11-07 17:29           ` Tom Rini
2022-11-10 20:40             ` Simon Glass

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.