2023-08-29 19:22:15

by Masahiro Yamada

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

On Sun, Aug 27, 2023 at 6:00 PM Sergey Senozhatsky
<[email protected]> 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 <[email protected]>
> ---
> 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


2023-08-31 00:15:55

by Sergey Senozhatsky

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

On (23/08/29 22:13), Masahiro Yamada wrote:
> > +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".

Done.

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

Done.

> > +++ 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).

My bad. Those 'continue' are left-overs from previous version.

> > + }
> > +
> > 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.

Same here. My bad.

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

Done.