Re: [PATCH] kconfig: add warn-unknown-symbols sanity check

[Masahiro Yamada]

On Sun, Aug 27, 2023 at 6:00 PM Sergey Senozhatsky
<senozhatsky@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> ---
>  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/kconfig.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 = false;
> +       const char *warn_unknown;
> +       const char *werror;
>
> +       warn_unknown = getenv("KCONFIG_WARN_UNKNOWN_SYMBOLS");
> +       werror = getenv("KCONFIG_WERROR");
>         if (name) {
>                 in = zconf_fopen(name);
>         } else {
> @@ -437,6 +442,13 @@ int conf_read_simple(const char *name, int def)
>                         if (def == S_DEF_USER) {
>                                 sym = sym_find(line + 2 + strlen(CONFIG_));
>                                 if (!sym) {
> +                                       if (warn_unknown) {
> +                                               conf_warning("unknown symbol: %s",
> +                                                            line + 2 + strlen(CONFIG_));
> +                                               found_unknown = 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 = sym_find(line + strlen(CONFIG_));
>                         if (!sym) {
> +                               if (warn_unknown && def != S_DEF_AUTO) {
> +                                       conf_warning("unknown symbol: %s",
> +                                                    line + strlen(CONFIG_));
> +                                       found_unknown = 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 = sym_find(line + strlen(CONFIG_));
                        if (!sym) {
-                               if (def == S_DEF_AUTO)
+                               if (def == 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 touched.
                                         */
                                        conf_touch_dep(line + strlen(CONFIG_));
-                               else
+                               } else {
+                                       if (warn_unknown && def != S_DEF_AUTO)
+                                               conf_warning("unknown
symbol: %s",
+                                                            line +
strlen(CONFIG_));
+
                                        conf_set_changed(true);
+                               }
                                continue;
                        }





> +                               }
> +
>                                 if (def == S_DEF_AUTO)
>                                         /*
>                                          * Reading from include/config/auto.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'.






-- 
Best Regards
Masahiro Yamada