From: Michal Marek on
On 25.7.2010 23:40, Sam Ravnborg wrote:
> From 5edffcc6890a7dbd43b8da9c453bee794e81e7c7 Mon Sep 17 00:00:00 2001
> From: Sam Ravnborg <sam(a)ravnborg.org>
> Date: Sun, 25 Jul 2010 23:18:47 +0200
> Subject: [PATCH 4/4] kconfig: add savedefconfig
> MIME-Version: 1.0
> Content-Type: text/plain; charset=utf-8
> Content-Transfer-Encoding: 8bit
>
> savedefconfig is used to save a minimal configuration
> that is useful for a defconfig.

General comment: I like this more than the "replace defconfigs with
Kconfig files" approach -- the .config format is efficient and easy to
read, we have tools to edit it (make *config), the issue was the size of
the defconfigs and the updates to them, which your patches address now.



> Signed-off-by: Sam Ravnborg <sam(a)ravnborg.org>
> Cc: Uwe Kleine-König <u.kleine-koenig(a)pengutronix.de>
> Cc: Stephen Rothwell <sfr(a)canb.auug.org.au>
> ---
> scripts/kconfig/Makefile | 7 +++-
> scripts/kconfig/conf.c | 20 ++++++++-
> scripts/kconfig/confdata.c | 94 ++++++++++++++++++++++++++++++++++++++++++
> scripts/kconfig/lkc.h | 2 +
> scripts/kconfig/lkc_proto.h | 1 +
> scripts/kconfig/symbol.c | 96 +++++++++++++++++++++++++++++++++++++++++++
> 6 files changed, 217 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/kconfig/Makefile b/scripts/kconfig/Makefile
> index 366711a..aeb04c2 100644
> --- a/scripts/kconfig/Makefile
> +++ b/scripts/kconfig/Makefile
> @@ -95,7 +95,8 @@ update-po-config: $(obj)/kxgettext $(obj)/gconf.glade.h
> $(Q)rm -f arch/um/Kconfig.arch
> $(Q)rm -f $(obj)/config.pot
>
> -PHONY += randconfig allyesconfig allnoconfig allmodconfig alldefconfig defconfig
> +PHONY += randconfig allyesconfig allnoconfig allmodconfig alldefconfig
> +PHONY += defconfig savedefconfig
>
> randconfig: $(obj)/conf
> $< -r $(Kconfig)
> @@ -112,6 +113,9 @@ allmodconfig: $(obj)/conf
> alldefconfig: $(obj)/conf
> $< -f $(Kconfig)
>
> +savedefconfig: $(obj)/conf
> + $< -M defconfig $(Kconfig)
> +

Out of curiosity, do you have any mnemonics for -f and -M, or are these
just random letters that were free? :) Maybe we should add long options
like --defconfig, --allmodconfig for each of the targets and use them in
the Makefile. The meaning of the single-letter options is not obvious
sometimes. scripts/genksyms already uses getopt_long() so we would not
regress in portability.


