[PATCH] env: Allow string CONFIG options in the text environment
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
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
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
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
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
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.
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
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.
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
participants (4)
-
Holger Brunck -
Rasmus Villemoes -
Simon Glass -
Tom Rini