2024-01-02 02:11:18

by sunying

[permalink] [raw]
Subject: [PATCHv3 next] kconfig: add dependency warning print about invalid values in verbose mode

From: Ying Sun <[email protected]>

Warning in verbose mode about incorrect causes and
mismatch dependency of invalid values to help users correct them.

Detailed warning messages are printed only when the environment variable
is set like "KCONFIG_WARN_CHANGED_INPUT=1".
By default, the current behavior is not changed.

Signed-off-by: Siyuan Guo <[email protected]>
Signed-off-by: Ying Sun <[email protected]>
---
v2 -> v3:
* Fixed warning message that mess up the ncurses display.
* Fixed memory leak where str_new() was called but str_free() was not used.
* Integrated the code to avoid excessive dispersion.
* Use the environment variable KCONFIG_WARN_CHANGED_INPUT as the switch

v1 -> v2:
* Reduced the number of code lines by refactoring and simplifying the logic.
* Changed the print "ERROR" to "WARNING".
* Focused on handling dependency errors: dir_dep and rev_dep, and range error.
- A downgrade from 'y' to 'm' has be warned.
- A new CONFIG option should not be warned.
- Overwriting caused by default value is not an error and is no longer printed.
* Fixed style issues.
---
---
scripts/kconfig/confdata.c | 76 ++++++++++++++++++++++++++++++++++++++
1 file changed, 76 insertions(+)

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index f1197e672431..352774928558 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -195,6 +195,52 @@ static void conf_message(const char *fmt, ...)
va_end(ap);
}

+static void conf_warn_unmet_rev_dep(struct symbol *sym)
+{
+ struct gstr gs = str_new();
+ char value = 'n';
+
+ switch (sym->curr.tri) {
+ case no:
+ value = 'n';
+ break;
+ case mod:
+ sym_calc_value(modules_sym);
+ value = (modules_sym->curr.tri == no) ? 'n' : 'm';
+ break;
+ case yes:
+ value = 'y';
+ }
+
+ str_printf(&gs,
+ "'%s' set to %c for it is selected\n",
+ sym->name, value);
+ expr_gstr_print_revdep(sym->rev_dep.expr, &gs, yes,
+ " Selected by [y]:\n");
+ expr_gstr_print_revdep(sym->rev_dep.expr, &gs, mod,
+ " Selected by [m]:\n");
+
+ conf_warning("%s", str_get(&gs));
+ str_free(&gs);
+}
+
+static void conf_warn_dep_error(struct symbol *sym)
+{
+ struct gstr gs = str_new();
+
+ str_printf(&gs,
+ "'%s' set to n for it unmet direct dependencies\n",
+ sym->name);
+
+ str_printf(&gs,
+ " Depends on [%c]: ",
+ sym->dir_dep.tri == mod ? 'm' : 'n');
+ expr_gstr_print(sym->dir_dep.expr, &gs);
+
+ conf_warning("%s\n", str_get(&gs));
+ str_free(&gs);
+}
+
const char *conf_get_configname(void)
{
char *name = getenv("KCONFIG_CONFIG");
@@ -568,6 +614,36 @@ int conf_read(const char *name)
continue;
conf_unsaved++;
/* maybe print value in verbose mode... */
+ if (getenv("KCONFIG_WARN_CHANGED_INPUT") && (sym->prop)) {
+ conf_filename = sym->prop->file->name;
+ conf_lineno = sym->prop->menu->lineno;
+
+ switch (sym->type) {
+ case S_BOOLEAN:
+ case S_TRISTATE:
+ if (sym->def[S_DEF_USER].tri != sym_get_tristate_value(sym)) {
+ if (sym->dir_dep.tri < sym->def[S_DEF_USER].tri)
+ conf_warn_dep_error(sym);
+ else if (sym->rev_dep.tri > sym->def[S_DEF_USER].tri)
+ conf_warn_unmet_rev_dep(sym);
+ else
+ conf_warning("'%s' set to %s due to option constraints\n",
+ sym->name, sym_get_string_value(sym));
+ }
+ break;
+ case S_INT:
+ case S_HEX:
+ case S_STRING:
+ if (sym->dir_dep.tri == no && sym_has_value(sym))
+ conf_warn_dep_error(sym);
+ else
+ conf_warning("'%s' set to %s due to option constraints\n",
+ sym->name, sym_get_string_value(sym));
+ break;
+ default:
+ break;
+ }
+ }
}

