Re: [PATCH v2] kconfig: Add yes2modconfig and mod2yesconfig targets.

[Masahiro Yamada]

On Mon, Dec 16, 2019 at 9:59 PM Tetsuo Handa
<penguin-kernel@xxxxxxxxxxxxxxxxxxx> wrote:
>
> Thank you for reviewing.
>
> On 2019/12/16 20:10, Masahiro Yamada wrote:
> > BTW, I have never contributed to the syzbot bug shooting.
> > So, please teach me if you know this:
> > Is there a a specific reason why the config set for syzbot
> > is close to allyesconfig instead of allmodconfig?
>
> I don't know. But I guess that all-in-one vmlinux file is easier to use
> (e.g. no need to copy .ko files into initramfs nor /lib/modules/ directory
> in the root filesystem image, no need to fetch .ko files when calculating
> locations in the source code from kernel addresses, no need to worry about
> availability of .ko loader program and request_module() dependency).

OK.
I just thought allmodconfig would be more convenient to trim
unrelated code without switching yes/mod back and forth.
Anyway...

> >> @@ -669,6 +684,8 @@ int main(int ac, char **av)
> >>         case listnewconfig:
> >>         case helpnewconfig:
> >>         case syncconfig:
> >> +       case yes2modconfig:
> >> +       case mod2yesconfig:
> >
> > This looks like
> > yes2mod/mod2yesconfig are interactive modes.
> > Why do you need this?
> >
> > I believe yes2mod/mod2yesconfig
> > should work non-interactively.
>
> I worried that simple s/=y$/=m/ or s/=m$/=y/ on tristate config fails to satisfy
> requirement/dependency.

conf_write() calls sym_calc_value() for every symbol
before writing them to the .config file.


> And I assumed that
>
>   /* Update until a loop caused no more changes */
>   do {
>         conf_cnt = 0;
>         check_conf(&rootmenu);
>   } while (conf_cnt);
>
> is the location to make modifications in order to adjust requirement/dependency.

This is not the place to meet requirement/dependency.

This loop requires the user to input his/her preference
for all visible symbols.

oldaskconfig, oldconfig and syncconfig are meant to be
interactive  (it shows a prompt for every new symbol),
that is why they runs this loop.



> But I might be wrong. I just assumed that we should behave as if "make oldconfig"
> after doing simple s/=y$/=m/ or s/=m$/=y/ on tristate config.
> Does some later function automatically adjust requirement/dependency ?

Yes, conf_write().

Thanks.



> If yes,
>
> >> @@ -638,6 +648,11 @@ int main(int ac, char **av)
> >>                 }
> >>         }
> >>
> >> +       if (input_mode == yes2modconfig)
> >> +               conf_rewrite_mod_or_yes(def_y2m);
> >> +       else if (input_mode == mod2yesconfig)
> >> +               conf_rewrite_mod_or_yes(def_m2y);
> >> +
> >
> > For consistency, why not put these lines into the switch statement below?
>
> conf_rewrite_mod_or_yes() should be put into the switch statement.
>
> >> diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
> >> index 3569d2dec37c..6832a04a1aa4 100644
> >> --- a/scripts/kconfig/confdata.c
> >> +++ b/scripts/kconfig/confdata.c
> >> @@ -1362,3 +1362,29 @@ bool conf_set_all_new_symbols(enum conf_def_mode mode)
> >>
> >>         return has_changed;
> >>  }
> >> +
> >> +bool conf_rewrite_mod_or_yes(enum conf_def_mode mode)
> >
> > If you do not use the return value of this function,
> > could you make it into a void function?
>
> OK.
>
> >> +{
> >> +       struct symbol *sym;
> >> +       int i;
> >> +       bool has_changed = false;
> >> +
> >> +       if (mode == def_y2m) {
> >> +               for_all_symbols(i, sym) {
> >> +                       if (sym_get_type(sym) == S_TRISTATE &&
> >> +                           sym->def[S_DEF_USER].tri == yes) {
> >> +                               sym->def[S_DEF_USER].tri = mod;
> >> +                               has_changed = true;
> >
> > sym_add_change_count(1); seems the convention way
> > to inform kconfig of some options being updated.
>
> Then, we can do "sym_add_change_count(1);" instead of "return has_changed;".
>


--
Best Regards
Masahiro Yamada