> defconfig: $(obj)/conf
> ifeq ($(KBUILD_DEFCONFIG),)
> $< -d $(Kconfig)
> @@ -140,6 +144,7 @@ help:
> @echo ' allmodconfig - New config selecting modules when possible'
> @echo ' allyesconfig - New config where all options are accepted with yes'
> @echo ' allnoconfig - New config where all options are answered with no'
> + @echo ' savedefconfig - Save current config as ./defconfig (minimal config)'
>
> # lxdialog stuff
> check-lxdialog := $(srctree)/$(src)/lxdialog/check-lxdialog.sh
> diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
> index 2b4775e..b1a903b 100644
> --- a/scripts/kconfig/conf.c
> +++ b/scripts/kconfig/conf.c
> @@ -28,7 +28,8 @@ enum {
> set_mod,
> set_no,
> set_default,
> - set_random
> + set_random,
> + save_defconfig,
> } input_mode = ask_all;
> char *defconfig_file;
>
> @@ -440,7 +441,7 @@ int main(int ac, char **av)
> bindtextdomain(PACKAGE, LOCALEDIR);
> textdomain(PACKAGE);
>
> - while ((opt = getopt(ac, av, "osdD:nmyfrh")) != -1) {
> + while ((opt = getopt(ac, av, "osdD:nmM:yfrh")) != -1) {
> switch (opt) {
> case 'o':
> input_mode = ask_silent;
> @@ -485,6 +486,10 @@ int main(int ac, char **av)
> input_mode = set_random;
> break;
> }
> + case 'M':
> + input_mode = save_defconfig;
> + defconfig_file = optarg;
> + break;
> case 'h':
> printf(_("See README for usage info\n"));
> exit(0);
> @@ -553,6 +558,9 @@ int main(int ac, char **av)
> else if (!stat("all.config", &tmpstat))
> conf_read_simple("all.config", S_DEF_USER);
> break;
> + case save_defconfig:
> + conf_read(NULL);
> + break;
> default:
> break;
> }
> @@ -601,6 +609,8 @@ int main(int ac, char **av)
> check_conf(&rootmenu);
> } while (conf_cnt);
> break;
> + case save_defconfig:
> + break;
> }
>
> if (sync_kconfig) {
> @@ -615,6 +625,12 @@ int main(int ac, char **av)
> fprintf(stderr, _("\n*** Error during update of the kernel configuration.\n\n"));
> return 1;
> }
> + } else if (input_mode == save_defconfig) {
> + if (conf_write_defconfig(defconfig_file)) {
> + fprintf(stderr, _("\n*** Error during writing of mini config to %s.\n\n"),
> + defconfig_file);
> + return 1;
> + }
> } else {
> if (conf_write(NULL)) {
> fprintf(stderr, _("\n*** Error during writing of the kernel configuration.\n\n"));
> diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
> index c4dec80..76c4e23 100644
> --- a/scripts/kconfig/confdata.c
> +++ b/scripts/kconfig/confdata.c
> @@ -396,6 +396,100 @@ int conf_read(const char *name)
> return 0;
> }
>
> +/*
> + * Write out a minimal config.
> + * All values that has default values are skipped as this is redundant.
> + */
> +int conf_write_defconfig(const char *filename)
> +{
> + struct symbol *sym;
> + const char *str;
> + FILE *out;
> + int i, l;
> +
> + out = fopen(filename, "w");
> + if (!out)
> + return 1;
> +
> + sym_clear_all_valid();
> +
> + for_all_symbols(i, sym) {
> + sym_calc_value(sym);
> + if (!(sym->flags & SYMBOL_WRITE) || !sym->name)
> + goto next_symbol;
> + /* If we cannot change the symbol - skip */
> + if (!sym_is_changable(sym))
> + goto next_symbol;
> + /* If symbol equals to default value - skip */
> + if (strcmp(sym_get_string_value(sym), sym_get_string_default(sym)) == 0)
> + goto next_symbol;
> +
> + /* choice symbols does not have a value - skip */
> + if (sym_is_choice(sym))
> + goto next_symbol;
> + /*
> + * If symbol is a choice value and equals to the
> + * default for a choice - skip.
> + * But only if value equal to "y".
> + */
> + if (sym_is_choice_value(sym)) {
> + struct symbol *cs;
> + struct symbol *ds;
> +
> + cs = prop_get_symbol(sym_get_choice_prop(sym));
> + ds = sym_choice_default(cs);
> + if (sym == ds) {
> + if ((sym->type == S_BOOLEAN ||
> + sym->type == S_TRISTATE) &&
> + sym_get_tristate_value(sym) == yes)
> + goto next_symbol;
> + }
> + }
> + switch (sym->type) {

This duplicates the switch statement in conf_write(), we should move it
into a separate function and call it from conf_write() and here.


> + case S_BOOLEAN:
> + case S_TRISTATE:
> + switch (sym_get_tristate_value(sym)) {
> + case no:
> + fprintf(out, "# CONFIG_%s is not set\n", sym->name);
> + break;
> + case yes:
> + fprintf(out, "CONFIG_%s=y\n", sym->name);
> + break;
> + case mod:
> + fprintf(out, "CONFIG_%s=m\n", sym->name);
> + break;
> + }
> + break;
> + case S_STRING:
> + str = sym_get_string_value(sym);
> + fprintf(out, "CONFIG_%s=\"", sym->name);
> + while (1) {
> + l = strcspn(str, "\"\\");
> + if (l) {
> + fwrite(str, l, 1, out);
> + str += l;
> + }
> + if (!*str)
> + break;
> + fprintf(out, "\\%c", *str++);
> + }
> + fputs("\"\n", out);
> + break;
> + case S_HEX:
> + case S_INT:
> + str = sym_get_string_value(sym);
> + fprintf(out, "CONFIG_%s=%s\n", sym->name, str);
> + break;
> + default:
> + break;
> + }
> +next_symbol:
> + ;
> + }

The gotos should be replaced by simple continue statements if there is
nothing to do after the label.

(maybe there are more issues, I didn't look closely into it. But I like
the approach.)

Michal

> + fclose(out);
> + return 0;
> +}
> +
> int conf_write(const char *name)
> {
> FILE *out;
> diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
> index ce6549c..76db065 100644
> --- a/scripts/kconfig/lkc.h
> +++ b/scripts/kconfig/lkc.h
> @@ -126,6 +126,8 @@ void sym_init(void);
> void sym_clear_all_valid(void);
> void sym_set_all_changed(void);
> void sym_set_changed(struct symbol *sym);
> +struct symbol *sym_choice_default(struct symbol *sym);
> +const char *sym_get_string_default(struct symbol *sym);
> struct symbol *sym_check_deps(struct symbol *sym);
> struct property *prop_alloc(enum prop_type type, struct symbol *sym);
> struct symbol *prop_get_symbol(struct property *prop);
> diff --git a/scripts/kconfig/lkc_proto.h b/scripts/kconfig/lkc_proto.h
> index 7cadcad..e30d5f2 100644
> --- a/scripts/kconfig/lkc_proto.h
> +++ b/scripts/kconfig/lkc_proto.h
> @@ -4,6 +4,7 @@ P(conf_parse,void,(const char *name));
> P(conf_read,int,(const char *name));
> P(conf_read_simple,int,(const char *name, int));
> P(conf_write,int,(const char *name));
> +P(conf_write_defconfig,int,(const char *name));
> P(conf_write_autoconf,int,(void));
> P(conf_get_changed,bool,(void));
> P(conf_set_changed_callback, void,(void (*fn)(void)));
> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
> index 0ea9c46..0bda646 100644
> --- a/scripts/kconfig/symbol.c
> +++ b/scripts/kconfig/symbol.c
> @@ -216,6 +216,41 @@ static void sym_calc_visibility(struct symbol *sym)
> }
> }
>
> +/*
> + * Find the default isymbol for a choice.
> + * First try the default values for the choice symbol
> + * Next locate the first visible choice value
> + * Return NULL if none was found
> + */
> +struct symbol *sym_choice_default(struct symbol *sym)
> +{
> + struct property *prop;
> + struct symbol *def_sym;
> + struct expr *e;
> +
> + /* any of the defaults visible? */
> + for_all_defaults(sym, prop) {
> + prop->visible.tri = expr_calc_value(prop->visible.expr);
> + if (prop->visible.tri == no)
> + continue;
> + def_sym = prop_get_symbol(prop);
> + sym_calc_visibility(def_sym);
> + if (def_sym->visible != no)
> + return def_sym;
> + }
> +
> + /* just get the first visible value */
> + prop = sym_get_choice_prop(sym);
> + expr_list_for_each_sym(prop->expr, e, def_sym) {
> + sym_calc_visibility(def_sym);
> + if (def_sym->visible != no)
> + return def_sym;
> + }
> +
> + /* failed to locate any defaults */
> + return NULL;
> +}
> +
> static struct symbol *sym_calc_choice(struct symbol *sym)
> {
> struct symbol *def_sym;
> @@ -646,6 +681,67 @@ const char *sym_get_string_value(struct symbol *sym)
> return (const char *)sym->curr.val;
> }
>
> +/*
> + * Find the default value associated to a symbol.
> + * For tristate symbol hande the modules=n case
> + * in which case "m" bocomes "y".
> + * If the symbol does not have any default fallback
> + * to the fixed default values.
> + */
> +const char *sym_get_string_default(struct symbol *sym)
> +{
> + struct property *prop;
> + struct symbol *ds;
> + tristate val;
> +
> + prop = sym_get_default_prop(sym);
> + if (!prop)
> + goto default_value;
> + ds = prop_get_symbol(prop);
> + if (!ds)
> + goto default_value;
> +
> + switch (sym->type) {
> + case S_BOOLEAN:
> + case S_TRISTATE:
> + val = sym_get_tristate_value(ds);
> + switch (val) {
> + case no:
> + return "n";
> + case yes:
> + return "y";
> + case mod:
> + if (sym_is_choice_value(sym))
> + return "m";
> + sym_calc_value(modules_sym);
> + if (modules_sym->curr.tri == no)
> + return "y";
> + else
> + return "m";
> + }
> + break;
> + default:
> + return (const char *)ds->curr.val;
> + }
> +
> +default_value:
> +
> + switch (sym->type) {
> + case S_BOOLEAN:
> + case S_TRISTATE:
> + return "n";
> + case S_INT:
> + case S_HEX:
> + return "0";
> + case S_STRING:
> + return "";
> + case S_OTHER:
> + case S_UNKNOWN:
> + break;
> + }
> + return "";
> +}
> +
> bool sym_is_changable(struct symbol *sym)
> {
> return sym->visible > sym->rev_dep.tri;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Sam Ravnborg on
> >
> > randconfig: $(obj)/conf
> > $< -r $(Kconfig)
> > @@ -112,6 +113,9 @@ allmodconfig: $(obj)/conf
> > alldefconfig: $(obj)/conf
> > $< -f $(Kconfig)
> >
> > +savedefconfig: $(obj)/conf
> > + $< -M defconfig $(Kconfig)
> > +
>
> Out of curiosity, do you have any mnemonics for -f and -M, or are these
> just random letters that were free? :) Maybe we should add long options
> like --defconfig, --allmodconfig for each of the targets and use them in
> the Makefile. The meaning of the single-letter options is not obvious
> sometimes. scripts/genksyms already uses getopt_long() so we would not
> regress in portability.
Random letters that were free.
I will take a look at getopt_long() in next spin.

