| Message ID | 20240728125527.690726-1-ojeda@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kbuild: pahole-version: avoid errors if executing fails | expand |
On Sun, Jul 28, 2024 at 02:55:27PM +0200, Miguel Ojeda wrote: > Like patch "rust: suppress error messages from > CONFIG_{RUSTC,BINDGEN}_VERSION_TEXT" [1], do not assume the file existing > and being executable implies executing it will succeed. Instead, bail > out if executing it fails for any reason. > > For instance, `pahole` may be built for another architecture, may be a > program we do not expect or may be completely broken: > > $ echo 'bad' > bad-pahole > $ chmod u+x bad-pahole > $ make PAHOLE=./bad-pahole defconfig > ... > ./bad-pahole: 1: bad: not found > init/Kconfig:112: syntax error > init/Kconfig:112: invalid statement > > Link: https://lore.kernel.org/rust-for-linux/20240727140302.1806011-1-masahiroy@kernel.org/ [1] > Signed-off-by: Miguel Ojeda <ojeda@kernel.org> > --- > scripts/pahole-version.sh | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/scripts/pahole-version.sh b/scripts/pahole-version.sh > index f8a32ab93ad1..a35b557f1901 100755 > --- a/scripts/pahole-version.sh > +++ b/scripts/pahole-version.sh > @@ -5,9 +5,9 @@ > # > # Prints pahole's version in a 3-digit form, such as 119 for v1.19. > > -if [ ! -x "$(command -v "$@")" ]; then > +if output=$("$@" --version 2>/dev/null); then > + echo "$output" | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/' > +else > echo 0 > exit 1 > fi > - > -"$@" --version | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/' > > base-commit: 256abd8e550ce977b728be79a74e1729438b4948 > -- > 2.45.2 > thanks, looks good to me. Reviewed-by: Nicolas Schier <nicolas@fjasle.eu>
On Sun, Jul 28, 2024 at 9:55 PM Miguel Ojeda <ojeda@kernel.org> wrote: > > Like patch "rust: suppress error messages from > CONFIG_{RUSTC,BINDGEN}_VERSION_TEXT" [1], do not assume the file existing > and being executable implies executing it will succeed. Instead, bail > out if executing it fails for any reason. > > For instance, `pahole` may be built for another architecture, may be a > program we do not expect or may be completely broken: > > $ echo 'bad' > bad-pahole > $ chmod u+x bad-pahole > $ make PAHOLE=./bad-pahole defconfig > ... > ./bad-pahole: 1: bad: not found > init/Kconfig:112: syntax error > init/Kconfig:112: invalid statement Even with this patch applied, a syntax error can happen. $ git log --oneline -1 dd1c54d77f11 kbuild: pahole-version: avoid errors if executing fails $ echo 'echo' > bad-pahole $ chmod u+x bad-pahole $ make PAHOLE=./bad-pahole defconfig *** Default configuration is based on 'x86_64_defconfig' init/Kconfig:114: syntax error init/Kconfig:114: invalid statement make[2]: *** [scripts/kconfig/Makefile:95: defconfig] Error 1 make[1]: *** [/home/masahiro/workspace/linux-kbuild/Makefile:680: defconfig] Error 2 make: *** [Makefile:224: __sub-make] Error 2 > Link: https://lore.kernel.org/rust-for-linux/20240727140302.1806011-1-masahiroy@kernel.org/ [1] > Signed-off-by: Miguel Ojeda <ojeda@kernel.org> > --- > scripts/pahole-version.sh | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/scripts/pahole-version.sh b/scripts/pahole-version.sh > index f8a32ab93ad1..a35b557f1901 100755 > --- a/scripts/pahole-version.sh > +++ b/scripts/pahole-version.sh > @@ -5,9 +5,9 @@ > # > # Prints pahole's version in a 3-digit form, such as 119 for v1.19. > > -if [ ! -x "$(command -v "$@")" ]; then > +if output=$("$@" --version 2>/dev/null); then > + echo "$output" | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/' > +else > echo 0 > exit 1 > fi > - > -"$@" --version | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/' > > base-commit: 256abd8e550ce977b728be79a74e1729438b4948 > -- > 2.45.2 > -- Best Regards Masahiro Yamada
On Fri 23 Aug 2024 02:28:28 GMT, Masahiro Yamada wrote: > Date: Fri, 23 Aug 2024 02:28:28 +0900 > From: Masahiro Yamada <masahiroy@kernel.org> > To: Miguel Ojeda <ojeda@kernel.org> > Cc: Alexei Starovoitov <ast@kernel.org>, Daniel Borkmann > <daniel@iogearbox.net>, Andrii Nakryiko <andrii@kernel.org>, Martin KaFai > Lau <martin.lau@linux.dev>, Eduard Zingerman <eddyz87@gmail.com>, Song Liu > <song@kernel.org>, Yonghong Song <yonghong.song@linux.dev>, John Fastabend > <john.fastabend@gmail.com>, KP Singh <kpsingh@kernel.org>, Stanislav > Fomichev <sdf@fomichev.me>, Hao Luo <haoluo@google.com>, Jiri Olsa > <jolsa@kernel.org>, bpf@vger.kernel.org, Nathan Chancellor > <nathan@kernel.org>, Nicolas Schier <nicolas@fjasle.eu>, > linux-kbuild@vger.kernel.org, linux-kernel@vger.kernel.org, > patches@lists.linux.dev > Subject: Re: [PATCH] kbuild: pahole-version: avoid errors if executing fails > X-Mailing-List: linux-kbuild@vger.kernel.org > Message-ID: <CAK7LNARhR=GGZ2Vr-SSog1yjnjh6iT7cCEe4mpYg889GhJnO9g@mail.gmail.com> > > On Sun, Jul 28, 2024 at 9:55 PM Miguel Ojeda <ojeda@kernel.org> wrote: > > > > Like patch "rust: suppress error messages from > > CONFIG_{RUSTC,BINDGEN}_VERSION_TEXT" [1], do not assume the file existing > > and being executable implies executing it will succeed. Instead, bail > > out if executing it fails for any reason. > > > > For instance, `pahole` may be built for another architecture, may be a > > program we do not expect or may be completely broken: > > > > $ echo 'bad' > bad-pahole > > $ chmod u+x bad-pahole > > $ make PAHOLE=./bad-pahole defconfig > > ... > > ./bad-pahole: 1: bad: not found > > init/Kconfig:112: syntax error > > init/Kconfig:112: invalid statement > > > > Even with this patch applied, a syntax error can happen. > > $ git log --oneline -1 > dd1c54d77f11 kbuild: pahole-version: avoid errors if executing fails > $ echo 'echo' > bad-pahole > $ chmod u+x bad-pahole > $ make PAHOLE=./bad-pahole defconfig > *** Default configuration is based on 'x86_64_defconfig' > init/Kconfig:114: syntax error > init/Kconfig:114: invalid statement > make[2]: *** [scripts/kconfig/Makefile:95: defconfig] Error 1 > make[1]: *** [/home/masahiro/workspace/linux-kbuild/Makefile:680: > defconfig] Error 2 > make: *** [Makefile:224: __sub-make] Error 2 > Do we have to catch all possibilities? Then, what about this: #!/bin/sh trap "echo 0; exit 1" EXIT set -e output=$("$@" --version 2>/dev/null) output=$(echo "${output}" | sed -nE 's/^v([0-9]+)\.([0-9][0-9])$/\1\2/p') if [ -z "${output}" ]; then echo "warning: pahole binary '$1' outputs incompatible version number, pahole will not be used." >&2 exit 1 fi echo "${output}" trap EXIT Kind regards, Nicolas
On Fri, Aug 23, 2024 at 4:00 PM Nicolas Schier <nicolas@fjasle.eu> wrote: > > Do we have to catch all possibilities? Then, what about this: Something like that sounds good to me too -- we do something similar in `rust_is_available.sh`. We also have a `1` in the beginning of (most of) the `sed` commands there to check only the first line. I guess it depends on whether Masahiro thinks the extra checks/complexity is worth it. Here I was aiming to catch the case he reported, i.e. non-successful programs. Cheers, Miguel
On Sat, Aug 24, 2024 at 3:48 AM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > On Fri, Aug 23, 2024 at 4:00 PM Nicolas Schier <nicolas@fjasle.eu> wrote: > > > > Do we have to catch all possibilities? Then, what about this: > > Something like that sounds good to me too -- we do something similar > in `rust_is_available.sh`. We also have a `1` in the beginning of > (most of) the `sed` commands there to check only the first line. > > I guess it depends on whether Masahiro thinks the extra > checks/complexity is worth it. Here I was aiming to catch the case he > reported, i.e. non-successful programs. My previous report was slightly different. CONFIG_{RUSTC,BINDGEN}_VERSION_TEXT are string type symbols. The shell command is allowed to return any string, including an empty string, as long as the value is enclosed by double quotes. In this case, CONFIG_PAHOLE_VERSION is an int type symbol, hence the shell command must not return an empty string. Ensuring this should be easy. Why don't we fix it properly while we are here? > > Cheers, > Miguel > -- Best Regards Masahiro Yamada
On Mon, Sep 2, 2024 at 4:15 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > Ensuring this should be easy. > Why don't we fix it properly while we are here? That is great, I would prefer that. Sent v2: https://lore.kernel.org/all/20240902160828.1092891-1-ojeda@kernel.org/ Cheers, Miguel
diff --git a/scripts/pahole-version.sh b/scripts/pahole-version.sh index f8a32ab93ad1..a35b557f1901 100755 --- a/scripts/pahole-version.sh +++ b/scripts/pahole-version.sh @@ -5,9 +5,9 @@ # # Prints pahole's version in a 3-digit form, such as 119 for v1.19. -if [ ! -x "$(command -v "$@")" ]; then +if output=$("$@" --version 2>/dev/null); then + echo "$output" | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/' +else echo 0 exit 1 fi - -"$@" --version | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/'
Like patch "rust: suppress error messages from CONFIG_{RUSTC,BINDGEN}_VERSION_TEXT" [1], do not assume the file existing and being executable implies executing it will succeed. Instead, bail out if executing it fails for any reason. For instance, `pahole` may be built for another architecture, may be a program we do not expect or may be completely broken: $ echo 'bad' > bad-pahole $ chmod u+x bad-pahole $ make PAHOLE=./bad-pahole defconfig ... ./bad-pahole: 1: bad: not found init/Kconfig:112: syntax error init/Kconfig:112: invalid statement Link: https://lore.kernel.org/rust-for-linux/20240727140302.1806011-1-masahiroy@kernel.org/ [1] Signed-off-by: Miguel Ojeda <ojeda@kernel.org> --- scripts/pahole-version.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) base-commit: 256abd8e550ce977b728be79a74e1729438b4948