| Message ID | 20210420144300.103757-1-oleksandr.suvorov@toradex.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [v2] Makefile: fix generating environment file | expand |
On 20/04/2021 16.43, Oleksandr Suvorov wrote: > If the CONFIG_USE_DEFAULT_ENV_FILE=y and CONFIG_DEFAULT_ENV_FILE > points to the empty environment file, the auto-generated file has > the wrong syntax so it leads to the compilation failure: > Glad someone is using CONFIG_USE_DEFAULT_ENV_FILE :) And thanks for reporting this. > > Fix this issue conditionally adding the delimiter ", ". Hm, yeah, that should work. But I wonder if it would make more sense to ensure tr always gets a final newline (which then gets translated to a nul byte, which in turn gives the trailing 0x00). Something like (untested) define filechk_defaultenv.h ( { grep -v '^#' | grep -v '^$$' ; echo '' ; } | \ tr '\n' '\0' | \ sed -e 's/\\\x0\s*//g' | \ xxd -i ; ) endef Rasmus
Hi Rasmus, Thanks for your feedback! Yes, I noted that there were no possible situations with the trailing code != 0x00, but simply removing the additional trailing 0x00 gives us an empty array default_environment[] for the empty defaultenv file. I need to test whether this case is handled in u-boot properly and then prepare the next patch version :P On Tue, Apr 20, 2021 at 10:33 PM Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > > On 20/04/2021 16.43, Oleksandr Suvorov wrote: > > If the CONFIG_USE_DEFAULT_ENV_FILE=y and CONFIG_DEFAULT_ENV_FILE > > points to the empty environment file, the auto-generated file has > > the wrong syntax so it leads to the compilation failure: > > > > Glad someone is using CONFIG_USE_DEFAULT_ENV_FILE :) And thanks for > reporting this. > > > > > Fix this issue conditionally adding the delimiter ", ". > > Hm, yeah, that should work. But I wonder if it would make more sense to > ensure tr always gets a final newline (which then gets translated to a > nul byte, which in turn gives the trailing 0x00). Something like (untested) > > define filechk_defaultenv.h > ( { grep -v '^#' | grep -v '^$$' ; echo '' ; } | \ > tr '\n' '\0' | \ > sed -e 's/\\\x0\s*//g' | \ > xxd -i ; ) > endef > > Rasmus -- Best regards Oleksandr Suvorov Toradex AG Ebenaustrasse 10 | 6048 Horw | Switzerland | T: +41 41 500 48 00
On 20/04/2021 23.10, Oleksandr Suvorov wrote: > Hi Rasmus, > > Thanks for your feedback! > Yes, I noted that there were no possible situations with the trailing > code != 0x00, but simply removing the additional trailing 0x00 > gives us an empty array default_environment[] for the empty defaultenv file. > I need to test whether this case is handled in u-boot properly and > then prepare the next patch version :P No, I'm not suggesting removing the trailing nul byte, it very much has to be there - the binary format of the environment is a sequence of nul-terminated C strings of the key=value form, concatenated back-to-back, terminated by an empty string. What I'm suggesting is to take the input file === foo=bar # Set our IP address ip=1.2.3.4 === do the comment- and empty-line stripping (the two first greps), and then after that add an extra empty line === foo=bar ip=1.2.3.4 === and then feed that to the 'replace \n by nul bytes' | 'delete backslash+nul+whitespace' | xxd pipe. That way there's always that trailing nul on the input to xxd, i.e. in the example above, we would feed foo=bar\0ip-1.2.3.4\0\0 into xxd, while with an initially empty file xxd would just receive that single nul byte. It's just that I think terminating the sequence of key=value lines by an empty line more exactly matches the binary format. Rasmus
Hi Rasmus, On Wed, Apr 21, 2021 at 12:34 AM Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > > On 20/04/2021 23.10, Oleksandr Suvorov wrote: > > Hi Rasmus, > > > > Thanks for your feedback! > > Yes, I noted that there were no possible situations with the trailing > > code != 0x00, but simply removing the additional trailing 0x00 > > gives us an empty array default_environment[] for the empty defaultenv file. > > I need to test whether this case is handled in u-boot properly and > > then prepare the next patch version :P > > No, I'm not suggesting removing the trailing nul byte, it very much has > to be there - the binary format of the environment is a sequence of > nul-terminated C strings of the key=value form, concatenated > back-to-back, terminated by an empty string. (/me saying: never answer at night, never answer at night, never answer at night :-D) > > What I'm suggesting is to take the input file > > === > foo=bar > > # Set our IP address > ip=1.2.3.4 > === > > do the comment- and empty-line stripping (the two first greps), and then > after that add an extra empty line > > === > foo=bar > ip=1.2.3.4 > > === > > and then feed that to the 'replace \n by nul bytes' | 'delete > backslash+nul+whitespace' | xxd pipe. That way there's always that > trailing nul on the input to xxd, i.e. in the example above, we would > feed foo=bar\0ip-1.2.3.4\0\0 into xxd, while with an initially empty > file xxd would just receive that single nul byte. > > It's just that I think terminating the sequence of key=value lines by an > empty line more exactly matches the binary format. Sure, now I see. Your solution is more straight and clear. Unfortunately, it doesn't work :) So if you don't mind, I'll try to make it work as it should and post the next version of the patch. > > Rasmus
On 21/04/2021 17.21, Oleksandr Suvorov wrote: > Hi Rasmus, > > On Wed, Apr 21, 2021 at 12:34 AM Rasmus Villemoes > <rasmus.villemoes@prevas.dk> wrote: >> >> On 20/04/2021 23.10, Oleksandr Suvorov wrote: >>> Hi Rasmus, >>> >>> Thanks for your feedback! >>> Yes, I noted that there were no possible situations with the trailing >>> code != 0x00, but simply removing the additional trailing 0x00 >>> gives us an empty array default_environment[] for the empty defaultenv file. >>> I need to test whether this case is handled in u-boot properly and >>> then prepare the next patch version :P >> >> No, I'm not suggesting removing the trailing nul byte, it very much has >> to be there - the binary format of the environment is a sequence of >> nul-terminated C strings of the key=value form, concatenated >> back-to-back, terminated by an empty string. > > (/me saying: never answer at night, never answer at night, never > answer at night :-D) > >> >> What I'm suggesting is to take the input file >> >> === >> foo=bar >> >> # Set our IP address >> ip=1.2.3.4 >> === >> >> do the comment- and empty-line stripping (the two first greps), and then >> after that add an extra empty line >> >> === >> foo=bar >> ip=1.2.3.4 >> >> === >> >> and then feed that to the 'replace \n by nul bytes' | 'delete >> backslash+nul+whitespace' | xxd pipe. That way there's always that >> trailing nul on the input to xxd, i.e. in the example above, we would >> feed foo=bar\0ip-1.2.3.4\0\0 into xxd, while with an initially empty >> file xxd would just receive that single nul byte. >> >> It's just that I think terminating the sequence of key=value lines by an >> empty line more exactly matches the binary format. > > Sure, now I see. Your solution is more straight and clear. > Unfortunately, it doesn't work :) Yeah, I didn't really expect it to. Ah, it's because "set -e" is in effect, so in ( { grep -v '^#' | grep -v '^$$' ; echo '' ; } | \ the return value of the grep -v '^#' | grep -v '^$$' pipeline is that of the second grep, and when there's no input lines that match (such as, with an empty input file), that's an EXIT_FAILURE. So the whole subshell exits at that point, and nothing gets written to defaultenv_autogenerated.h. Doing define filechk_defaultenv.h ( { grep -v '^#' | grep -v '^$$' || true ; echo '' ; } | \ tr '\n' '\0' | \ sed -e 's/\\\x0\s*//g' | \ xxd -i ; ) endef seems to work. Rasmus
On Wed, Apr 21, 2021 at 11:56 PM Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > > On 21/04/2021 17.21, Oleksandr Suvorov wrote: > > Hi Rasmus, > > > > On Wed, Apr 21, 2021 at 12:34 AM Rasmus Villemoes > > <rasmus.villemoes@prevas.dk> wrote: > >> > >> On 20/04/2021 23.10, Oleksandr Suvorov wrote: > >>> Hi Rasmus, > >>> > >>> Thanks for your feedback! > >>> Yes, I noted that there were no possible situations with the trailing > >>> code != 0x00, but simply removing the additional trailing 0x00 > >>> gives us an empty array default_environment[] for the empty defaultenv file. > >>> I need to test whether this case is handled in u-boot properly and > >>> then prepare the next patch version :P > >> > >> No, I'm not suggesting removing the trailing nul byte, it very much has > >> to be there - the binary format of the environment is a sequence of > >> nul-terminated C strings of the key=value form, concatenated > >> back-to-back, terminated by an empty string. > > > > (/me saying: never answer at night, never answer at night, never > > answer at night :-D) > > > >> > >> What I'm suggesting is to take the input file > >> > >> === > >> foo=bar > >> > >> # Set our IP address > >> ip=1.2.3.4 > >> === > >> > >> do the comment- and empty-line stripping (the two first greps), and then > >> after that add an extra empty line > >> > >> === > >> foo=bar > >> ip=1.2.3.4 > >> > >> === > >> > >> and then feed that to the 'replace \n by nul bytes' | 'delete > >> backslash+nul+whitespace' | xxd pipe. That way there's always that > >> trailing nul on the input to xxd, i.e. in the example above, we would > >> feed foo=bar\0ip-1.2.3.4\0\0 into xxd, while with an initially empty > >> file xxd would just receive that single nul byte. > >> > >> It's just that I think terminating the sequence of key=value lines by an > >> empty line more exactly matches the binary format. > > > > Sure, now I see. Your solution is more straight and clear. > > Unfortunately, it doesn't work :) > > Yeah, I didn't really expect it to. Ah, it's because "set -e" is in > effect, so in > > ( { grep -v '^#' | grep -v '^$$' ; echo '' ; } | \ > > the return value of the grep -v '^#' | grep -v '^$$' pipeline is that > of the second grep, and when there's no input lines that match (such as, > with an empty input file), that's an EXIT_FAILURE. So the whole subshell > exits at that point, and nothing gets written to defaultenv_autogenerated.h. > > Doing > > define filechk_defaultenv.h > ( { grep -v '^#' | grep -v '^$$' || true ; echo '' ; } | \ > tr '\n' '\0' | \ > sed -e 's/\\\x0\s*//g' | \ > xxd -i ; ) > endef > > seems to work. So will you post your own patch? > Rasmus
diff --git a/Makefile b/Makefile index e423f6de746..4e0a9b51cb7 100644 --- a/Makefile +++ b/Makefile @@ -1853,7 +1853,9 @@ define filechk_defaultenv.h grep -v '^$$' | \ tr '\n' '\0' | \ sed -e 's/\\\x0\s*//g' | \ - xxd -i ; echo ", 0x00" ; ) + xxd -i | \ + sed -r 's/([0-9a-f])$$/\1, /'; \ + echo "0x00" ; ) endef define filechk_dt.h
If the CONFIG_USE_DEFAULT_ENV_FILE=y and CONFIG_DEFAULT_ENV_FILE points to the empty environment file, the auto-generated file has the wrong syntax so it leads to the compilation failure: In file included from include/env_default.h:115, from env/common.c:29: include/generated/defaultenv_autogenerated.h:1:1: error: expected expression before ‘,’ token 1 | , 0x00 | ^ make[1]: *** [scripts/Makefile.build:266: env/common.o] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1744: env] Error 2 Fix this issue conditionally adding the delimiter ", ". Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> --- Changes in v2: - Fix the hex-decimal class matching. Makefile | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)