diff mbox series

kheaders: explicitly validate existence of cpio command

Message ID 20230117-kernel-kheaders-cpio-v1-1-058d3e1c8621@weissschuh.net (mailing list archive)
State New, archived
Headers show
Series kheaders: explicitly validate existence of cpio command | expand

Commit Message

Thomas Weißschuh Jan. 17, 2023, 5:30 a.m. UTC
If the cpio command is not available the error emitted by
gen_kheaders.so is not clear as all output of the call to cpio is
discarded:

GNU make 4.4:

  GEN     kernel/kheaders_data.tar.xz
find: 'standard output': Broken pipe
find: write error
make[2]: *** [kernel/Makefile:157: kernel/kheaders_data.tar.xz] Error 127
make[1]: *** [scripts/Makefile.build:504: kernel] Error 2

GNU make < 4.4:

  GEN     kernel/kheaders_data.tar.xz
make[2]: *** [kernel/Makefile:157: kernel/kheaders_data.tar.xz] Error 127
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [scripts/Makefile.build:504: kernel] Error 2

Add an explicit check that will trigger a clear message about the issue:

  CHK     kernel/kheaders_data.tar.xz
./kernel/gen_kheaders.sh: line 17: type: cpio: not found

The other commands executed by gen_kheaders.sh are part of a standard
installation, so they are not checked.

Reported-by: Amy Parker <apark0006@student.cerritos.edu>
Link: https://lore.kernel.org/lkml/CAPOgqxFva=tOuh1UitCSN38+28q3BNXKq19rEsVNPRzRqKqZ+g@mail.gmail.com/
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 kernel/gen_kheaders.sh | 2 ++
 1 file changed, 2 insertions(+)


---
base-commit: d532dd102151cc69fcd00b13e5a9689b23c0c8d9
change-id: 20230117-kernel-kheaders-cpio-2f7c70bc9c19

Best regards,

Comments

Nicolas Schier Jan. 17, 2023, 8:46 a.m. UTC | #1
On Tue, Jan 17, 2023 at 05:30:43AM +0000, Thomas Weißschuh wrote:
> If the cpio command is not available the error emitted by
> gen_kheaders.so is not clear as all output of the call to cpio is
> discarded:
> 
> GNU make 4.4:
> 
>   GEN     kernel/kheaders_data.tar.xz
> find: 'standard output': Broken pipe
> find: write error
> make[2]: *** [kernel/Makefile:157: kernel/kheaders_data.tar.xz] Error 127
> make[1]: *** [scripts/Makefile.build:504: kernel] Error 2
> 
> GNU make < 4.4:
> 
>   GEN     kernel/kheaders_data.tar.xz
> make[2]: *** [kernel/Makefile:157: kernel/kheaders_data.tar.xz] Error 127
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [scripts/Makefile.build:504: kernel] Error 2
> 
> Add an explicit check that will trigger a clear message about the issue:
> 
>   CHK     kernel/kheaders_data.tar.xz
> ./kernel/gen_kheaders.sh: line 17: type: cpio: not found

Thanks for the patch!  What would you think about an even more verbose
message?  Perhaps something like:

    echo >&2 ' *** "cpio" is required for "CONFIG_IKHEADERS". >&2

Reviewed-by: Nicolas Schier <n.schier@avm.de>

Kind regards,
Nicolas
Thomas Weißschuh Jan. 17, 2023, 1:19 p.m. UTC | #2
On Tue, Jan 17, 2023 at 09:46:02AM +0100, Nicolas Schier wrote:
> On Tue, Jan 17, 2023 at 05:30:43AM +0000, Thomas Weißschuh wrote:
> > If the cpio command is not available the error emitted by
> > gen_kheaders.so is not clear as all output of the call to cpio is
> > discarded:
> > 
> > GNU make 4.4:
> > 
> >   GEN     kernel/kheaders_data.tar.xz
> > find: 'standard output': Broken pipe
> > find: write error
> > make[2]: *** [kernel/Makefile:157: kernel/kheaders_data.tar.xz] Error 127
> > make[1]: *** [scripts/Makefile.build:504: kernel] Error 2
> > 
> > GNU make < 4.4:
> > 
> >   GEN     kernel/kheaders_data.tar.xz
> > make[2]: *** [kernel/Makefile:157: kernel/kheaders_data.tar.xz] Error 127
> > make[2]: *** Waiting for unfinished jobs....
> > make[1]: *** [scripts/Makefile.build:504: kernel] Error 2
> > 
> > Add an explicit check that will trigger a clear message about the issue:
> > 
> >   CHK     kernel/kheaders_data.tar.xz
> > ./kernel/gen_kheaders.sh: line 17: type: cpio: not found
> 
> Thanks for the patch!  What would you think about an even more verbose
> message?  Perhaps something like:
> 
>     echo >&2 ' *** "cpio" is required for "CONFIG_IKHEADERS". >&2

Wouldn't hurt obviously.
The other scripts used by the kernel don't seem to try to provide this
kind of explicit message either, though.
Having a line number, some sort of "not found" and the name of the
command are enough, in my opinion.

> Reviewed-by: Nicolas Schier <n.schier@avm.de>

