[PATCH v2] Makefile: fix generating environment file

[PATCH v2] Makefile: fix generating environment file

[Oleksandr Suvorov]

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(-)

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
-- 
2.31.1

[PATCH v2] Makefile: fix generating environment file

[Rasmus Villemoes]

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

[PATCH v2] Makefile: fix generating environment file

[Oleksandr Suvorov]

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

[PATCH v2] Makefile: fix generating environment file

[Rasmus Villemoes]

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

[PATCH v2] Makefile: fix generating environment file

[Oleksandr Suvorov]

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



-- 
Best regards
Oleksandr Suvorov

Toradex AG
Ebenaustrasse 10 | 6048 Horw | Switzerland | T: +41 41 500 48 00

[PATCH v2] Makefile: fix generating environment file

[Rasmus Villemoes]

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

[PATCH v2] Makefile: fix generating environment file

[Oleksandr Suvorov]

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


-- 
Best regards
Oleksandr Suvorov

Toradex AG
Ebenaustrasse 10 | 6048 Horw | Switzerland | T: +41 41 500 48 00

[PATCH] Makefile: fix generation of defaultenv.h from empty initial file

[Rasmus Villemoes]

When CONFIG_USE_DEFAULT_ENV_FILE=y and the file
CONFIG_DEFAULT_ENV_FILE is empty (or at least doesn't contain any
non-comment, non-empty lines), we end up feeding nothing into xxd,
which in turn then outputs nothing. Then blindly appending ", 0x00"
means that we end up trying to compile (roughly)

const char defaultenv[] = { , 0x00 }

which is of course broken.

To fix that, change the frobbing of the text file so that we always
end up printing an extra empty line (which gets turned into that extra
nul byte we need) - that corresponds better to the binary format
consisting of a series of key=val nul terminated strings, terminated
by an empty string.

Reported-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 Makefile | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 3fc9777b0b..b7af2b936d 100644
--- a/Makefile
+++ b/Makefile
@@ -1854,11 +1854,10 @@ define filechk_timestamp.h
 endef
 
 define filechk_defaultenv.h
-	(grep -v '^#' | \
-	 grep -v '^$$' | \
+	( { grep -v '^#' | grep -v '^$$' || true ; echo '' ; } | \
 	 tr '\n' '\0' | \
 	 sed -e 's/\\\x0\s*//g' | \
-	 xxd -i ; echo ", 0x00" ; )
+	 xxd -i ; )
 endef
 
 define filechk_dt.h
-- 
2.29.2

[PATCH] Makefile: fix generation of defaultenv.h from empty initial file

[Oleksandr Suvorov]

Hi Rasmus,

Thanks for the patch, I've tested it.

On Thu, Apr 22, 2021 at 10:44 AM Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> When CONFIG_USE_DEFAULT_ENV_FILE=y and the file
> CONFIG_DEFAULT_ENV_FILE is empty (or at least doesn't contain any
> non-comment, non-empty lines), we end up feeding nothing into xxd,
> which in turn then outputs nothing. Then blindly appending ", 0x00"
> means that we end up trying to compile (roughly)
>
> const char defaultenv[] = { , 0x00 }
>
> which is of course broken.
>
> To fix that, change the frobbing of the text file so that we always
> end up printing an extra empty line (which gets turned into that extra
> nul byte we need) - that corresponds better to the binary format
> consisting of a series of key=val nul terminated strings, terminated
> by an empty string.
>
> Reported-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Reviewed-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>

> ---
>  Makefile | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 3fc9777b0b..b7af2b936d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1854,11 +1854,10 @@ define filechk_timestamp.h
>  endef
>
>  define filechk_defaultenv.h
> -       (grep -v '^#' | \
> -        grep -v '^$$' | \
> +       ( { grep -v '^#' | grep -v '^$$' || true ; echo '' ; } | \
>          tr '\n' '\0' | \
>          sed -e 's/\\\x0\s*//g' | \
> -        xxd -i ; echo ", 0x00" ; )
> +        xxd -i ; )
>  endef
>
>  define filechk_dt.h
> --
> 2.29.2
>

-- 
Best regards
Oleksandr Suvorov

Toradex AG
Ebenaustrasse 10 | 6048 Horw | Switzerland | T: +41 41 500 48 00

[PATCH] Makefile: fix generation of defaultenv.h from empty initial file

[Tom Rini]

On Thu, Apr 22, 2021 at 09:44:18AM +0200, Rasmus Villemoes wrote:

> When CONFIG_USE_DEFAULT_ENV_FILE=y and the file
> CONFIG_DEFAULT_ENV_FILE is empty (or at least doesn't contain any
> non-comment, non-empty lines), we end up feeding nothing into xxd,
> which in turn then outputs nothing. Then blindly appending ", 0x00"
> means that we end up trying to compile (roughly)
> 
> const char defaultenv[] = { , 0x00 }
> 
> which is of course broken.
> 
> To fix that, change the frobbing of the text file so that we always
> end up printing an extra empty line (which gets turned into that extra
> nul byte we need) - that corresponds better to the binary format
> consisting of a series of key=val nul terminated strings, terminated
> by an empty string.
> 
> Reported-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Reviewed-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210427/6a9cafe4/attachment.sig>