| Message ID | 201304221251.54695.vapier@gentoo.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/22/13 09:51, Mike Frysinger wrote: > the current EXPERT menuconfig is broken by some new options that happen to be > sprinkled into the wrong place. seems like if a node is unprintable, it > should get skipped for menuconfig purposes ? otherwise, this is a constantly > losing battle where someone inserts new Kconfig options and forgets this > nuance, and then it stays broken for a while until someone notices. this > particular bug wrt EXPERT has been linux-3.2. I only noticed a few days ago and then forgot to send a patch for it. > for example, in the General setup section, you currently see: > [ ] Configure standard kernel features (expert users) ---> > [ ] Embedded system > > if you enable EXPERT there, the options get dumped into the same level instead > of being under that menuconfig: > [*] Configure standard kernel features (expert users) ---> > [ ] Sysctl syscall support > [*] Load all symbols for debugging/ksymoops > ... > [ ] Embedded system > > is this feasible in the kconfig code ? A kconfig fix would be very nice. for the patch below: Acked-by: Randy Dunlap <rdunlap@infradead.org> Thanks. > -mike > > --- a/init/Kconfig > +++ b/init/Kconfig > @@ -1177,6 +1177,35 @@ config SYSCTL > config ANON_INODES > bool > > +config HAVE_UID16 > + bool > + > +config SYSCTL_EXCEPTION_TRACE > + bool > + help > + Enable support for /proc/sys/debug/exception-trace. > + > +config SYSCTL_ARCH_UNALIGN_NO_WARN > + bool > + help > + Enable support for /proc/sys/kernel/ignore-unaligned-usertrap > + Allows arch to define/use @no_unaligned_warning to possibly warn > + about unaligned access emulation going on under the hood. > + > +config SYSCTL_ARCH_UNALIGN_ALLOW > + bool > + help > + Enable support for /proc/sys/kernel/unaligned-trap > + Allows arches to define/use @unaligned_enabled to runtime toggle > + the unaligned access emulation. > + see arch/parisc/kernel/unaligned.c for reference > + > +config HOTPLUG > + def_bool y > + > +config HAVE_PCSPKR_PLATFORM > + bool > + > menuconfig EXPERT > bool "Configure standard kernel features (expert users)" > # Unhide debug options, to make the on-by-default options visible > @@ -1187,9 +1216,6 @@ menuconfig EXPERT > environments which can tolerate a "non-standard" kernel. > Only use this if you really know what you are doing. > > -config HAVE_UID16 > - bool > - > config UID16 > bool "Enable 16-bit UID system calls" if EXPERT > depends on HAVE_UID16 > @@ -1214,26 +1240,6 @@ config SYSCTL_SYSCALL > > If unsure say N here. > > -config SYSCTL_EXCEPTION_TRACE > - bool > - help > - Enable support for /proc/sys/debug/exception-trace. > - > -config SYSCTL_ARCH_UNALIGN_NO_WARN > - bool > - help > - Enable support for /proc/sys/kernel/ignore-unaligned-usertrap > - Allows arch to define/use @no_unaligned_warning to possibly warn > - about unaligned access emulation going on under the hood. > - > -config SYSCTL_ARCH_UNALIGN_ALLOW > - bool > - help > - Enable support for /proc/sys/kernel/unaligned-trap > - Allows arches to define/use @unaligned_enabled to runtime toggle > - the unaligned access emulation. > - see arch/parisc/kernel/unaligned.c for reference > - > config KALLSYMS > bool "Load all symbols for debugging/ksymoops" if EXPERT > default y > @@ -1259,9 +1265,6 @@ config KALLSYMS_ALL > > Say N unless you really need all symbols. > > -config HOTPLUG > - def_bool y > - > config PRINTK > default y > bool "Enable support for printk" if EXPERT > @@ -1300,9 +1303,6 @@ config PCSPKR_PLATFORM > This option allows to disable the internal PC-Speaker > support, saving some memory. > > -config HAVE_PCSPKR_PLATFORM > - bool > - > config BASE_FULL > default y > bool "Enable full-sized data structures for core" if EXPERT >
Mike, All, On Mon, Apr 22, 2013 at 12:51:53PM -0400, Mike Frysinger wrote: > the current EXPERT menuconfig is broken by some new options that happen to be > sprinkled into the wrong place. seems like if a node is unprintable, it > should get skipped for menuconfig purposes ? otherwise, this is a constantly > losing battle where someone inserts new Kconfig options and forgets this > nuance, and then it stays broken for a while until someone notices. this > particular bug wrt EXPERT has been linux-3.2. > > for example, in the General setup section, you currently see: > [ ] Configure standard kernel features (expert users) ---> > [ ] Embedded system > > if you enable EXPERT there, the options get dumped into the same level instead > of being under that menuconfig: > [*] Configure standard kernel features (expert users) ---> > [ ] Sysctl syscall support > [*] Load all symbols for debugging/ksymoops > ... > [ ] Embedded system Yes, this is indeed disturbing. Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > is this feasible in the kconfig code ? From Documentation/kbuild/kconfig-language.txt: menuconfig: "menuconfig" <symbol> <config options> This is similar to the simple config entry above, but it also gives a hint to front ends, that all suboptions should be displayed as a separate list of options. This text does not explicit what a "sub-options" is. In fact, it is an option that depends on the menuconfig, and that is not separated from it from a non-dependent option. Thus: menuconfig MENU_A bool "A" config OPT_B bool "B" depends on MENU_A config OPT_C bool "C" depends on MENU_A config OPT_D bool "D" config OPT_E bool "E" depends on MENU_A OPT_B and OPT_C are sub-options of MENU_A, as they depend on it, and directly follow it. OPT_D is not a sub-option of MENU_A, as it does not depend on it. OPT_E, although it depends on MENU_A, is not a sub-option as it does not directly follow it. As far as I remember, this has always been the behaviour of menuconfig. What do you suggest the frontends do in this case, short of re-ordering the options (which I think is not correct) ? On the other hand, we could consider dependent options as candidates for being sub-options until we see the first non-dependent option with a prompt. But I do not think this is sane behaviour, as as soon a new option with a prompt is added in-between sub-options, we again break the relation to the menuconfig. What I always do to ensure consistency is I explicitly use a conditional around the options I consider as sub-options, which forces me to properly order the options in the Kconfig file, eg. (with the same options as above): menuconfig MENU_A bool "A" if MENU_A config OPT_B bool "B" config OPT_C bool "C" config OPT_E bool "E" endif # MENU_A config OPT_D bool "D" Maybe we could change the semantic of menuconfig so that, for an option to be considered as a sub-option, it shall be enclosed in an 'if' block. Another option is to explicitly require and 'endmenu' statement to close the menuconfig. Either way, that would mean we'd have to audit and fix quite a bunch of Kconfig files: $ grep --exclude-dir=.git --exclude-dir=Documentation \ -r -E -e '^menuconfig' . \ |wc -l 215 So, 215 occurences of 'menuconfig' (in 189 Kconfig files). Regards, Yann E. MORIN.
On Monday 22 April 2013 13:08:37 Randy Dunlap wrote: > for the patch below: > Acked-by: Randy Dunlap <rdunlap@infradead.org> ok, i posted it for real to lkml -mike
On Monday 22 April 2013 14:00:46 Yann E. MORIN wrote: > On Mon, Apr 22, 2013 at 12:51:53PM -0400, Mike Frysinger wrote: > > the current EXPERT menuconfig is broken by some new options that happen > > to be sprinkled into the wrong place. seems like if a node is > > unprintable, it should get skipped for menuconfig purposes ? otherwise, > > this is a constantly losing battle where someone inserts new Kconfig > > options and forgets this nuance, and then it stays broken for a while > > until someone notices. this particular bug wrt EXPERT has been > > linux-3.2. > > > > for example, in the General setup section, you currently see: > > [ ] Configure standard kernel features (expert users) ---> > > [ ] Embedded system > > > > if you enable EXPERT there, the options get dumped into the same level > > instead > > > > of being under that menuconfig: > > [*] Configure standard kernel features (expert users) ---> > > [ ] Sysctl syscall support > > [*] Load all symbols for debugging/ksymoops > > ... > > [ ] Embedded system > > Yes, this is indeed disturbing. > > Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > > > is this feasible in the kconfig code ? > > From Documentation/kbuild/kconfig-language.txt: when i said "kconfig code", i was referring to making changes to the parsing source, not the kconfig language. > As far as I remember, this has always been the behaviour of menuconfig. > > What do you suggest the frontends do in this case, short of re-ordering > the options (which I think is not correct) ? i think you missed a critical part of my proposal: seems like *if a node is unprintable*, it should get skipped for menuconfig purposes to modify your example, i'm proposing a change for: menuconfig MENU_A bool "A" config OPT_B bool config OPT_C bool "C" depends on MENU_A OPT_B is not shown to the user (there is no option text), therefore it should automatically get skipped when searching for options to collapse into the menuconfig MENU_A. i do agree that if OPT_B had text, then the existing behavior would be unchanged. -mike
Mike, All, On Mon, Apr 22, 2013 at 02:19:50PM -0400, Mike Frysinger wrote: > On Monday 22 April 2013 14:00:46 Yann E. MORIN wrote: > > As far as I remember, this has always been the behaviour of menuconfig. > > > > What do you suggest the frontends do in this case, short of re-ordering > > the options (which I think is not correct) ? > > i think you missed a critical part of my proposal: > seems like *if a node is unprintable*, it should get skipped for > menuconfig purposes Ah, yes, I missed it. And it is part of the solutions I exposed, too: On the other hand, we could consider dependent options as candidates for being sub-options until we see the first non-dependent option with a prompt. > to modify your example, i'm proposing a change for: > menuconfig MENU_A > bool "A" > > config OPT_B > bool > > config OPT_C > bool "C" > depends on MENU_A > > OPT_B is not shown to the user (there is no option text), therefore it should > automatically get skipped when searching for options to collapse into the > menuconfig MENU_A. > > i do agree that if OPT_B had text, then the existing behavior would be > unchanged. And as I pointed out, this would break as soon as an option with a prompt (which you call 'option text') is added in-between. For example, starting with: menuconfig MENU_A bool "A" config OPT_B bool config OPT_C bool "C" depends on MENU_A And then modifying into: menuconfig MENU_A bool "A" config OPT_D bool "D" config OPT_B bool config OPT_C bool "C" depends on MENU_A (which would be trivivaly noticed here, but can be more complex in real life, as you havee seen in the current init/Kconfig). This would change the semantic of the language anyway, where currently your example disrupts the dependendcy chain. Granted, it looks like a good change, but I think we should not encourage people to be lazy, and we better want them to be careful about what they intend to do, about what they review and ack (myself largely included! :-] ). I'll see if I can coerce the kconfig parser and frontends (yes, frontends are probably impacted, too) to behave the way we discussed in this thread, and see how good it works when confronted to real life. Thank you! Regards, Yann E. MORIN.
On Monday 22 April 2013 16:26:49 Yann E. MORIN wrote: > On Mon, Apr 22, 2013 at 02:19:50PM -0400, Mike Frysinger wrote: > > On Monday 22 April 2013 14:00:46 Yann E. MORIN wrote: > > > As far as I remember, this has always been the behaviour of menuconfig. > > > > > > What do you suggest the frontends do in this case, short of re-ordering > > > the options (which I think is not correct) ? > > > > i think you missed a critical part of my proposal: > > seems like *if a node is unprintable*, it should get skipped for > > menuconfig purposes > > Ah, yes, I missed it. And it is part of the solutions I exposed, too: > > On the other hand, we could consider dependent options as candidates > for being sub-options until we see the first non-dependent option with a > prompt. > > > to modify your example, i'm proposing a change for: > > menuconfig MENU_A > > bool "A" > > > > config OPT_B > > bool > > > > config OPT_C > > bool "C" > > depends on MENU_A > > > > OPT_B is not shown to the user (there is no option text), therefore it > > should automatically get skipped when searching for options to collapse > > into the menuconfig MENU_A. > > > > i do agree that if OPT_B had text, then the existing behavior would be > > unchanged. > > And as I pointed out, this would break as soon as an option with a prompt > (which you call 'option text') is added in-between. For example, starting > with: sure. i'm focusing on the non-displayed case since it wouldn't be a behavioral change, and because that's the only reason that EXPERT broke. there aren't EXPERT & non-EXPERT options there (at least, unintentional). i don't think we should worry about this case because, as you pointed out, we already have kconfig language to enforce the behavior if truly desired. using the EXPERT case as an example, i don't think we want the re-ordering as this is a general knob. we've got specific items which make more sense when grouped elsewhere, and we've got a "dumping bucket" which the current EXPERT menuconfig is designed to hold. for example, checkpoint/restore makes more sense when grouped with cgroups/namespace (and it actually appears *before* the EXPERT menuconfig, so you'd have to build the entire tree first before doing possible re- ordering/processing). another example, the vmcounters/slub debug better fits with the non-expert memory settings (like "choose your allocator"). > This would change the semantic of the language anyway, where currently > your example disrupts the dependendcy chain. Granted, it looks like a > good change, but I think we should not encourage people to be lazy, and > we better want them to be careful about what they intend to do, about > what they review and ack (myself largely included! :-] ). sure, but i think we can segment this. i don't think we should hassle developers with details that make 0 difference to the end result. that's why i think automatically handling options that don't get displayed is OK. but resorting options and possible sticking them in different menus than what the source Kconfig files declare violates KISS. -mike
--- a/init/Kconfig +++ b/init/Kconfig @@ -1177,6 +1177,35 @@ config SYSCTL config ANON_INODES bool +config HAVE_UID16 + bool + +config SYSCTL_EXCEPTION_TRACE + bool + help + Enable support for /proc/sys/debug/exception-trace. + +config SYSCTL_ARCH_UNALIGN_NO_WARN + bool + help + Enable support for /proc/sys/kernel/ignore-unaligned-usertrap + Allows arch to define/use @no_unaligned_warning to possibly warn + about unaligned access emulation going on under the hood. + +config SYSCTL_ARCH_UNALIGN_ALLOW + bool + help + Enable support for /proc/sys/kernel/unaligned-trap + Allows arches to define/use @unaligned_enabled to runtime toggle + the unaligned access emulation. + see arch/parisc/kernel/unaligned.c for reference + +config HOTPLUG + def_bool y + +config HAVE_PCSPKR_PLATFORM + bool + menuconfig EXPERT bool "Configure standard kernel features (expert users)" # Unhide debug options, to make the on-by-default options visible @@ -1187,9 +1216,6 @@ menuconfig EXPERT environments which can tolerate a "non-standard" kernel. Only use this if you really know what you are doing. -config HAVE_UID16 - bool - config UID16 bool "Enable 16-bit UID system calls" if EXPERT depends on HAVE_UID16 @@ -1214,26 +1240,6 @@ config SYSCTL_SYSCALL If unsure say N here. -config SYSCTL_EXCEPTION_TRACE - bool - help - Enable support for /proc/sys/debug/exception-trace. - -config SYSCTL_ARCH_UNALIGN_NO_WARN - bool - help - Enable support for /proc/sys/kernel/ignore-unaligned-usertrap - Allows arch to define/use @no_unaligned_warning to possibly warn - about unaligned access emulation going on under the hood. - -config SYSCTL_ARCH_UNALIGN_ALLOW - bool - help - Enable support for /proc/sys/kernel/unaligned-trap - Allows arches to define/use @unaligned_enabled to runtime toggle - the unaligned access emulation. - see arch/parisc/kernel/unaligned.c for reference - config KALLSYMS bool "Load all symbols for debugging/ksymoops" if EXPERT default y @@ -1259,9 +1265,6 @@ config KALLSYMS_ALL Say N unless you really need all symbols. -config HOTPLUG - def_bool y - config PRINTK default y bool "Enable support for printk" if EXPERT @@ -1300,9 +1303,6 @@ config PCSPKR_PLATFORM This option allows to disable the internal PC-Speaker support, saving some memory. -config HAVE_PCSPKR_PLATFORM - bool - config BASE_FULL default y bool "Enable full-sized data structures for core" if EXPERT