Thanks!

Thomas
Masahiro Yamada Jan. 17, 2023, 2:33 p.m. UTC | #3
On Tue, Jan 17, 2023 at 10:19 PM Thomas Weißschuh <thomas@t-8ch.de> wrote:
>
> On Tue, Jan 17, 2023 at 09:46:02AM +0100, Nicolas Schier wrote:
> > On Tue, Jan 17, 2023 at 05:30:43AM +0000, Thomas Weißschuh wrote:
> > > If the cpio command is not available the error emitted by
> > > gen_kheaders.so is not clear as all output of the call to cpio is
> > > discarded:
> > >
> > > GNU make 4.4:
> > >
> > >   GEN     kernel/kheaders_data.tar.xz
> > > find: 'standard output': Broken pipe
> > > find: write error
> > > make[2]: *** [kernel/Makefile:157: kernel/kheaders_data.tar.xz] Error 127
> > > make[1]: *** [scripts/Makefile.build:504: kernel] Error 2
> > >
> > > GNU make < 4.4:
> > >
> > >   GEN     kernel/kheaders_data.tar.xz
> > > make[2]: *** [kernel/Makefile:157: kernel/kheaders_data.tar.xz] Error 127
> > > make[2]: *** Waiting for unfinished jobs....
> > > make[1]: *** [scripts/Makefile.build:504: kernel] Error 2
> > >
> > > Add an explicit check that will trigger a clear message about the issue:
> > >
> > >   CHK     kernel/kheaders_data.tar.xz
> > > ./kernel/gen_kheaders.sh: line 17: type: cpio: not found
> >
> > Thanks for the patch!  What would you think about an even more verbose
> > message?  Perhaps something like:
> >
> >     echo >&2 ' *** "cpio" is required for "CONFIG_IKHEADERS". >&2
>
> Wouldn't hurt obviously.
> The other scripts used by the kernel don't seem to try to provide this
> kind of explicit message either, though.
> Having a line number, some sort of "not found" and the name of the
> command are enough, in my opinion.



Agree.  I am fine with this patch.


Just a side note.


'type' is defined in posix [1], so this code is portable.

Kbuild often uses "command -v" to check the existence of a command
because it is also posix compliant [2].


I just wondered why we did not use 'type' before...

'type' is more efficient because it is shell-builtin
(at least for dash and bash) but 'command' is not.



[1]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/type.html

[2]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/command.html







>
> > Reviewed-by: Nicolas Schier <n.schier@avm.de>
>
> Thanks!
>
> Thomas
Masahiro Yamada Jan. 18, 2023, 7:16 a.m. UTC | #4
On Tue, Jan 17, 2023 at 10:19 PM Thomas Weißschuh <thomas@t-8ch.de> wrote:
>
> On Tue, Jan 17, 2023 at 09:46:02AM +0100, Nicolas Schier wrote:
> > On Tue, Jan 17, 2023 at 05:30:43AM +0000, Thomas Weißschuh wrote:
> > > If the cpio command is not available the error emitted by
> > > gen_kheaders.so is not clear as all output of the call to cpio is
> > > discarded:
> > >
> > > GNU make 4.4:
> > >
> > >   GEN     kernel/kheaders_data.tar.xz
> > > find: 'standard output': Broken pipe
> > > find: write error
> > > make[2]: *** [kernel/Makefile:157: kernel/kheaders_data.tar.xz] Error 127
> > > make[1]: *** [scripts/Makefile.build:504: kernel] Error 2
> > >
> > > GNU make < 4.4:
> > >
> > >   GEN     kernel/kheaders_data.tar.xz
> > > make[2]: *** [kernel/Makefile:157: kernel/kheaders_data.tar.xz] Error 127
> > > make[2]: *** Waiting for unfinished jobs....
> > > make[1]: *** [scripts/Makefile.build:504: kernel] Error 2
> > >
> > > Add an explicit check that will trigger a clear message about the issue:
> > >
> > >   CHK     kernel/kheaders_data.tar.xz
> > > ./kernel/gen_kheaders.sh: line 17: type: cpio: not found
> >
> > Thanks for the patch!  What would you think about an even more verbose
> > message?  Perhaps something like:
> >
> >     echo >&2 ' *** "cpio" is required for "CONFIG_IKHEADERS". >&2
>
> Wouldn't hurt obviously.
> The other scripts used by the kernel don't seem to try to provide this
> kind of explicit message either, though.
> Having a line number, some sort of "not found" and the name of the
> command are enough, in my opinion.
>
> > Reviewed-by: Nicolas Schier <n.schier@avm.de>
>
> Thanks!
>
> Thomas


Applied to linux-kbuild. Thanks.
diff mbox series

Patch

diff --git a/kernel/gen_kheaders.sh b/kernel/gen_kheaders.sh
index 473036b43c83..81b97f0f6556 100755
--- a/kernel/gen_kheaders.sh
+++ b/kernel/gen_kheaders.sh
@@ -14,6 +14,8 @@  include/
 arch/$SRCARCH/include/
 "
 
+type cpio > /dev/null
+
 # Support incremental builds by skipping archive generation
 # if timestamps of files being archived are not changed.