>
>
> > defconfig: $(obj)/conf
> > ifeq ($(KBUILD_DEFCONFIG),)
> > $< -d $(Kconfig)
> > @@ -140,6 +144,7 @@ help:
> > @echo ' allmodconfig - New config selecting modules when possible'
> > @echo ' allyesconfig - New config where all options are accepted with yes'
> > @echo ' allnoconfig - New config where all options are answered with no'
> > + @echo ' savedefconfig - Save current config as ./defconfig (minimal config)'
> >
> > # lxdialog stuff
> > check-lxdialog := $(srctree)/$(src)/lxdialog/check-lxdialog.sh
> > diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
> > index 2b4775e..b1a903b 100644
> > --- a/scripts/kconfig/conf.c
> > +++ b/scripts/kconfig/conf.c
> > @@ -28,7 +28,8 @@ enum {
> > set_mod,
> > set_no,
> > set_default,
> > - set_random
> > + set_random,
> > + save_defconfig,
> > } input_mode = ask_all;
> > char *defconfig_file;
> >
> > @@ -440,7 +441,7 @@ int main(int ac, char **av)
> > bindtextdomain(PACKAGE, LOCALEDIR);
> > textdomain(PACKAGE);
> >
> > - while ((opt = getopt(ac, av, "osdD:nmyfrh")) != -1) {
> > + while ((opt = getopt(ac, av, "osdD:nmM:yfrh")) != -1) {
> > switch (opt) {
> > case 'o':
> > input_mode = ask_silent;
> > @@ -485,6 +486,10 @@ int main(int ac, char **av)
> > input_mode = set_random;
> > break;
> > }
> > + case 'M':
> > + input_mode = save_defconfig;
> > + defconfig_file = optarg;
> > + break;
> > case 'h':
> > printf(_("See README for usage info\n"));
> > exit(0);
> > @@ -553,6 +558,9 @@ int main(int ac, char **av)
> > else if (!stat("all.config", &tmpstat))
> > conf_read_simple("all.config", S_DEF_USER);
> > break;
> > + case save_defconfig:
> > + conf_read(NULL);
> > + break;
> > default:
> > break;
> > }
> > @@ -601,6 +609,8 @@ int main(int ac, char **av)
> > check_conf(&rootmenu);
> > } while (conf_cnt);
> > break;
> > + case save_defconfig:
> > + break;
> > }
> >
> > if (sync_kconfig) {
> > @@ -615,6 +625,12 @@ int main(int ac, char **av)
> > fprintf(stderr, _("\n*** Error during update of the kernel configuration.\n\n"));
> > return 1;
> > }
> > + } else if (input_mode == save_defconfig) {
> > + if (conf_write_defconfig(defconfig_file)) {
> > + fprintf(stderr, _("\n*** Error during writing of mini config to %s.\n\n"),
> > + defconfig_file);
> > + return 1;
> > + }
> > } else {
> > if (conf_write(NULL)) {
> > fprintf(stderr, _("\n*** Error during writing of the kernel configuration.\n\n"));
> > diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
> > index c4dec80..76c4e23 100644
> > --- a/scripts/kconfig/confdata.c
> > +++ b/scripts/kconfig/confdata.c
> > @@ -396,6 +396,100 @@ int conf_read(const char *name)
> > return 0;
> > }
> >
> > +/*
> > + * Write out a minimal config.
> > + * All values that has default values are skipped as this is redundant.
> > + */
> > +int conf_write_defconfig(const char *filename)
> > +{
> > + struct symbol *sym;
> > + const char *str;
> > + FILE *out;
> > + int i, l;
> > +
> > + out = fopen(filename, "w");
> > + if (!out)
> > + return 1;
> > +
> > + sym_clear_all_valid();
> > +
> > + for_all_symbols(i, sym) {
> > + sym_calc_value(sym);
> > + if (!(sym->flags & SYMBOL_WRITE) || !sym->name)
> > + goto next_symbol;
> > + /* If we cannot change the symbol - skip */
> > + if (!sym_is_changable(sym))
> > + goto next_symbol;
> > + /* If symbol equals to default value - skip */
> > + if (strcmp(sym_get_string_value(sym), sym_get_string_default(sym)) == 0)
> > + goto next_symbol;
> > +
> > + /* choice symbols does not have a value - skip */
> > + if (sym_is_choice(sym))
> > + goto next_symbol;
> > + /*
> > + * If symbol is a choice value and equals to the
> > + * default for a choice - skip.
> > + * But only if value equal to "y".
> > + */
> > + if (sym_is_choice_value(sym)) {
> > + struct symbol *cs;
> > + struct symbol *ds;
> > +
> > + cs = prop_get_symbol(sym_get_choice_prop(sym));
> > + ds = sym_choice_default(cs);
> > + if (sym == ds) {
> > + if ((sym->type == S_BOOLEAN ||
> > + sym->type == S_TRISTATE) &&
> > + sym_get_tristate_value(sym) == yes)
> > + goto next_symbol;
> > + }
> > + }
> > + switch (sym->type) {
>
> This duplicates the switch statement in conf_write(), we should move it
> into a separate function and call it from conf_write() and here.
Will do.
Same goes for some of the code in symbol.c.

I will do it in a few steps to ease review.

>
>
> > + case S_BOOLEAN:
> > + case S_TRISTATE:
> > + switch (sym_get_tristate_value(sym)) {
> > + case no:
> > + fprintf(out, "# CONFIG_%s is not set\n", sym->name);
> > + break;
> > + case yes:
> > + fprintf(out, "CONFIG_%s=y\n", sym->name);
> > + break;
> > + case mod:
> > + fprintf(out, "CONFIG_%s=m\n", sym->name);
> > + break;
> > + }
> > + break;
> > + case S_STRING:
> > + str = sym_get_string_value(sym);
> > + fprintf(out, "CONFIG_%s=\"", sym->name);
> > + while (1) {
> > + l = strcspn(str, "\"\\");
> > + if (l) {
> > + fwrite(str, l, 1, out);
> > + str += l;
> > + }
> > + if (!*str)
> > + break;
> > + fprintf(out, "\\%c", *str++);
> > + }
> > + fputs("\"\n", out);
> > + break;
> > + case S_HEX:
> > + case S_INT:
> > + str = sym_get_string_value(sym);
> > + fprintf(out, "CONFIG_%s=%s\n", sym->name, str);
> > + break;
> > + default:
> > + break;
> > + }
> > +next_symbol:
> > + ;
> > + }
>
> The gotos should be replaced by simple continue statements if there is
> nothing to do after the label.

In an earlier version I jumped to the goto inside a nested for loop.
It is gone so I will use continue as you recommend.

>
> (maybe there are more issues, I didn't look closely into it. But I like
> the approach.)

Likely - I will take a critical look when I respin the patches.
Thanks for the review!

Sam
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Uwe Kleine-König on
Hello,

On Sat, Jul 31, 2010 at 11:35:34PM +0200, Sam Ravnborg wrote:
> savedefconfig will save a minimal config to a file
> named "defconfig".
>
> The config symbols are saved in the same order as
> they appear in the menu structure so it should
> be possible to map them to the relevant menus
> if desired.
>
> The implementation was tested against several minimal
> configs for arm which was created using brute-force.
s/was/were/ I think.

> There was one regression related to default numbers
> which had their valid range further limited by another symbol.
>
> Sample:
>
> config FOO
> int "foo"
> default 4
>
> config BAR
> int "bar"
> range 0 FOO
>
> If FOO is set to 3 then BAR cannot take a value higher than 3.
> But the current implementation will set BAR equal to 4.
>
> This is seldomly used and the final configuration is OK,
> and the fix was non-trivial.
> So it was documented in the code and left as is.
>
> Signed-off-by: Sam Ravnborg <sam(a)ravnborg.org>
Acked-by: Uwe Kleine-K�nig <u.kleine-koenig(a)pengutronix.de>

Thanks
Uwe

--
Pengutronix e.K. | Uwe Kleine-K�nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/