diff mbox series

[v1,2/3] kconfig: Ask user if string needs to be changed when dependency changed

Message ID 20210215122513.1773897-3-mic@digikod.net (mailing list archive)
State New
Headers show
Series Kconfig oldconfig string update | expand

Commit Message

Mickaël Salaün Feb. 15, 2021, 12:25 p.m. UTC
From: Mickaël Salaün <mic@linux.microsoft.com>

Content of string configuration may depend on related kernel
configurations.  Modify oldconfig and syncconfig to inform users about
possible required configuration update and give them the opportunity to
update it:
* if dependencies of this string has changed (e.g. enabled or disabled),
* and if the current value of this string is different than the (new)
  default one.

This is particularly relevant for CONFIG_LSM which contains a list of
LSMs enabled at boot, but users will not have a chance to update this
list with a make oldconfig.

Cc: Casey Schaufler <casey@schaufler-ca.com>
Cc: James Morris <jmorris@namei.org>
Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: Serge E. Hallyn <serge@hallyn.com>
Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
Link: https://lore.kernel.org/r/20210215122513.1773897-3-mic@digikod.net
---
 scripts/kconfig/conf.c | 37 ++++++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

Comments

Boris Kolpackov Feb. 15, 2021, 2:13 p.m. UTC | #1
Mickaël Salaün <mic@digikod.net> writes:

> Content of string configuration may depend on related kernel
> configurations.  Modify oldconfig and syncconfig to inform users about
> possible required configuration update and give them the opportunity to
> update it:
> * if dependencies of this string has changed (e.g. enabled or disabled),
> * and if the current value of this string is different than the (new)
>   default one.

I have a number of questions:

1. Why is a change in dependencies necessarily means that the dependent's
   value must be revised? Here is a specific example (to make sure we are
   talking about the same things):

   config FOO
     string "Foo value"
     depends on BAR || BAZ

   Why, in the general case, when I disable BAR and enable BAZ I must
   also revise the value of FOO?

2. How do you know that what's in the user's .config is the old default
   and in Kconfig -- the new default value? What if in the user's .config
   is a custom value (with which the user is perfectly happy) and what's
   in Kconfig is the old default (which the user has already seen)?

3. Why limit this to strings only?


> This is particularly relevant for CONFIG_LSM which contains a list of
> LSMs enabled at boot, but users will not have a chance to update this
> list with a make oldconfig.

If my understanding above is correct, this feels like it's been purpose-
made to address whatever issue you are having with CONFIG_LSM. If so,
what about potential numerous other options that don't have this issue
but will now be presented to the user for modification?
Mickaël Salaün Feb. 15, 2021, 3:40 p.m. UTC | #2
On 15/02/2021 15:13, Boris Kolpackov wrote:
> Mickaël Salaün <mic@digikod.net> writes:
> 
>> Content of string configuration may depend on related kernel
>> configurations.  Modify oldconfig and syncconfig to inform users about
>> possible required configuration update and give them the opportunity to
>> update it:
>> * if dependencies of this string has changed (e.g. enabled or disabled),
>> * and if the current value of this string is different than the (new)
>>   default one.
> 
> I have a number of questions:
> 
> 1. Why is a change in dependencies necessarily means that the dependent's
>    value must be revised? Here is a specific example (to make sure we are
>    talking about the same things):
> 
>    config FOO
>      string "Foo value"
>      depends on BAR || BAZ
> 
>    Why, in the general case, when I disable BAR and enable BAZ I must
>    also revise the value of FOO?

It may be necessary, or not, depending of the use of the string. This
semantic is not clearly expressed by kconfig but looking at the current
configuration, there is only 4 strings depending on more than one
dependency:
* SIMDISK1_FILENAME for arch/xtensa
* CMDLINE for arch/sh
* SECURITY_TOMOYO_POLICY_LOADER
* SECURITY_TOMOYO_ACTIVATION_TRIGGER

Such patterns seem in line with this patch.

> 
> 2. How do you know that what's in the user's .config is the old default
>    and in Kconfig -- the new default value? What if in the user's .config
>    is a custom value (with which the user is perfectly happy) and what's
>    in Kconfig is the old default (which the user has already seen)?

The current behavior (i.e. keeping the current user config) is not
changed. The oldconfig target only stops when a string may require an
update, shows to the user the (potentially new but not necessary best)
default value along with the value already in place in the .config file,
and if the user just type enter this current value will not be changed.

> 
> 3. Why limit this to strings only?

Strings contain configuration blobs that may be interpreted by the
kernel but not by kconfig (cf. CONFIG_LSM). It will still be possible to
handle other types if there is some related use cases.

> 
> 
>> This is particularly relevant for CONFIG_LSM which contains a list of
>> LSMs enabled at boot, but users will not have a chance to update this
>> list with a make oldconfig.
> 
> If my understanding above is correct, this feels like it's been purpose-
> made to address whatever issue you are having with CONFIG_LSM. If so,
> what about potential numerous other options that don't have this issue
> but will now be presented to the user for modification?

This patch series helps address the LSM stacking issue. The 4 other
cases may benefit from this patch too.
diff mbox series

Patch

diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
index 18a233d27a8d..8633dacd39a9 100644
--- a/scripts/kconfig/conf.c
+++ b/scripts/kconfig/conf.c
@@ -82,6 +82,26 @@  static void xfgets(char *str, int size, FILE *in)
 		printf("%s", str);
 }
 
+static bool may_need_string_update(struct symbol *sym, const char *def)
+{
+	const struct symbol *dep_sym;
+	const struct expr *e;
+
+	if (sym->type != S_STRING)
+		return false;
+	if (strcmp(def, sym_get_string_default(sym)) == 0)
+		return false;
+	/*
+	 * The user may want to synchronize the content of a string related to
+	 * changed dependencies (e.g. CONFIG_LSM).
+	 */
+	expr_list_for_each_sym(sym->dir_dep.expr, e, dep_sym) {
+		if (dep_sym->flags & SYMBOL_CHANGED)
+			return true;
+	}
+	return false;
+}
+
 static int conf_askvalue(struct symbol *sym, const char *def)
 {
 	enum symbol_type type = sym_get_type(sym);
@@ -102,7 +122,7 @@  static int conf_askvalue(struct symbol *sym, const char *def)
 	switch (input_mode) {
 	case oldconfig:
 	case syncconfig:
-		if (sym_has_value(sym)) {
+		if (sym_has_value(sym) && !may_need_string_update(sym, def)) {
 			printf("%s\n", def);
 			return 0;
 		}
@@ -137,8 +157,19 @@  static int conf_string(struct menu *menu)
 		printf("%*s%s ", indent - 1, "", menu->prompt->text);
 		printf("(%s) ", sym->name);
 		def = sym_get_string_value(sym);
-		if (def)
-			printf("[%s] ", def);
+		if (def) {
+			if (may_need_string_update(sym, def)) {
+				indent += 2;
+				printf("\n%*sDefault value is [%s]\n",
+						indent - 1, "",
+						sym_get_string_default(sym));
+				printf("%*sCurrent value is [%s] ",
+						indent - 1, "", def);
+				indent -= 2;
+			} else {
+				printf("[%s] ", def);
+			}
+		}
 		if (!conf_askvalue(sym, def))
 			return 0;
 		switch (line[0]) {