Re: [PATCH] kconfig: add dependency warning print about invalid values in verbose mode
From: Masahiro Yamada
Date: Sat Aug 19 2023 - 20:30:49 EST
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.
>
> 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"};
> 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]);
> + 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");
> + 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'};
>
> /* 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,
> + 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,
> + 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