for_all_symbols(i, sym) {
--
2.43.0



2024-02-10 23:25:39

by Masahiro Yamada

[permalink] [raw]
Subject: Re: [PATCHv3 next] kconfig: add dependency warning print about invalid values in verbose mode

"in verbose mode" is stale.

Please rephrase the subject.




On Tue, Jan 2, 2024 at 11:11 AM <[email protected]> wrote:
>
> From: Ying Sun <[email protected]>
>
> Warning in verbose mode about incorrect causes and
> mismatch dependency of invalid values to help users correct them.

Same here, "verbose mode" does not exist in this patch.




>
> Detailed warning messages are printed only when the environment variable
> is set like "KCONFIG_WARN_CHANGED_INPUT=1".
> By default, the current behavior is not changed.
>
> Signed-off-by: Siyuan Guo <[email protected]>
> Signed-off-by: Ying Sun <[email protected]>
> ---
> v2 -> v3:
> * Fixed warning message that mess up the ncurses display.
> * Fixed memory leak where str_new() was called but str_free() was not used.
> * Integrated the code to avoid excessive dispersion.
> * Use the environment variable KCONFIG_WARN_CHANGED_INPUT as the switch




This checker prints wrong reports.


I just attached one test case.



[test Kconfig]

config MODULES
def_bool y
modules

config FOO
tristate "foo"
depends on BAR

config BAR
tristate "bar"

config BAZ
tristate "baz"
select FOO


[test .config]

CONFIG_FOO=m
# CONFIG_BAR is not set
CONFIG_BAZ=y



[test command]


$ KCONFIG_WARN_CHANGED_INPUT=1 make olddefconfig



[result]


Kconfig:8:warning: 'MODULES' set to y due to option constraints


WARNING: unmet direct dependencies detected for FOO
Depends on [n]: BAR [=n]
Selected by [y]:
- BAZ [=y]
Kconfig:12:warning: 'FOO' set to n for it unmet direct dependencies
Depends on [n]: BAR [=n]



$ cat .config
#
# Automatically generated file; DO NOT EDIT.
# Linux/x86 6.8.0-rc3 Kernel Configuration
#
CONFIG_MODULES=y
CONFIG_FOO=y
# CONFIG_BAR is not set
CONFIG_BAZ=y







The first report

Kconfig:8:warning: 'MODULES' set to y due to option constraints

should not be printed.

CONFIG options without prompt can be omitted.




The second report

Kconfig:12:warning: 'FOO' set to n for it unmet direct dependencies

is completely wrong.



CONFIG_FOO was changed to 'y' due to the select.





One more thing,

Add
export KCONFIG_WARN_CHANGED_INPUT=1

to scripts/kconfig/Makefile




> v1 -> v2:
> * Reduced the number of code lines by refactoring and simplifying the logic.
> * Changed the print "ERROR" to "WARNING".
> * Focused on handling dependency errors: dir_dep and rev_dep, and range error.
> - A downgrade from 'y' to 'm' has be warned.
> - A new CONFIG option should not be warned.
> - Overwriting caused by default value is not an error and is no longer printed.
> * Fixed style issues.
> ---
> ---
> scripts/kconfig/confdata.c | 76 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 76 insertions(+)
>
> diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
> index f1197e672431..352774928558 100644
> --- a/scripts/kconfig/confdata.c
> +++ b/scripts/kconfig/confdata.c
> @@ -195,6 +195,52 @@ static void conf_message(const char *fmt, ...)
> va_end(ap);
> }
>
> +static void conf_warn_unmet_rev_dep(struct symbol *sym)
> +{
> + struct gstr gs = str_new();
> + char value = 'n';
> +
> + switch (sym->curr.tri) {
> + case no:
> + value = 'n';
> + break;
> + case mod:
> + sym_calc_value(modules_sym);
> + value = (modules_sym->curr.tri == no) ? 'n' : 'm';
> + break;
> + case yes:
> + value = 'y';
> + }
> +
> + str_printf(&gs,
> + "'%s' set to %c for it is selected\n",
> + sym->name, value);
> + expr_gstr_print_revdep(sym->rev_dep.expr, &gs, yes,
> + " Selected by [y]:\n");
> + expr_gstr_print_revdep(sym->rev_dep.expr, &gs, mod,
> + " Selected by [m]:\n");
> +
> + conf_warning("%s", str_get(&gs));
> + str_free(&gs);
> +}
> +
> +static void conf_warn_dep_error(struct symbol *sym)
> +{
> + struct gstr gs = str_new();
> +
> + str_printf(&gs,
> + "'%s' set to n for it unmet direct dependencies\n",
> + sym->name);
> +
> + str_printf(&gs,
> + " Depends on [%c]: ",
> + sym->dir_dep.tri == mod ? 'm' : 'n');
> + expr_gstr_print(sym->dir_dep.expr, &gs);
> +
> + conf_warning("%s\n", str_get(&gs));
> + str_free(&gs);
> +}
> +
> const char *conf_get_configname(void)
> {
> char *name = getenv("KCONFIG_CONFIG");
> @@ -568,6 +614,36 @@ int conf_read(const char *name)
> continue;
> conf_unsaved++;
> /* maybe print value in verbose mode... */
> + if (getenv("KCONFIG_WARN_CHANGED_INPUT") && (sym->prop)) {
> + conf_filename = sym->prop->file->name;
> + conf_lineno = sym->prop->menu->lineno;
> +
> + switch (sym->type) {
> + case S_BOOLEAN:
> + case S_TRISTATE:
> + if (sym->def[S_DEF_USER].tri != sym_get_tristate_value(sym)) {
> + if (sym->dir_dep.tri < sym->def[S_DEF_USER].tri)
> + conf_warn_dep_error(sym);
> + else if (sym->rev_dep.tri > sym->def[S_DEF_USER].tri)
> + conf_warn_unmet_rev_dep(sym);
> + else
> + conf_warning("'%s' set to %s due to option constraints\n",
> + sym->name, sym_get_string_value(sym));
> + }
> + break;
> + case S_INT:
> + case S_HEX:
> + case S_STRING:
> + if (sym->dir_dep.tri == no && sym_has_value(sym))
> + conf_warn_dep_error(sym);
> + else
> + conf_warning("'%s' set to %s due to option constraints\n",
> + sym->name, sym_get_string_value(sym));
> + break;
> + default:
> + break;
> + }
> + }
> }
>
> for_all_symbols(i, sym) {
> --
> 2.43.0
>
>


--
Best Regards


Masahiro Yamada