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

[Masahiro Yamada]

On Fri, Feb 28, 2020 at 7:03 PM Quentin Perret <qperret@xxxxxxxxxx> wrote:
>
> 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:


Sounds good.

Thanks.



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



-- 
Best Regards
Masahiro Yamada