On Sat, Aug 19, 2023 at 8:59 PM Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote: > > On Wed, Aug 9, 2023 at 9:32 AM <sunying@xxxxxxxxxxxxxx> wrote: > > > > From: Ying Sun <sunying@xxxxxxxxxxxxxx> > > > > Add warning about the configuration option's invalid value in verbose mode, > > including error causes, mismatch dependency, old and new values, > > to help users correct them. It should also warn about invalid options which were being talked about in other places. https://lore.kernel.org/all/20230817012007.131868-1-senozhatsky@xxxxxxxxxxxx/T/#u https://lore.kernel.org/lkml/CAMuHMdWF0LseSGK6=aodXaoK9D16mxok4DDRs=gKoGox8k6zjg@xxxxxxxxxxxxxx/T/#mb5b73532166014ce0a66b0e7e11dbe06528d863c I can also cause a segfault by removing options such as removing `CONFIG_SMP=y` from x86_64_defconfig > > > > Detailed error messages are printed only when the environment variable > > is set like "KCONFIG_VERBOSE=1". > > By default, the current behavior is not changed. > > > > Signed-off-by: Siyuan Guo <zy21df106@xxxxxxxxxxx> > > Signed-off-by: Ying Sun <sunying@xxxxxxxxxxxxxx> > > > This patch is much bigger than I had expected. > > I did not go through all the lines, but > I think the behavior should be improved. > > > > [1] A downgrade from 'y' to 'm' should be warned. > > > You only check (sym->dir_dep.tri == no && sym->def[S_DEF_USER].tri != no) > > but (sym->dir_dep.tri == mod && sym->def[S_DEF_USER].tri == yes) > will cause =y to be downgraded to =m. > This must be warned. > > > > > [2] A new CONFIG option should not be warned. > > $ make defconfig > > Add the following two lines to a Kconfig file. > > config THIS_IS_A_NEW_OPTION > def_bool y > > > $ make KCONFIG_VERBOSE=1 oldconfig > > ERROR : THIS_IS_A_NEW_OPTION[n => y] value is invalid > due to it has default value > # > # configuration written to .config > # > > > > > This is totally harmless. > I do not understand why it is warned. > > > > > > BTW, your patch subject says > "add dependency warning", but your actually code prints "ERROR". > > Also, check the coding style. > > > > > > > > > --- > > scripts/kconfig/confdata.c | 121 +++++++++++++++++++++++++++++++++++-- > > scripts/kconfig/lkc.h | 3 + > > scripts/kconfig/symbol.c | 82 +++++++++++++++++++++++-- > > 3 files changed, 195 insertions(+), 11 deletions(-) > > > > diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c > > index 992575f1e976..fa2ae6f63352 100644 > > --- a/scripts/kconfig/confdata.c > > +++ b/scripts/kconfig/confdata.c > > @@ -154,6 +154,7 @@ static void conf_message(const char *fmt, ...) > > > > static const char *conf_filename; > > static int conf_lineno, conf_warnings; > > +const char *verbose; > > > > static void conf_warning(const char *fmt, ...) > > { > > @@ -226,7 +227,7 @@ static const char *conf_get_rustccfg_name(void) > > static int conf_set_sym_val(struct symbol *sym, int def, int def_flags, char *p) > > { > > char *p2; > > - > > + static const char * const type[] = {"unknown", "bool", "tristate", "int", "hex", "string"}; Add a new line after the declaration. Not sure why checkpatch didn't get this. > > switch (sym->type) { > > case S_TRISTATE: > > if (p[0] == 'm') { > > @@ -246,9 +247,14 @@ static int conf_set_sym_val(struct symbol *sym, int def, int def_flags, char *p) > > sym->flags |= def_flags; > > break; > > } > > - if (def != S_DEF_AUTO) > > - conf_warning("symbol value '%s' invalid for %s", > > + if (def != S_DEF_AUTO) { > > + if (verbose) > > + conf_warning("symbol value '%s' invalid for %s\n due to its type is %s", > > + p, sym->name, type[sym->type]); normally indented to the opening parenthesis, same with the rest. > > + else > > + conf_warning("symbol value '%s' invalid for %s", > > p, sym->name); > > + } > > return 1; > > case S_STRING: > > /* No escaping for S_DEF_AUTO (include/config/auto.conf) */ > > @@ -274,9 +280,14 @@ static int conf_set_sym_val(struct symbol *sym, int def, int def_flags, char *p) > > sym->def[def].val = xstrdup(p); > > sym->flags |= def_flags; > > } else { > > - if (def != S_DEF_AUTO) > > - conf_warning("symbol value '%s' invalid for %s", > > - p, sym->name); > > + if (def != S_DEF_AUTO) { > > + if (verbose) > > + conf_warning("symbol value '%s' invalid for %s\n due to its type is %s", > > + p, sym->name, type[sym->type]); > > + else > > + conf_warning("symbol value '%s' invalid for %s", > > + p, sym->name); > > + } > > return 1; > > } > > break; > > @@ -528,6 +539,7 @@ int conf_read(const char *name) > > int conf_unsaved = 0; > > int i; > > > > + verbose = getenv("KCONFIG_VERBOSE"); > > conf_set_changed(false); > > > > if (conf_read_simple(name, S_DEF_USER)) { > > @@ -559,6 +571,103 @@ int conf_read(const char *name) > > continue; > > conf_unsaved++; > > /* maybe print value in verbose mode... */ > > + if (verbose) { > > + if (sym_is_choice_value(sym)) { > > + struct property *prop = sym_get_choice_prop(sym); > > + struct symbol *defsym = prop_get_symbol(prop)->curr.val; > > + > > + if (defsym && defsym != sym) { > > + struct gstr gs = str_new(); > > + > > + str_printf(&gs, > > + "\nERROR : %s[%c => %c] value is invalid\n", > > + sym->name, > > + tristate2char[sym->def[S_DEF_USER].tri], > > + tristate2char[sym->curr.tri]); > > + str_printf(&gs, > > + " due to its not the choice default symbol\n"); > > + str_printf(&gs, > > + " the default symbol is %s\n", > > + defsym->name); > > + fputs(str_get(&gs), stderr); > > + } > > + } else { > > + switch (sym->type) { > > + case S_BOOLEAN: > > + case S_TRISTATE: > > + if (sym->dir_dep.tri == no && > > + sym->def[S_DEF_USER].tri != no) { > > + struct gstr gs = str_new(); > > + > > + str_printf(&gs, > > + "\nERROR: unmet direct dependencies detected for %s[%c => %c]\n", > > + sym->name, > > + tristate2char[sym->def[S_DEF_USER].tri], > > + tristate2char[sym->curr.tri]); > > + str_printf(&gs, > > + " Depends on [%c]: ", > > + sym->dir_dep.tri == mod ? 'm' : 'n'); > > + expr_gstr_print(sym->dir_dep.expr, &gs); > > + str_printf(&gs, "\n"); > > + fputs(str_get(&gs), stderr); > > + } else if (sym->rev_dep.tri != no) { > > + struct gstr gs = str_new(); > > + > > + str_printf(&gs, > > + "\nERROR : %s[%c => %c] value is invalid\n", > > + sym->name, > > + tristate2char[sym->def[S_DEF_USER].tri], > > + tristate2char[sym->curr.tri]); > > + str_printf(&gs, > > + " due to its invisible and it is selected\n"); As Masahiro said this is unnecessary. Maybe add a verbosity level for this. > > + 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"); > > + fputs(str_get(&gs), stderr); > > + } else { > > + sym_validate_default(sym); > > + } > > + break; > > + case S_INT: > > + case S_HEX: > > + if (sym->dir_dep.tri == no && > > + strcmp((char *)(sym->def[S_DEF_USER].val), "") != 0) { > > + struct gstr gs = str_new(); > > + > > + str_printf(&gs, > > + "\nERROR: unmet direct dependencies detected for %s\n", > > + sym->name); > > + str_printf(&gs, > > + " Depends on [%c]: ", > > + sym->dir_dep.tri == mod ? 'm' : 'n'); > > + expr_gstr_print(sym->dir_dep.expr, &gs); > > + str_printf(&gs, "\n"); > > + fputs(str_get(&gs), stderr); > > + } else { > > + sym_validate_default(sym); > > + } > > + break; > > + case S_STRING: > > + if (sym->dir_dep.tri == no && > > + strcmp((char *)(sym->def[S_DEF_USER].val), "") != 0) { > > + struct gstr gs = str_new(); > > + > > + str_printf(&gs, > > + "\nERROR: unmet direct dependencies detected for %s\n", > > + sym->name); > > + str_printf(&gs, > > + " Depends on [%c]: ", > > + sym->dir_dep.tri == mod ? 'm' : 'n'); > > + expr_gstr_print(sym->dir_dep.expr, &gs); > > + str_printf(&gs, "\n"); > > + fputs(str_get(&gs), stderr); > > + } > > + default: > > + break; > > + } > > + } > > + } > > } > > > > for_all_symbols(i, sym) { > > diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h > > index 471a59acecec..820a47fb4968 100644 > > --- a/scripts/kconfig/lkc.h > > +++ b/scripts/kconfig/lkc.h > > @@ -38,6 +38,8 @@ void zconf_initscan(const char *name); > > void zconf_nextfile(const char *name); > > int zconf_lineno(void); > > const char *zconf_curname(void); > > +extern const char *verbose; > > +static const char tristate2char[3] = {'n', 'm', 'y'}; This seems non-ideal > > > > /* confdata.c */ > > const char *conf_get_configname(void); > > @@ -112,6 +114,7 @@ struct property *sym_get_range_prop(struct symbol *sym); > > const char *sym_get_string_default(struct symbol *sym); > > struct symbol *sym_check_deps(struct symbol *sym); > > struct symbol *prop_get_symbol(struct property *prop); > > +void sym_validate_default(struct symbol *sym); > > > > static inline tristate sym_get_tristate_value(struct symbol *sym) > > { > > diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c > > index 0572330bf8a7..8b11d6ea1d30 100644 > > --- a/scripts/kconfig/symbol.c > > +++ b/scripts/kconfig/symbol.c > > @@ -91,6 +91,53 @@ static struct property *sym_get_default_prop(struct symbol *sym) > > return NULL; > > } > > > > +void sym_validate_default(struct symbol *sym) > > +{ > > + if (sym->visible == no) { > > + struct gstr gs = str_new(); > > + const char *value = sym_get_string_default(sym); > > + > > + switch (sym->type) { > > + case S_BOOLEAN: > > + case S_TRISTATE: > > + if (strcmp(value, "n") != 0) { > > + str_printf(&gs, > > + "\nERROR : %s[%c => %c] value is invalid\n due to it has default value\n", > > + sym->name, Same here > > + tristate2char[sym->def[S_DEF_USER].tri], > > + tristate2char[sym->curr.tri]); > > + } else if (sym->implied.tri != no) { > > + str_printf(&gs, > > + "\nERROR : %s[%c => %c] value is invalid\n due to its invisible and has imply value\n", > > + sym->name, And here > > + tristate2char[sym->def[S_DEF_USER].tri], > > + tristate2char[sym->curr.tri]); > > + str_printf(&gs, > > + " Imply : "); > > + expr_gstr_print(sym->implied.expr, &gs); > > + str_printf(&gs, "\n"); > > + } > > + break; > > + case S_STRING: > > + case S_INT: > > + case S_HEX: > > + if (strcmp(value, "") != 0) { > > + str_printf(&gs, > > + "\nERROR : %s[%s => %s] value is invalid\n", > > + sym->name, > > + (char *)(sym->def[S_DEF_USER].val), > > + (char *)(sym->curr.val)); > > + str_printf(&gs, > > + " due to it has default value\n"); > > + } > > + break; > > + default: > > + break; > > + } > > + fputs(str_get(&gs), stderr); > > + } > > +} > > + > > struct property *sym_get_range_prop(struct symbol *sym) > > { > > struct property *prop; > > @@ -600,7 +647,8 @@ bool sym_string_valid(struct symbol *sym, const char *str) > > bool sym_string_within_range(struct symbol *sym, const char *str) > > { > > struct property *prop; > > - long long val; > > + long long val, left, right; > > + struct gstr gs = str_new(); > > > > switch (sym->type) { > > case S_STRING: > > @@ -612,8 +660,20 @@ bool sym_string_within_range(struct symbol *sym, const char *str) > > if (!prop) > > return true; > > val = strtoll(str, NULL, 10); > > - return val >= sym_get_range_val(prop->expr->left.sym, 10) && > > - val <= sym_get_range_val(prop->expr->right.sym, 10); > > + left = sym_get_range_val(prop->expr->left.sym, 10); > > + right = sym_get_range_val(prop->expr->right.sym, 10); > > + if (val >= left && val <= right) > > + return true; > > + if (verbose) { > > + str_printf(&gs, > > + "\nERROR: unmet range detected for %s\n", > > + sym->name); > > + str_printf(&gs, > > + " symbol value is %lld, the range is (%lld %lld)\n", > > + val, left, right); > > + fputs(str_get(&gs), stderr); > > + } > > + return false; > > case S_HEX: > > if (!sym_string_valid(sym, str)) > > return false; > > @@ -621,8 +681,20 @@ bool sym_string_within_range(struct symbol *sym, const char *str) > > if (!prop) > > return true; > > val = strtoll(str, NULL, 16); > > - return val >= sym_get_range_val(prop->expr->left.sym, 16) && > > - val <= sym_get_range_val(prop->expr->right.sym, 16); > > + left = sym_get_range_val(prop->expr->left.sym, 16); > > + right = sym_get_range_val(prop->expr->right.sym, 16); > > + if (val >= left && val <= right) > > + return true; > > + if (verbose) { > > + str_printf(&gs, > > + "\nERROR: unmet range detected for %s\n", > > + sym->name); > > + str_printf(&gs, > > + " symbol value is 0x%llx, the range is (0x%llx 0x%llx)\n", > > + val, left, right); > > + fputs(str_get(&gs), stderr); > > + } > > + return false; > > case S_BOOLEAN: > > case S_TRISTATE: > > switch (str[0]) { > > -- > > 2.17.1 > > > > > -- > Best Regards > Masahiro Yamada
