Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230826071359.2060501-1-senozhatsky@chromium.org>
In-Reply-To: <20230826071359.2060501-1-senozhatsky@chromium.org>
From:   Masahiro Yamada <masahiroy@kernel.org>
Date:   Tue, 29 Aug 2023 22:13:42 +0900
Message-ID: <CAK7LNAROnZpZiOC4eS5kTcv4Q2YDrE9KYBD-dVcfXwBPQWvbmg@mail.gmail.com>
Subject: Re: [PATCH] kconfig: add warn-unknown-symbols sanity check
To:     Sergey Senozhatsky <senozhatsky@chromium.org>
Cc:     Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Nicolas Schier <nicolas@fjasle.eu>,
        Jonathan Corbet <corbet@lwn.net>,
        Tomasz Figa <tfiga@chromium.org>, linux-kbuild@vger.kernel.org,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sun, Aug 27, 2023 at 6:00=E2=80=AFPM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> Introduce KCONFIG_WARN_UNKNOWN_SYMBOLS environment variable,
> which makes Kconfig warn about unknown .config symbols.
>
> This is especially useful for continuous kernel uprevs when
> some symbols can be either removed or renamed between kernel
> releases (which can go unnoticed otherwise).
>
> By default KCONFIG_WARN_UNKNOWN_SYMBOLS generates warnings,
> which are non-terminal. There is an additional environment
> variable KCONFIG_WERROR that overrides this behaviour and
> turns warnings into errors.
>
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  Documentation/kbuild/kconfig.rst | 11 +++++++++++
>  scripts/kconfig/confdata.c       | 23 +++++++++++++++++++++++
>  2 files changed, 34 insertions(+)
>
> diff --git a/Documentation/kbuild/kconfig.rst b/Documentation/kbuild/kcon=
fig.rst
> index 6530ecd99da3..4de1f5435b7b 100644
> --- a/Documentation/kbuild/kconfig.rst
> +++ b/Documentation/kbuild/kconfig.rst
> @@ -56,6 +56,17 @@ KCONFIG_OVERWRITECONFIG
>  If you set KCONFIG_OVERWRITECONFIG in the environment, Kconfig will not
>  break symlinks when .config is a symlink to somewhere else.
>
> +KCONFIG_WARN_UNKNOWN_SYMBOLS
> +----------------------------
> +This environment variable makes Kconfig warn about all unrecognized
> +symbols in the .config file.


This warns not only for the .config but also defconfig files.

Could you reword it?

For example,

 "symbols in the config input".


> +
> +KCONFIG_WERROR
> +--------------
> +If set, Kconfig will treat `KCONFIG_WARN_UNKNOWN_SYMBOLS` warnings as
> +errors.

My hope is to turn other warnings in the config file into errors.

See below.


> +
> +
>  `CONFIG_`
>  ---------
>  If you set `CONFIG_` in the environment, Kconfig will prefix all symbols
> diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
> index 992575f1e976..c24f637827fe 100644
> --- a/scripts/kconfig/confdata.c
> +++ b/scripts/kconfig/confdata.c
> @@ -349,7 +349,12 @@ int conf_read_simple(const char *name, int def)
>         char *p, *p2;
>         struct symbol *sym;
>         int i, def_flags;
> +       bool found_unknown =3D false;
> +       const char *warn_unknown;
> +       const char *werror;
>
> +       warn_unknown =3D getenv("KCONFIG_WARN_UNKNOWN_SYMBOLS");
> +       werror =3D getenv("KCONFIG_WERROR");
>         if (name) {
>                 in =3D zconf_fopen(name);
>         } else {
> @@ -437,6 +442,13 @@ int conf_read_simple(const char *name, int def)
>                         if (def =3D=3D S_DEF_USER) {
>                                 sym =3D sym_find(line + 2 + strlen(CONFIG=
_));
>                                 if (!sym) {
> +                                       if (warn_unknown) {
> +                                               conf_warning("unknown sym=
bol: %s",
> +                                                            line + 2 + s=
trlen(CONFIG_));
> +                                               found_unknown =3D true;
> +                                               continue;

Please drop this 'continue' because it would skip
conf_set_changed(true).

warn_unknown should keep the same code flow
except showing the warning message.


> +                                       }
> +
>                                         conf_set_changed(true);
>                                         continue;
>                                 }
> @@ -471,6 +483,13 @@ int conf_read_simple(const char *name, int def)
>
>                         sym =3D sym_find(line + strlen(CONFIG_));
>                         if (!sym) {
> +                               if (warn_unknown && def !=3D S_DEF_AUTO) =
{
> +                                       conf_warning("unknown symbol: %s"=
,
> +                                                    line + strlen(CONFIG=
_));
> +                                       found_unknown =3D true;
> +                                       continue;

Same here.
When KCONFIG_WARN_UNKNOWN_SYMBOLS is set,
conf_set_changed(true) will be skipped.


You can do like this:


@@ -471,7 +483,7 @@ int conf_read_simple(const char *name, int def)

                        sym =3D sym_find(line + strlen(CONFIG_));
                        if (!sym) {
-                               if (def =3D=3D S_DEF_AUTO)
+                               if (def =3D=3D S_DEF_AUTO) {
                                        /*
                                         * Reading from include/config/auto=
.conf
                                         * If CONFIG_FOO previously existed=
 in
@@ -479,8 +491,13 @@ int conf_read_simple(const char *name, int def)
                                         * include/config/FOO must be touch=
ed.
                                         */
                                        conf_touch_dep(line + strlen(CONFIG=
_));
-                               else
+                               } else {
+                                       if (warn_unknown && def !=3D S_DEF_=
AUTO)
+                                               conf_warning("unknown
symbol: %s",
+                                                            line +
strlen(CONFIG_));
+
                                        conf_set_changed(true);
+                               }
                                continue;
                        }


> +                               }
> +
>                                 if (def =3D=3D S_DEF_AUTO)
>                                         /*
>                                          * Reading from include/config/au=
to.conf
> @@ -519,6 +538,10 @@ int conf_read_simple(const char *name, int def)
>         }
>         free(line);
>         fclose(in);
> +
> +       if (found_unknown && werror)
> +               exit(1);


I like to reuse 'conf_warnings' as you did in the previous version.

      if (conf_warnings && werror)
                exit(1)


Then, you do not need to add 'found_unknown'.


--=20
Best Regards
Masahiro Yamada