Re: [PATCH v5 2/3] kbuild: split adjust_autoksyms.sh in two parts

[Quentin Perret]

On Friday 28 Feb 2020 at 01:41:50 (+0900), Masahiro Yamada wrote:
> Hi.
> 
> On Tue, Feb 18, 2020 at 6:41 PM Quentin Perret <qperret@xxxxxxxxxx> wrote:
> >
> > In order to prepare the ground for a build-time optimization, split
> > adjust_autoksyms.sh into two scripts: one that generates autoksyms.h
> > based on all currently available information (whitelist, and .mod
> > files), and the other to inspect the diff between two versions of
> > autoksyms.h and trigger appropriate rebuilds.
> >
> > Acked-by: Nicolas Pitre <nico@xxxxxxxxxxx>
> > Tested-by: Matthias Maennich <maennich@xxxxxxxxxx>
> > Reviewed-by: Matthias Maennich <maennich@xxxxxxxxxx>
> > Signed-off-by: Quentin Perret <qperret@xxxxxxxxxx>
> > ---
> >  scripts/adjust_autoksyms.sh | 36 +++-----------------------
> >  scripts/gen_autoksyms.sh    | 51 +++++++++++++++++++++++++++++++++++++
> >  2 files changed, 55 insertions(+), 32 deletions(-)
> >  create mode 100755 scripts/gen_autoksyms.sh
> >
> > diff --git a/scripts/adjust_autoksyms.sh b/scripts/adjust_autoksyms.sh
> > index ff46996525d3..2b366d945ccb 100755
> > --- a/scripts/adjust_autoksyms.sh
> > +++ b/scripts/adjust_autoksyms.sh
> > @@ -1,14 +1,13 @@
> >  #!/bin/sh
> >  # SPDX-License-Identifier: GPL-2.0-only
> >
> > -# Script to create/update include/generated/autoksyms.h and dependency files
> > +# Script to update include/generated/autoksyms.h and dependency files
> >  #
> >  # Copyright:   (C) 2016  Linaro Limited
> >  # Created by:  Nicolas Pitre, January 2016
> >  #
> >
> > -# Create/update the include/generated/autoksyms.h file from the list
> > -# of all module's needed symbols as recorded on the second line of *.mod files.
> > +# Update the include/generated/autoksyms.h file.
> >  #
> >  # For each symbol being added or removed, the corresponding dependency
> >  # file's timestamp is updated to force a rebuild of the affected source
> > @@ -38,35 +37,8 @@ esac
> >  # We need access to CONFIG_ symbols
> >  . include/config/auto.conf
> >
> > -ksym_wl=/dev/null
> > -if [ -n "$CONFIG_UNUSED_KSYMS_WHITELIST" ]; then
> > -       # Use 'eval' to expand the whitelist path and check if it is relative
> > -       eval ksym_wl="$CONFIG_UNUSED_KSYMS_WHITELIST"
> > -       [ "${ksym_wl}" != "${ksym_wl#/}" ] || ksym_wl="$abs_srctree/$ksym_wl"
> > -       if [ ! -f "$ksym_wl" ]; then
> 
> 
> Just a Nit.
> 
> Maybe, is testing  -r better ?
> 
> 'cat - "$ksym_wl"' is piped, so its error code is not checked.
> 
> So, checking the read permission here is robust, I think.

Right, that's a good point. And actually, I think we want both -f and
-r. -r alone would consider a path to a folder as correct.

This should do the trick:

diff --git a/scripts/gen_autoksyms.sh b/scripts/gen_autoksyms.sh
index 679c9f05e4b4..16c0b2ddaa4c 100755
--- a/scripts/gen_autoksyms.sh
+++ b/scripts/gen_autoksyms.sh
@@ -24,7 +24,7 @@ if [ -n "$CONFIG_UNUSED_KSYMS_WHITELIST" ]; then
        # Use 'eval' to expand the whitelist path and check if it is relative
        eval ksym_wl="$CONFIG_UNUSED_KSYMS_WHITELIST"
        [ "${ksym_wl}" != "${ksym_wl#/}" ] || ksym_wl="$abs_srctree/$ksym_wl"
-       if [ ! -f "$ksym_wl" ]; then
+       if [ ! -f "$ksym_wl" ] || [ ! -r "$ksym_wl" ]; then
                echo "ERROR: '$ksym_wl' whitelist file not found" >&2
                exit 1
        fi

I'll send a v6 shortly.

Thanks!
Quentin