* [PATCH 01/15] kconfig: Fix defconfig when one choice menu selects options that another choice menu depends on
2013-06-24 18:11 [pull request v2] Pull request for branch yem-kconfig-for-next Yann E. MORIN
@ 2013-06-24 18:11 ` Yann E. MORIN
2013-06-24 18:11 ` [PATCH 02/15] kconfig/lxdialog: Add definitions for mininimum (re)size values Yann E. MORIN
` (14 subsequent siblings)
15 siblings, 0 replies; 36+ messages in thread
From: Yann E. MORIN @ 2013-06-24 18:11 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Michal Marek, Arve Hjønnevåg, Yann E. MORIN
From: Arve Hjønnevåg <arve@android.com>
The defconfig and Kconfig combination below, which is based on 3.10-rc4
Kconfigs, resulted in several options getting set to "m" instead of "y".
defconfig.choice:
---8<---
CONFIG_MODULES=y
CONFIG_USB_ZERO=y
---8<---
Kconfig.choice:
---8<---
menuconfig MODULES
bool "Enable loadable module support"
config CONFIGFS_FS
tristate "Userspace-driven configuration filesystem"
config OCFS2_FS
tristate "OCFS2 file system support"
depends on CONFIGFS_FS
select CRC32
config USB_LIBCOMPOSITE
tristate
select CONFIGFS_FS
choice
tristate "USB Gadget Drivers"
default USB_ETH
config USB_ZERO
tristate "Gadget Zero (DEVELOPMENT)"
select USB_LIBCOMPOSITE
config USB_ETH
tristate "Ethernet Gadget (with CDC Ethernet support)"
select USB_LIBCOMPOSITE
endchoice
config CRC32
tristate "CRC32/CRC32c functions"
default y
choice
prompt "CRC32 implementation"
depends on CRC32
default CRC32_SLICEBY8
config CRC32_SLICEBY8
bool "Slice by 8 bytes"
endchoice
---8<---
$ scripts/kconfig/conf --defconfig=defconfig.choice Kconfig.choice
would result in:
.config:
---8<---
CONFIG_MODULES=y
CONFIG_CONFIGFS_FS=m
CONFIG_USB_LIBCOMPOSITE=m
CONFIG_USB_ZERO=m
CONFIG_CRC32=y
CONFIG_CRC32_SLICEBY8=y
---8<---
when the expected result would be:
.config:
---8<---
CONFIG_MODULES=y
CONFIG_CONFIGFS_FS=y
CONFIG_USB_LIBCOMPOSITE=y
CONFIG_USB_ZERO=y
CONFIG_CRC32=y
CONFIG_CRC32_SLICEBY8=y
---8<---
Signed-off-by: Arve Hjønnevåg <arve@android.com>
[yann.morin.1998@free.fr: add the resulting .config to commit log,
remove unneeded USB_GADGET from the defconfig]
Tested-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
---
scripts/kconfig/confdata.c | 14 ++++++++++----
scripts/kconfig/expr.h | 3 +++
scripts/kconfig/lkc.h | 1 +
scripts/kconfig/symbol.c | 11 +++++++++++
4 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index 43eda40..35e0f16 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -1083,7 +1083,7 @@ static void randomize_choice_values(struct symbol *csym)
csym->flags &= ~(SYMBOL_VALID);
}
-static void set_all_choice_values(struct symbol *csym)
+void set_all_choice_values(struct symbol *csym)
{
struct property *prop;
struct symbol *sym;
@@ -1100,7 +1100,7 @@ static void set_all_choice_values(struct symbol *csym)
}
csym->flags |= SYMBOL_DEF_USER;
/* clear VALID to get value calculated */
- csym->flags &= ~(SYMBOL_VALID);
+ csym->flags &= ~(SYMBOL_VALID | SYMBOL_NEED_SET_CHOICE_VALUES);
}
void conf_set_all_new_symbols(enum conf_def_mode mode)
@@ -1202,6 +1202,14 @@ void conf_set_all_new_symbols(enum conf_def_mode mode)
* selected in a choice block and we set it to yes,
* and the rest to no.
*/
+ if (mode != def_random) {
+ for_all_symbols(i, csym) {
+ if ((sym_is_choice(csym) && !sym_has_value(csym)) ||
+ sym_is_choice_value(csym))
+ csym->flags |= SYMBOL_NEED_SET_CHOICE_VALUES;
+ }
+ }
+
for_all_symbols(i, csym) {
if (sym_has_value(csym) || !sym_is_choice(csym))
continue;
@@ -1209,7 +1217,5 @@ void conf_set_all_new_symbols(enum conf_def_mode mode)
sym_calc_value(csym);
if (mode == def_random)
randomize_choice_values(csym);
- else
- set_all_choice_values(csym);
}
}
diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
index cdd4860..df198a5 100644
--- a/scripts/kconfig/expr.h
+++ b/scripts/kconfig/expr.h
@@ -106,6 +106,9 @@ struct symbol {
#define SYMBOL_DEF3 0x40000 /* symbol.def[S_DEF_3] is valid */
#define SYMBOL_DEF4 0x80000 /* symbol.def[S_DEF_4] is valid */
+/* choice values need to be set before calculating this symbol value */
+#define SYMBOL_NEED_SET_CHOICE_VALUES 0x100000
+
#define SYMBOL_MAXLENGTH 256
#define SYMBOL_HASHSIZE 9973
diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
index f8aee5f..0c8d419 100644
--- a/scripts/kconfig/lkc.h
+++ b/scripts/kconfig/lkc.h
@@ -87,6 +87,7 @@ char *conf_get_default_confname(void);
void sym_set_change_count(int count);
void sym_add_change_count(int count);
void conf_set_all_new_symbols(enum conf_def_mode mode);
+void set_all_choice_values(struct symbol *csym);
struct conf_printer {
void (*print_symbol)(FILE *, struct symbol *, const char *, void *);
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index ecc5aa5..ab8f4c8 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -300,6 +300,14 @@ void sym_calc_value(struct symbol *sym)
if (sym->flags & SYMBOL_VALID)
return;
+
+ if (sym_is_choice_value(sym) &&
+ sym->flags & SYMBOL_NEED_SET_CHOICE_VALUES) {
+ sym->flags &= ~SYMBOL_NEED_SET_CHOICE_VALUES;
+ prop = sym_get_choice_prop(sym);
+ sym_calc_value(prop_get_symbol(prop));
+ }
+
sym->flags |= SYMBOL_VALID;
oldval = sym->curr;
@@ -425,6 +433,9 @@ void sym_calc_value(struct symbol *sym)
if (sym->flags & SYMBOL_AUTO)
sym->flags &= ~SYMBOL_WRITE;
+
+ if (sym->flags & SYMBOL_NEED_SET_CHOICE_VALUES)
+ set_all_choice_values(sym);
}
void sym_clear_all_valid(void)
--
1.8.1.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 02/15] kconfig/lxdialog: Add definitions for mininimum (re)size values
2013-06-24 18:11 [pull request v2] Pull request for branch yem-kconfig-for-next Yann E. MORIN
2013-06-24 18:11 ` [PATCH 01/15] kconfig: Fix defconfig when one choice menu selects options that another choice menu depends on Yann E. MORIN
@ 2013-06-24 18:11 ` Yann E. MORIN
2013-06-24 18:11 ` [PATCH 03/15] kconfig/lxdialog: Use new mininimum resize definitions in conf_choice() Yann E. MORIN
` (13 subsequent siblings)
15 siblings, 0 replies; 36+ messages in thread
From: Yann E. MORIN @ 2013-06-24 18:11 UTC (permalink / raw)
To: linux-kbuild; +Cc: linux-kernel, Michal Marek, Sedat Dilek, Yann E. MORIN
From: Sedat Dilek <sedat.dilek@gmail.com>
Commit c8dc68ad0fbd ("kconfig/lxdialog: support resize") added support
for resizing, but forgot to collect all hardcoded values at one single
place.
Also add a definition for the check for a minimum screen/window size
of 80x19.
[ ChangeLog v3:
* Rename MENU_{HEIGTH,WIDTH}_MIN -> MENUBOX_{HEIGTH,WIDTH}_MIN
ChangeLog v2:
* Rename WIN_{HEIGTH,WIDTH}_MIN -> WINDOW_{HEIGTH,WIDTH}_MIN
* Mention the check for a minimum screen/window size in the changelog
* Add a comment above the block of new definitions ]
Signed-off-by: Sedat Dilek <sedat.dilek@gmail.com>
Acked-by: Wang YanQing <udknight@gmail.com>
Tested-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
scripts/kconfig/lxdialog/checklist.c | 4 ++--
scripts/kconfig/lxdialog/dialog.h | 14 ++++++++++++++
scripts/kconfig/lxdialog/inputbox.c | 4 ++--
| 2 +-
scripts/kconfig/lxdialog/textbox.c | 2 +-
scripts/kconfig/lxdialog/util.c | 2 +-
scripts/kconfig/lxdialog/yesno.c | 4 ++--
7 files changed, 23 insertions(+), 9 deletions(-)
diff --git a/scripts/kconfig/lxdialog/checklist.c b/scripts/kconfig/lxdialog/checklist.c
index a2eb80f..3034057 100644
--- a/scripts/kconfig/lxdialog/checklist.c
+++ b/scripts/kconfig/lxdialog/checklist.c
@@ -132,9 +132,9 @@ int dialog_checklist(const char *title, const char *prompt, int height,
}
do_resize:
- if (getmaxy(stdscr) < (height + 6))
+ if (getmaxy(stdscr) < (height + CHECKLIST_HEIGTH_MIN))
return -ERRDISPLAYTOOSMALL;
- if (getmaxx(stdscr) < (width + 6))
+ if (getmaxx(stdscr) < (width + CHECKLIST_WIDTH_MIN))
return -ERRDISPLAYTOOSMALL;
max_choice = MIN(list_height, item_count());
diff --git a/scripts/kconfig/lxdialog/dialog.h b/scripts/kconfig/lxdialog/dialog.h
index 10993370..b4343d3 100644
--- a/scripts/kconfig/lxdialog/dialog.h
+++ b/scripts/kconfig/lxdialog/dialog.h
@@ -200,6 +200,20 @@ int item_is_tag(char tag);
int on_key_esc(WINDOW *win);
int on_key_resize(void);
+/* minimum (re)size values */
+#define CHECKLIST_HEIGTH_MIN 6 /* For dialog_checklist() */
+#define CHECKLIST_WIDTH_MIN 6
+#define INPUTBOX_HEIGTH_MIN 2 /* For dialog_inputbox() */
+#define INPUTBOX_WIDTH_MIN 2
+#define MENUBOX_HEIGTH_MIN 15 /* For dialog_menu() */
+#define MENUBOX_WIDTH_MIN 65
+#define TEXTBOX_HEIGTH_MIN 8 /* For dialog_textbox() */
+#define TEXTBOX_WIDTH_MIN 8
+#define YESNO_HEIGTH_MIN 4 /* For dialog_yesno() */
+#define YESNO_WIDTH_MIN 4
+#define WINDOW_HEIGTH_MIN 19 /* For init_dialog() */
+#define WINDOW_WIDTH_MIN 80
+
int init_dialog(const char *backtitle);
void set_dialog_backtitle(const char *backtitle);
void set_dialog_subtitles(struct subtitle_list *subtitles);
diff --git a/scripts/kconfig/lxdialog/inputbox.c b/scripts/kconfig/lxdialog/inputbox.c
index 21404a0..7b01add 100644
--- a/scripts/kconfig/lxdialog/inputbox.c
+++ b/scripts/kconfig/lxdialog/inputbox.c
@@ -56,9 +56,9 @@ int dialog_inputbox(const char *title, const char *prompt, int height, int width
strcpy(instr, init);
do_resize:
- if (getmaxy(stdscr) <= (height - 2))
+ if (getmaxy(stdscr) <= (height - INPUTBOX_HEIGTH_MIN))
return -ERRDISPLAYTOOSMALL;
- if (getmaxx(stdscr) <= (width - 2))
+ if (getmaxx(stdscr) <= (width - INPUTBOX_WIDTH_MIN))
return -ERRDISPLAYTOOSMALL;
/* center dialog box on screen */
--git a/scripts/kconfig/lxdialog/menubox.c b/scripts/kconfig/lxdialog/menubox.c
index 48d382e..00d2841 100644
--- a/scripts/kconfig/lxdialog/menubox.c
+++ b/scripts/kconfig/lxdialog/menubox.c
@@ -193,7 +193,7 @@ int dialog_menu(const char *title, const char *prompt,
do_resize:
height = getmaxy(stdscr);
width = getmaxx(stdscr);
- if (height < 15 || width < 65)
+ if (height < MENUBOX_HEIGTH_MIN || width < MENUBOX_WIDTH_MIN)
return -ERRDISPLAYTOOSMALL;
height -= 4;
diff --git a/scripts/kconfig/lxdialog/textbox.c b/scripts/kconfig/lxdialog/textbox.c
index a48bb93..907cdcb 100644
--- a/scripts/kconfig/lxdialog/textbox.c
+++ b/scripts/kconfig/lxdialog/textbox.c
@@ -80,7 +80,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
do_resize:
getmaxyx(stdscr, height, width);
- if (height < 8 || width < 8)
+ if (height < TEXTBOX_HEIGTH_MIN || width < TEXTBOX_WIDTH_MIN)
return -ERRDISPLAYTOOSMALL;
if (initial_height != 0)
height = initial_height;
diff --git a/scripts/kconfig/lxdialog/util.c b/scripts/kconfig/lxdialog/util.c
index a0e97c2..78fe4e9 100644
--- a/scripts/kconfig/lxdialog/util.c
+++ b/scripts/kconfig/lxdialog/util.c
@@ -317,7 +317,7 @@ int init_dialog(const char *backtitle)
getyx(stdscr, saved_y, saved_x);
getmaxyx(stdscr, height, width);
- if (height < 19 || width < 80) {
+ if (height < WINDOW_HEIGTH_MIN || width < WINDOW_WIDTH_MIN) {
endwin();
return -ERRDISPLAYTOOSMALL;
}
diff --git a/scripts/kconfig/lxdialog/yesno.c b/scripts/kconfig/lxdialog/yesno.c
index 4e6e809..abb0c39 100644
--- a/scripts/kconfig/lxdialog/yesno.c
+++ b/scripts/kconfig/lxdialog/yesno.c
@@ -45,9 +45,9 @@ int dialog_yesno(const char *title, const char *prompt, int height, int width)
WINDOW *dialog;
do_resize:
- if (getmaxy(stdscr) < (height + 4))
+ if (getmaxy(stdscr) < (height + YESNO_HEIGTH_MIN))
return -ERRDISPLAYTOOSMALL;
- if (getmaxx(stdscr) < (width + 4))
+ if (getmaxx(stdscr) < (width + YESNO_WIDTH_MIN))
return -ERRDISPLAYTOOSMALL;
/* center dialog box on screen */
--
1.8.1.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 03/15] kconfig/lxdialog: Use new mininimum resize definitions in conf_choice()
2013-06-24 18:11 [pull request v2] Pull request for branch yem-kconfig-for-next Yann E. MORIN
2013-06-24 18:11 ` [PATCH 01/15] kconfig: Fix defconfig when one choice menu selects options that another choice menu depends on Yann E. MORIN
2013-06-24 18:11 ` [PATCH 02/15] kconfig/lxdialog: Add definitions for mininimum (re)size values Yann E. MORIN
@ 2013-06-24 18:11 ` Yann E. MORIN
2013-06-24 18:11 ` [PATCH 04/15] kconfig/lxdialog: handle newline characters in print_autowrap() Yann E. MORIN
` (12 subsequent siblings)
15 siblings, 0 replies; 36+ messages in thread
From: Yann E. MORIN @ 2013-06-24 18:11 UTC (permalink / raw)
To: linux-kbuild; +Cc: linux-kernel, Michal Marek, Sedat Dilek, Yann E. MORIN
From: Sedat Dilek <sedat.dilek@gmail.com>
This is a cleanup which uses the proper (new) definitions and does
not change current behaviour.
Signed-off-by: Sedat Dilek <sedat.dilek@gmail.com>
Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Tested-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
Yann had some more ideas on improvements:
"What would be nice is an improvement that scales the choice window to
the number of entries in the choice. If there are a lot of choice
entries, then the choice popup grows in height (but does not overflow
the screen of course). So, instead of seeing only 6 entries, we'd see
as much as possible in the current screen.
Ditto for the width: the popup adapts to the longest prompt (but does
not overflow the screen either, of course), so prompts are not
truncated."
NOTE: This patch requires [1].
[1] http://marc.info/?l=linux-kbuild&m=137128726917166&w=2
---
scripts/kconfig/mconf.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c
index 387dc8d..2396c5b 100644
--- a/scripts/kconfig/mconf.c
+++ b/scripts/kconfig/mconf.c
@@ -825,7 +825,9 @@ static void conf_choice(struct menu *menu)
dialog_clear();
res = dialog_checklist(prompt ? _(prompt) : _("Main Menu"),
_(radiolist_instructions),
- 15, 70, 6);
+ MENUBOX_HEIGTH_MIN,
+ MENUBOX_WIDTH_MIN,
+ CHECKLIST_HEIGTH_MIN);
selected = item_activate_selected();
switch (res) {
case 0:
--
1.8.1.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 04/15] kconfig/lxdialog: handle newline characters in print_autowrap()
2013-06-24 18:11 [pull request v2] Pull request for branch yem-kconfig-for-next Yann E. MORIN
` (2 preceding siblings ...)
2013-06-24 18:11 ` [PATCH 03/15] kconfig/lxdialog: Use new mininimum resize definitions in conf_choice() Yann E. MORIN
@ 2013-06-24 18:11 ` Yann E. MORIN
2013-06-24 18:11 ` [PATCH 05/15] mconf: use function calls instead of ncurses' variables LINES and COLS Yann E. MORIN
` (11 subsequent siblings)
15 siblings, 0 replies; 36+ messages in thread
From: Yann E. MORIN @ 2013-06-24 18:11 UTC (permalink / raw)
To: linux-kbuild; +Cc: linux-kernel, Michal Marek, Dirk Gouders, Yann E. MORIN
From: Dirk Gouders <dirk@gouders.net>
When exiting menuconfig with unsaved changes, a dialog like
the following is shown:
Do you wish to save your new configuration ? <ESC><ESC>
to continue.
The author of the dialog text specified a newline after the '?',
and probably expected it to be processed, so let print_autowrap()
handle newlines propperly.
Also, reword that dialog's second phrase with a real sentence.
Signed-off-by: Dirk Gouders <dirk@gouders.net>
Tested-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
[yann.morin.1998@free.fr: very slightly tweak the commit message]
Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
---
scripts/kconfig/lxdialog/util.c | 31 +++++++++++++++++--------------
scripts/kconfig/mconf.c | 4 ++--
2 files changed, 19 insertions(+), 16 deletions(-)
diff --git a/scripts/kconfig/lxdialog/util.c b/scripts/kconfig/lxdialog/util.c
index 78fe4e9..0fa567e 100644
--- a/scripts/kconfig/lxdialog/util.c
+++ b/scripts/kconfig/lxdialog/util.c
@@ -371,27 +371,19 @@ void print_title(WINDOW *dialog, const char *title, int width)
/*
* Print a string of text in a window, automatically wrap around to the
* next line if the string is too long to fit on one line. Newline
- * characters '\n' are replaced by spaces. We start on a new line
+ * characters '\n' are propperly processed. We start on a new line
* if there is no room for at least 4 nonblanks following a double-space.
*/
void print_autowrap(WINDOW * win, const char *prompt, int width, int y, int x)
{
int newl, cur_x, cur_y;
- int i, prompt_len, room, wlen;
- char tempstr[MAX_LEN + 1], *word, *sp, *sp2;
+ int prompt_len, room, wlen;
+ char tempstr[MAX_LEN + 1], *word, *sp, *sp2, *newline_separator = 0;
strcpy(tempstr, prompt);
prompt_len = strlen(tempstr);
- /*
- * Remove newlines
- */
- for (i = 0; i < prompt_len; i++) {
- if (tempstr[i] == '\n')
- tempstr[i] = ' ';
- }
-
if (prompt_len <= width - x * 2) { /* If prompt is short */
wmove(win, y, (width - prompt_len) / 2);
waddstr(win, tempstr);
@@ -401,7 +393,10 @@ void print_autowrap(WINDOW * win, const char *prompt, int width, int y, int x)
newl = 1;
word = tempstr;
while (word && *word) {
- sp = strchr(word, ' ');
+ sp = strpbrk(word, "\n ");
+ if (sp && *sp == '\n')
+ newline_separator = sp;
+
if (sp)
*sp++ = 0;
@@ -413,7 +408,7 @@ void print_autowrap(WINDOW * win, const char *prompt, int width, int y, int x)
if (wlen > room ||
(newl && wlen < 4 && sp
&& wlen + 1 + strlen(sp) > room
- && (!(sp2 = strchr(sp, ' '))
+ && (!(sp2 = strpbrk(sp, "\n "))
|| wlen + 1 + (sp2 - sp) > room))) {
cur_y++;
cur_x = x;
@@ -421,7 +416,15 @@ void print_autowrap(WINDOW * win, const char *prompt, int width, int y, int x)
wmove(win, cur_y, cur_x);
waddstr(win, word);
getyx(win, cur_y, cur_x);
- cur_x++;
+
+ /* Move to the next line if the word separator was a newline */
+ if (newline_separator) {
+ cur_y++;
+ cur_x = x;
+ newline_separator = 0;
+ } else
+ cur_x++;
+
if (sp && *sp == ' ') {
cur_x++; /* double space */
while (*++sp == ' ') ;
diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c
index 2396c5b..cb8cf4a 100644
--- a/scripts/kconfig/mconf.c
+++ b/scripts/kconfig/mconf.c
@@ -958,8 +958,8 @@ static int handle_exit(void)
dialog_clear();
if (conf_get_changed())
res = dialog_yesno(NULL,
- _("Do you wish to save your new configuration ?\n"
- "<ESC><ESC> to continue."),
+ _("Do you wish to save your new configuration?\n"
+ "(Press <ESC><ESC> to continue kernel configuration.)"),
6, 60);
else
res = -1;
--
1.8.1.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 05/15] mconf: use function calls instead of ncurses' variables LINES and COLS
2013-06-24 18:11 [pull request v2] Pull request for branch yem-kconfig-for-next Yann E. MORIN
` (3 preceding siblings ...)
2013-06-24 18:11 ` [PATCH 04/15] kconfig/lxdialog: handle newline characters in print_autowrap() Yann E. MORIN
@ 2013-06-24 18:11 ` Yann E. MORIN
2013-06-24 18:11 ` [PATCH 06/15] nconf: " Yann E. MORIN
` (10 subsequent siblings)
15 siblings, 0 replies; 36+ messages in thread
From: Yann E. MORIN @ 2013-06-24 18:11 UTC (permalink / raw)
To: linux-kbuild; +Cc: linux-kernel, Michal Marek, Dirk Gouders, Yann E. MORIN
From: Dirk Gouders <dirk@gouders.net>
According to the documentation [1], LINES and COLS are initialized by
initscr(); it does not say anything about the behavior when windows are
resized.
Do not rely on the current implementation of ncurses that updates
these variables on resize, but use the propper function calls to get
window dimensions.
init_dialog() could make use of the variables, but for the sake of
consistency we do not change it's current use of the macro getmaxyx().
[1] ncurses(3X)
Signed-off-by: Dirk Gouders <dirk@gouders.net>
Tested-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
---
scripts/kconfig/lxdialog/checklist.c | 4 ++--
scripts/kconfig/lxdialog/inputbox.c | 4 ++--
| 4 ++--
scripts/kconfig/lxdialog/textbox.c | 4 ++--
scripts/kconfig/lxdialog/util.c | 13 +++++++++----
scripts/kconfig/lxdialog/yesno.c | 4 ++--
6 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/scripts/kconfig/lxdialog/checklist.c b/scripts/kconfig/lxdialog/checklist.c
index 3034057..3b15c08 100644
--- a/scripts/kconfig/lxdialog/checklist.c
+++ b/scripts/kconfig/lxdialog/checklist.c
@@ -140,8 +140,8 @@ do_resize:
max_choice = MIN(list_height, item_count());
/* center dialog box on screen */
- x = (COLS - width) / 2;
- y = (LINES - height) / 2;
+ x = (getmaxx(stdscr) - width) / 2;
+ y = (getmaxy(stdscr) - height) / 2;
draw_shadow(stdscr, y, x, height, width);
diff --git a/scripts/kconfig/lxdialog/inputbox.c b/scripts/kconfig/lxdialog/inputbox.c
index 7b01add..447a582 100644
--- a/scripts/kconfig/lxdialog/inputbox.c
+++ b/scripts/kconfig/lxdialog/inputbox.c
@@ -62,8 +62,8 @@ do_resize:
return -ERRDISPLAYTOOSMALL;
/* center dialog box on screen */
- x = (COLS - width) / 2;
- y = (LINES - height) / 2;
+ x = (getmaxx(stdscr) - width) / 2;
+ y = (getmaxy(stdscr) - height) / 2;
draw_shadow(stdscr, y, x, height, width);
--git a/scripts/kconfig/lxdialog/menubox.c b/scripts/kconfig/lxdialog/menubox.c
index 00d2841..92b89a6 100644
--- a/scripts/kconfig/lxdialog/menubox.c
+++ b/scripts/kconfig/lxdialog/menubox.c
@@ -203,8 +203,8 @@ do_resize:
max_choice = MIN(menu_height, item_count());
/* center dialog box on screen */
- x = (COLS - width) / 2;
- y = (LINES - height) / 2;
+ x = (getmaxx(stdscr) - width) / 2;
+ y = (getmaxy(stdscr) - height) / 2;
draw_shadow(stdscr, y, x, height, width);
diff --git a/scripts/kconfig/lxdialog/textbox.c b/scripts/kconfig/lxdialog/textbox.c
index 907cdcb..1773319 100644
--- a/scripts/kconfig/lxdialog/textbox.c
+++ b/scripts/kconfig/lxdialog/textbox.c
@@ -98,8 +98,8 @@ do_resize:
width = 0;
/* center dialog box on screen */
- x = (COLS - width) / 2;
- y = (LINES - height) / 2;
+ x = (getmaxx(stdscr) - width) / 2;
+ y = (getmaxy(stdscr) - height) / 2;
draw_shadow(stdscr, y, x, height, width);
diff --git a/scripts/kconfig/lxdialog/util.c b/scripts/kconfig/lxdialog/util.c
index 0fa567e..58a8289 100644
--- a/scripts/kconfig/lxdialog/util.c
+++ b/scripts/kconfig/lxdialog/util.c
@@ -254,7 +254,12 @@ void attr_clear(WINDOW * win, int height, int width, chtype attr)
void dialog_clear(void)
{
- attr_clear(stdscr, LINES, COLS, dlg.screen.atr);
+ int lines, columns;
+
+ lines = getmaxy(stdscr);
+ columns = getmaxx(stdscr);
+
+ attr_clear(stdscr, lines, columns, dlg.screen.atr);
/* Display background title if it exists ... - SLH */
if (dlg.backtitle != NULL) {
int i, len = 0, skip = 0;
@@ -269,10 +274,10 @@ void dialog_clear(void)
}
wmove(stdscr, 1, 1);
- if (len > COLS - 2) {
+ if (len > columns - 2) {
const char *ellipsis = "[...] ";
waddstr(stdscr, ellipsis);
- skip = len - (COLS - 2 - strlen(ellipsis));
+ skip = len - (columns - 2 - strlen(ellipsis));
}
for (pos = dlg.subtitles; pos != NULL; pos = pos->next) {
@@ -298,7 +303,7 @@ void dialog_clear(void)
skip--;
}
- for (i = len + 1; i < COLS - 1; i++)
+ for (i = len + 1; i < columns - 1; i++)
waddch(stdscr, ACS_HLINE);
}
wnoutrefresh(stdscr);
diff --git a/scripts/kconfig/lxdialog/yesno.c b/scripts/kconfig/lxdialog/yesno.c
index abb0c39..676fb2f 100644
--- a/scripts/kconfig/lxdialog/yesno.c
+++ b/scripts/kconfig/lxdialog/yesno.c
@@ -51,8 +51,8 @@ do_resize:
return -ERRDISPLAYTOOSMALL;
/* center dialog box on screen */
- x = (COLS - width) / 2;
- y = (LINES - height) / 2;
+ x = (getmaxx(stdscr) - width) / 2;
+ y = (getmaxy(stdscr) - height) / 2;
draw_shadow(stdscr, y, x, height, width);
--
1.8.1.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 06/15] nconf: use function calls instead of ncurses' variables LINES and COLS
2013-06-24 18:11 [pull request v2] Pull request for branch yem-kconfig-for-next Yann E. MORIN
` (4 preceding siblings ...)
2013-06-24 18:11 ` [PATCH 05/15] mconf: use function calls instead of ncurses' variables LINES and COLS Yann E. MORIN
@ 2013-06-24 18:11 ` Yann E. MORIN
2013-06-24 18:11 ` [PATCH 07/15] mconf/nconf: mark empty menus/menuconfigs different from non-empty ones Yann E. MORIN
` (9 subsequent siblings)
15 siblings, 0 replies; 36+ messages in thread
From: Yann E. MORIN @ 2013-06-24 18:11 UTC (permalink / raw)
To: linux-kbuild; +Cc: linux-kernel, Michal Marek, Dirk Gouders, Yann E. MORIN
From: Dirk Gouders <dirk@gouders.net>
According to the documentation [1], LINES and COLS are initialized by
initscr(); it does not say anything about the behavior when windows are
resized.
Do not rely on the current implementation of ncurses that updates
these variables on resize, but use the propper function calls or macros
to get window dimensions.
The use of the variables in main() was OK, but for the sake of
consistency it was modified to use the macro getmaxyx().
[1] ncurses(3X)
Signed-off-by: Dirk Gouders <dirk@gouders.net>
Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
[yann.morin.1998@free.fr: declare 'lines' and 'columns' on a single line]
Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
---
scripts/kconfig/nconf.c | 21 ++++++++++++++-------
scripts/kconfig/nconf.gui.c | 20 +++++++++++---------
2 files changed, 25 insertions(+), 16 deletions(-)
diff --git a/scripts/kconfig/nconf.c b/scripts/kconfig/nconf.c
index dbf31ed..aac85d3 100644
--- a/scripts/kconfig/nconf.c
+++ b/scripts/kconfig/nconf.c
@@ -365,15 +365,16 @@ static void print_function_line(void)
int i;
int offset = 1;
const int skip = 1;
+ int lines = getmaxy(stdscr);
for (i = 0; i < function_keys_num; i++) {
(void) wattrset(main_window, attributes[FUNCTION_HIGHLIGHT]);
- mvwprintw(main_window, LINES-3, offset,
+ mvwprintw(main_window, lines-3, offset,
"%s",
function_keys[i].key_str);
(void) wattrset(main_window, attributes[FUNCTION_TEXT]);
offset += strlen(function_keys[i].key_str);
- mvwprintw(main_window, LINES-3,
+ mvwprintw(main_window, lines-3,
offset, "%s",
function_keys[i].func);
offset += strlen(function_keys[i].func) + skip;
@@ -954,7 +955,7 @@ static void show_menu(const char *prompt, const char *instructions,
clear();
(void) wattrset(main_window, attributes[NORMAL]);
- print_in_middle(stdscr, 1, 0, COLS,
+ print_in_middle(stdscr, 1, 0, getmaxx(stdscr),
menu_backtitle,
attributes[MAIN_HEADING]);
@@ -1455,14 +1456,18 @@ static void conf_save(void)
void setup_windows(void)
{
+ int lines, columns;
+
+ getmaxyx(stdscr, lines, columns);
+
if (main_window != NULL)
delwin(main_window);
/* set up the menu and menu window */
- main_window = newwin(LINES-2, COLS-2, 2, 1);
+ main_window = newwin(lines-2, columns-2, 2, 1);
keypad(main_window, TRUE);
- mwin_max_lines = LINES-7;
- mwin_max_cols = COLS-6;
+ mwin_max_lines = lines-7;
+ mwin_max_cols = columns-6;
/* panels order is from bottom to top */
new_panel(main_window);
@@ -1470,6 +1475,7 @@ void setup_windows(void)
int main(int ac, char **av)
{
+ int lines, columns;
char *mode;
setlocale(LC_ALL, "");
@@ -1495,7 +1501,8 @@ int main(int ac, char **av)
keypad(stdscr, TRUE);
curs_set(0);
- if (COLS < 75 || LINES < 20) {
+ getmaxyx(stdscr, lines, columns);
+ if (columns < 75 || lines < 20) {
endwin();
printf("Your terminal should have at "
"least 20 lines and 75 columns\n");
diff --git a/scripts/kconfig/nconf.gui.c b/scripts/kconfig/nconf.gui.c
index 9f8c44e..8275f0e5 100644
--- a/scripts/kconfig/nconf.gui.c
+++ b/scripts/kconfig/nconf.gui.c
@@ -276,8 +276,8 @@ int btn_dialog(WINDOW *main_window, const char *msg, int btn_num, ...)
total_width = max(msg_width, btns_width);
/* place dialog in middle of screen */
- y = (LINES-(msg_lines+4))/2;
- x = (COLS-(total_width+4))/2;
+ y = (getmaxy(stdscr)-(msg_lines+4))/2;
+ x = (getmaxx(stdscr)-(total_width+4))/2;
/* create the windows */
@@ -387,8 +387,8 @@ int dialog_inputbox(WINDOW *main_window,
prompt_width = max(prompt_width, strlen(title));
/* place dialog in middle of screen */
- y = (LINES-(prompt_lines+4))/2;
- x = (COLS-(prompt_width+4))/2;
+ y = (getmaxy(stdscr)-(prompt_lines+4))/2;
+ x = (getmaxx(stdscr)-(prompt_width+4))/2;
strncpy(result, init, *result_len);
@@ -545,7 +545,7 @@ void show_scroll_win(WINDOW *main_window,
{
int res;
int total_lines = get_line_no(text);
- int x, y;
+ int x, y, lines, columns;
int start_x = 0, start_y = 0;
int text_lines = 0, text_cols = 0;
int total_cols = 0;
@@ -556,6 +556,8 @@ void show_scroll_win(WINDOW *main_window,
WINDOW *pad;
PANEL *panel;
+ getmaxyx(stdscr, lines, columns);
+
/* find the widest line of msg: */
total_lines = get_line_no(text);
for (i = 0; i < total_lines; i++) {
@@ -569,14 +571,14 @@ void show_scroll_win(WINDOW *main_window,
(void) wattrset(pad, attributes[SCROLLWIN_TEXT]);
fill_window(pad, text);
- win_lines = min(total_lines+4, LINES-2);
- win_cols = min(total_cols+2, COLS-2);
+ win_lines = min(total_lines+4, lines-2);
+ win_cols = min(total_cols+2, columns-2);
text_lines = max(win_lines-4, 0);
text_cols = max(win_cols-2, 0);
/* place window in middle of screen */
- y = (LINES-win_lines)/2;
- x = (COLS-win_cols)/2;
+ y = (lines-win_lines)/2;
+ x = (columns-win_cols)/2;
win = newwin(win_lines, win_cols, y, x);
keypad(win, TRUE);
--
1.8.1.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 07/15] mconf/nconf: mark empty menus/menuconfigs different from non-empty ones
2013-06-24 18:11 [pull request v2] Pull request for branch yem-kconfig-for-next Yann E. MORIN
` (5 preceding siblings ...)
2013-06-24 18:11 ` [PATCH 06/15] nconf: " Yann E. MORIN
@ 2013-06-24 18:11 ` Yann E. MORIN
2013-06-24 18:11 ` [PATCH 08/15] scripts/config: replace hard-coded script name by a dynamic value Yann E. MORIN
` (8 subsequent siblings)
15 siblings, 0 replies; 36+ messages in thread
From: Yann E. MORIN @ 2013-06-24 18:11 UTC (permalink / raw)
To: linux-kbuild; +Cc: linux-kernel, Michal Marek, Dirk Gouders, Yann E. MORIN
From: Dirk Gouders <dirk@gouders.net>
Submenus are sometimes empty and it would be nice if there is
something that notifies us that we should not expect any content
_before_ we enter a submenu.
A new function menu_is_empty() was introduced and empty menus and
menuconfigs are now marked by "----" as opposed to non-empty ones that
are marked by "--->".
This scheme was suggested by "Yann E. MORIN" <yann.morin.1998@free.fr>.
Signed-off-by: Dirk Gouders <dirk@gouders.net>
Tested-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
scripts/kconfig/lkc_proto.h | 1 +
scripts/kconfig/mconf.c | 11 ++++++-----
| 16 ++++++++++++++++
scripts/kconfig/nconf.c | 16 ++++++++--------
4 files changed, 31 insertions(+), 13 deletions(-)
diff --git a/scripts/kconfig/lkc_proto.h b/scripts/kconfig/lkc_proto.h
index ef1a738..ecdb965 100644
--- a/scripts/kconfig/lkc_proto.h
+++ b/scripts/kconfig/lkc_proto.h
@@ -14,6 +14,7 @@ P(conf_set_message_callback, void,(void (*fn)(const char *fmt, va_list ap)));
/* menu.c */
P(rootmenu,struct menu,);
+P(menu_is_empty, bool, (struct menu *menu));
P(menu_is_visible, bool, (struct menu *menu));
P(menu_has_prompt, bool, (struct menu *menu));
P(menu_get_prompt,const char *,(struct menu *menu));
diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c
index cb8cf4a..6ee4aae 100644
--- a/scripts/kconfig/mconf.c
+++ b/scripts/kconfig/mconf.c
@@ -48,7 +48,7 @@ static const char mconf_readme[] = N_(
"----------\n"
"o Use the Up/Down arrow keys (cursor keys) to highlight the item\n"
" you wish to change or submenu wish to select and press <Enter>.\n"
-" Submenus are designated by \"--->\".\n"
+" Submenus are designated by \"--->\", empty ones by \"----\".\n"
"\n"
" Shortcut: Press the option's highlighted letter (hotkey).\n"
" Pressing a hotkey more than once will sequence\n"
@@ -176,7 +176,7 @@ static const char mconf_readme[] = N_(
"\n"),
menu_instructions[] = N_(
"Arrow keys navigate the menu. "
- "<Enter> selects submenus --->. "
+ "<Enter> selects submenus ---> (or empty submenus ----). "
"Highlighted letters are hotkeys. "
"Pressing <Y> includes, <N> excludes, <M> modularizes features. "
"Press <Esc><Esc> to exit, <?> for Help, </> for Search. "
@@ -498,8 +498,9 @@ static void build_conf(struct menu *menu)
menu->data ? "-->" : "++>",
indent + 1, ' ', prompt);
} else
- item_make(" %*c%s --->", indent + 1, ' ', prompt);
-
+ item_make(" %*c%s %s",
+ indent + 1, ' ', prompt,
+ menu_is_empty(menu) ? "----" : "--->");
item_set_tag('m');
item_set_data(menu);
if (single_menu_mode && menu->data)
@@ -630,7 +631,7 @@ static void build_conf(struct menu *menu)
(sym_has_value(sym) || !sym_is_changable(sym)) ?
"" : _(" (NEW)"));
if (menu->prompt->type == P_MENU) {
- item_add_str(" --->");
+ item_add_str(" %s", menu_is_empty(menu) ? "----" : "--->");
return;
}
}
--git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index b5c7d90..6d11c8f 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -430,6 +430,22 @@ bool menu_has_prompt(struct menu *menu)
return true;
}
+/*
+ * Determine if a menu is empty.
+ * A menu is considered empty if it contains no or only
+ * invisible entries.
+ */
+bool menu_is_empty(struct menu *menu)
+{
+ struct menu *child;
+
+ for (child = menu->list; child; child = child->next) {
+ if (menu_is_visible(child))
+ return(false);
+ }
+ return(true);
+}
+
bool menu_is_visible(struct menu *menu)
{
struct menu *child;
diff --git a/scripts/kconfig/nconf.c b/scripts/kconfig/nconf.c
index aac85d3..0183153 100644
--- a/scripts/kconfig/nconf.c
+++ b/scripts/kconfig/nconf.c
@@ -45,8 +45,8 @@ static const char nconf_global_help[] = N_(
"<n> to remove it. You may press the <Space> key to cycle through the\n"
"available options.\n"
"\n"
-"A trailing \"--->\" designates a submenu.\n"
-"\n"
+"A trailing \"--->\" designates a submenu, a trailing \"----\" an\n"
+"empty submenu.\n"
"\n"
"Menu navigation keys\n"
"----------------------------------------------------------------------\n"
@@ -131,7 +131,7 @@ static const char nconf_global_help[] = N_(
"\n"),
menu_no_f_instructions[] = N_(
"Legend: [*] built-in [ ] excluded <M> module < > module capable.\n"
-"Submenus are designated by a trailing \"--->\".\n"
+"Submenus are designated by a trailing \"--->\", empty ones by \"----\".\n"
"\n"
"Use the following keys to navigate the menus:\n"
"Move up or down with <Up> and <Down>.\n"
@@ -148,7 +148,7 @@ menu_no_f_instructions[] = N_(
"For help related to the current menu entry press <?> or <h>.\n"),
menu_instructions[] = N_(
"Legend: [*] built-in [ ] excluded <M> module < > module capable.\n"
-"Submenus are designated by a trailing \"--->\".\n"
+"Submenus are designated by a trailing \"--->\", empty ones by \"----\".\n"
"\n"
"Use the following keys to navigate the menus:\n"
"Move up or down with <Up> or <Down>.\n"
@@ -760,9 +760,9 @@ static void build_conf(struct menu *menu)
indent + 1, ' ', prompt);
} else
item_make(menu, 'm',
- " %*c%s --->",
- indent + 1,
- ' ', prompt);
+ " %*c%s %s",
+ indent + 1, ' ', prompt,
+ menu_is_empty(menu) ? "----" : "--->");
if (single_menu_mode && menu->data)
goto conf_childs;
@@ -904,7 +904,7 @@ static void build_conf(struct menu *menu)
(sym_has_value(sym) || !sym_is_changable(sym)) ?
"" : _(" (NEW)"));
if (menu->prompt && menu->prompt->type == P_MENU) {
- item_add_str(" --->");
+ item_add_str(" %s", menu_is_empty(menu) ? "----" : "--->");
return;
}
}
--
1.8.1.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 08/15] scripts/config: replace hard-coded script name by a dynamic value
2013-06-24 18:11 [pull request v2] Pull request for branch yem-kconfig-for-next Yann E. MORIN
` (6 preceding siblings ...)
2013-06-24 18:11 ` [PATCH 07/15] mconf/nconf: mark empty menus/menuconfigs different from non-empty ones Yann E. MORIN
@ 2013-06-24 18:11 ` Yann E. MORIN
2013-06-24 18:11 ` [PATCH 09/15] kconfig/conf: fix randconfig setting multiple symbols in a choice Yann E. MORIN
` (7 subsequent siblings)
15 siblings, 0 replies; 36+ messages in thread
From: Yann E. MORIN @ 2013-06-24 18:11 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Michal Marek, Clement Chauplannaz, Yann E. MORIN
From: Clement Chauplannaz <chauplac@gmail.com>
The script `config' prints its name in usage() function. It is currently
hard-coded to value `config'. However, the script may be reused under
a different name in contexts other than the Linux Kernel.
Replace the hard-coded value `config' by the name of the script at runtime.
Signed-off-by: Clement Chauplannaz <chauplac@gmail.com>
Acked-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
---
scripts/config | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/scripts/config b/scripts/config
index bb4d3de..6b3272e 100755
--- a/scripts/config
+++ b/scripts/config
@@ -1,6 +1,8 @@
#!/bin/bash
# Manipulate options in a .config file from the command line
+myname=${0##*/}
+
# If no prefix forced, use the default CONFIG_
CONFIG_="${CONFIG_-CONFIG_}"
@@ -8,7 +10,7 @@ usage() {
cat >&2 <<EOL
Manipulate options in a .config file from the command line.
Usage:
-config options command ...
+$myname options command ...
commands:
--enable|-e option Enable option
--disable|-d option Disable option
@@ -33,14 +35,14 @@ options:
--file config-file .config file to change (default .config)
--keep-case|-k Keep next symbols' case (dont' upper-case it)
-config doesn't check the validity of the .config file. This is done at next
+$myname doesn't check the validity of the .config file. This is done at next
make time.
-By default, config will upper-case the given symbol. Use --keep-case to keep
+By default, $myname will upper-case the given symbol. Use --keep-case to keep
the case of all following symbols unchanged.
-config uses 'CONFIG_' as the default symbol prefix. Set the environment
-variable CONFIG_ to the prefix to use. Eg.: CONFIG_="FOO_" config ...
+$myname uses 'CONFIG_' as the default symbol prefix. Set the environment
+variable CONFIG_ to the prefix to use. Eg.: CONFIG_="FOO_" $myname ...
EOL
exit 1
}
--
1.8.1.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 09/15] kconfig/conf: fix randconfig setting multiple symbols in a choice
2013-06-24 18:11 [pull request v2] Pull request for branch yem-kconfig-for-next Yann E. MORIN
` (7 preceding siblings ...)
2013-06-24 18:11 ` [PATCH 08/15] scripts/config: replace hard-coded script name by a dynamic value Yann E. MORIN
@ 2013-06-24 18:11 ` Yann E. MORIN
2013-06-25 7:23 ` Sedat Dilek
2013-06-24 18:11 ` [PATCH 10/15] kconfig/conf: accept a base-16 seed for randconfig Yann E. MORIN
` (6 subsequent siblings)
15 siblings, 1 reply; 36+ messages in thread
From: Yann E. MORIN @ 2013-06-24 18:11 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Michal Marek, Yann E. MORIN, Matthieu CASTET,
Arnaud Lacombe, Sedat Dilek
From: "Yann E. MORIN" <yann.morin.1998@free.fr>
Currently, randconfig may set more than one symbol in a given choice.
Given this config file:
config A
bool "A"
if A
choice
bool "B/C/D"
config B
bool "B"
config C
bool "C"
config D
bool "D"
endchoice
endif # A
Then randconfig generates such .config files (case where A is not set is not
shown below for brevity), and where only the right-most .config is valid:
CONFIG_A=y CONFIG_A=y CONFIG_A=y
CONFIG_B=y CONFIG_B=y CONFIG_B=y
CONFIG_C=y # CONFIG_C is not set # CONFIG_C is not set
# CONFIG_D is not set CONFIG_D=y # CONFIG_D is not set
That is, in a randomised choice, the first symbol is always selected,
and at most one other symbol may be selected.
This is due to symbol randomised in a choice not being properly flagged
as having a value.
Fix that by flagging those symbols adequately: have a user-defined value,
and be not valid (to force recalculation of the symbol).
Note: if the choice is not conditional, then the randomisation is properly
done.
Reported-by: Matthieu CASTET <matthieu.castet@parrot.com>
Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
[yann.morin.1998@free.fr: independently re-done the same patch as Matthieu,
as pointed out by Sedat]
Cc: Arnaud Lacombe <lacombar@gmail.com>
Cc: Sedat Dilek <sedat.dilek@gmail.com>
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
scripts/kconfig/confdata.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index 35e0f16..d36bc1f 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -1077,6 +1077,9 @@ static void randomize_choice_values(struct symbol *csym)
else {
sym->def[S_DEF_USER].tri = no;
}
+ sym->flags |= SYMBOL_DEF_USER;
+ /* clear VALID to get value calculated */
+ sym->flags &= ~SYMBOL_VALID;
}
csym->flags |= SYMBOL_DEF_USER;
/* clear VALID to get value calculated */
--
1.8.1.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 09/15] kconfig/conf: fix randconfig setting multiple symbols in a choice
2013-06-24 18:11 ` [PATCH 09/15] kconfig/conf: fix randconfig setting multiple symbols in a choice Yann E. MORIN
@ 2013-06-25 7:23 ` Sedat Dilek
0 siblings, 0 replies; 36+ messages in thread
From: Sedat Dilek @ 2013-06-25 7:23 UTC (permalink / raw)
To: Yann E. MORIN, Michal Marek
Cc: linux-kbuild, linux-kernel, Matthieu CASTET, Arnaud Lacombe,
Alexander Kriegisch
On Mon, Jun 24, 2013 at 8:11 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
>
> Currently, randconfig may set more than one symbol in a given choice.
> Given this config file:
> config A
> bool "A"
> if A
> choice
> bool "B/C/D"
> config B
> bool "B"
> config C
> bool "C"
> config D
> bool "D"
> endchoice
> endif # A
>
> Then randconfig generates such .config files (case where A is not set is not
> shown below for brevity), and where only the right-most .config is valid:
> CONFIG_A=y CONFIG_A=y CONFIG_A=y
> CONFIG_B=y CONFIG_B=y CONFIG_B=y
> CONFIG_C=y # CONFIG_C is not set # CONFIG_C is not set
> # CONFIG_D is not set CONFIG_D=y # CONFIG_D is not set
>
> That is, in a randomised choice, the first symbol is always selected,
> and at most one other symbol may be selected.
>
> This is due to symbol randomised in a choice not being properly flagged
> as having a value.
>
> Fix that by flagging those symbols adequately: have a user-defined value,
> and be not valid (to force recalculation of the symbol).
>
> Note: if the choice is not conditional, then the randomisation is properly
> done.
>
> Reported-by: Matthieu CASTET <matthieu.castet@parrot.com>
> Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
> [yann.morin.1998@free.fr: independently re-done the same patch as Matthieu,
> as pointed out by Sedat]
> Cc: Arnaud Lacombe <lacombar@gmail.com>
> Cc: Sedat Dilek <sedat.dilek@gmail.com>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
[ CC Alexander ]
Can you or Marek (kbuild maintainer) please add also a Reported-by of
Alexander (see [1])?
Reported-by: Alexander Kriegisch <Alexander@kriegisch.name>
It's a bit a pity that you discovered this issue by yourself independantly.
As said in PM, the Freetz router project has included it when playing
with Kconfig of Linux-v3.1-rc9.
Thanks for taking care.
- Sedat -
[1] http://www.spinics.net/lists/linux-kbuild/msg05702.html
[2] http://freetz.org/browser/trunk/tools/make/patches/340-fix_randconfig_choice.kconfig.patch
> ---
> scripts/kconfig/confdata.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
> index 35e0f16..d36bc1f 100644
> --- a/scripts/kconfig/confdata.c
> +++ b/scripts/kconfig/confdata.c
> @@ -1077,6 +1077,9 @@ static void randomize_choice_values(struct symbol *csym)
> else {
> sym->def[S_DEF_USER].tri = no;
> }
> + sym->flags |= SYMBOL_DEF_USER;
> + /* clear VALID to get value calculated */
> + sym->flags &= ~SYMBOL_VALID;
> }
> csym->flags |= SYMBOL_DEF_USER;
> /* clear VALID to get value calculated */
> --
> 1.8.1.2
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 10/15] kconfig/conf: accept a base-16 seed for randconfig
2013-06-24 18:11 [pull request v2] Pull request for branch yem-kconfig-for-next Yann E. MORIN
` (8 preceding siblings ...)
2013-06-24 18:11 ` [PATCH 09/15] kconfig/conf: fix randconfig setting multiple symbols in a choice Yann E. MORIN
@ 2013-06-24 18:11 ` Yann E. MORIN
2013-06-24 18:11 ` [PATCH 11/15] kconfig/conf: print the seed used to initialise the RNG " Yann E. MORIN
` (5 subsequent siblings)
15 siblings, 0 replies; 36+ messages in thread
From: Yann E. MORIN @ 2013-06-24 18:11 UTC (permalink / raw)
To: linux-kbuild; +Cc: linux-kernel, Michal Marek, Yann E. MORIN
From: "Yann E. MORIN" <yann.morin.1998@free.fr>
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
scripts/kconfig/conf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
index bde5b95..94521c7 100644
--- a/scripts/kconfig/conf.c
+++ b/scripts/kconfig/conf.c
@@ -527,7 +527,7 @@ int main(int ac, char **av)
seed_env = getenv("KCONFIG_SEED");
if( seed_env && *seed_env ) {
char *endp;
- int tmp = (int)strtol(seed_env, &endp, 10);
+ int tmp = (int)strtol(seed_env, &endp, 0);
if (*endp == '\0') {
seed = tmp;
}
--
1.8.1.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 11/15] kconfig/conf: print the seed used to initialise the RNG for randconfig
2013-06-24 18:11 [pull request v2] Pull request for branch yem-kconfig-for-next Yann E. MORIN
` (9 preceding siblings ...)
2013-06-24 18:11 ` [PATCH 10/15] kconfig/conf: accept a base-16 seed for randconfig Yann E. MORIN
@ 2013-06-24 18:11 ` Yann E. MORIN
2013-06-24 18:11 ` [PATCH 12/15] kconfig: sort found symbols by relevance Yann E. MORIN
` (4 subsequent siblings)
15 siblings, 0 replies; 36+ messages in thread
From: Yann E. MORIN @ 2013-06-24 18:11 UTC (permalink / raw)
To: linux-kbuild; +Cc: linux-kernel, Michal Marek, Yann E. MORIN
From: "Yann E. MORIN" <yann.morin.1998@free.fr>
... so the user has a chance to reproduce a test-case.
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
scripts/kconfig/conf.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
index 94521c7..38616c1 100644
--- a/scripts/kconfig/conf.c
+++ b/scripts/kconfig/conf.c
@@ -532,6 +532,7 @@ int main(int ac, char **av)
seed = tmp;
}
}
+ fprintf( stderr, "KCONFIG_SEED=0x%X\n", seed );
srand(seed);
break;
}
--
1.8.1.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 12/15] kconfig: sort found symbols by relevance
2013-06-24 18:11 [pull request v2] Pull request for branch yem-kconfig-for-next Yann E. MORIN
` (10 preceding siblings ...)
2013-06-24 18:11 ` [PATCH 11/15] kconfig/conf: print the seed used to initialise the RNG " Yann E. MORIN
@ 2013-06-24 18:11 ` Yann E. MORIN
2013-07-08 11:19 ` Jean Delvare
2013-06-24 18:11 ` [PATCH 13/15] kconfig/[mn]conf: make it explicit in the search box that a regexp is possible Yann E. MORIN
` (3 subsequent siblings)
15 siblings, 1 reply; 36+ messages in thread
From: Yann E. MORIN @ 2013-06-24 18:11 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Michal Marek, Yann E. MORIN, Jean Delvare,
Roland Eggner, Wang YanQing
From: "Yann E. MORIN" <yann.morin.1998@free.fr>
When searching for symbols, return the symbols sorted by relevance.
Sorting is done as thus:
- first, symbols that match exactly
- then, alphabetical sort
Since the search can be a regexp, it is possible that more than one symbol
matches exactly. In this case, we can't decide which to sort first, so we
fallback to alphabeticall sort.
Explain this (new!) sorting heuristic in the documentation.
Reported-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Jean Delvare <jdelvare@suse.de>
Cc: Michal Marek <mmarek@suse.cz>
Cc: Roland Eggner <edvx1@systemanalysen.net>
Cc: Wang YanQing <udknight@gmail.com>
--
Changes v1->v2:
- drop the previous, complex heuristic in favour of a simpler heuristic
that is both easier to understand, *and* to maintain (Jean)
- explain sorting heuristic in the doc (Jean)
---
Documentation/kbuild/kconfig.txt | 13 +++++++
scripts/kconfig/symbol.c | 78 +++++++++++++++++++++++++++++++++++-----
2 files changed, 82 insertions(+), 9 deletions(-)
diff --git a/Documentation/kbuild/kconfig.txt b/Documentation/kbuild/kconfig.txt
index 3f429ed..e9f9e76 100644
--- a/Documentation/kbuild/kconfig.txt
+++ b/Documentation/kbuild/kconfig.txt
@@ -174,6 +174,19 @@ Searching in menuconfig:
/^hotplug
+ When searching, symbols are sorted thus:
+ - exact match first: an exact match is when the search matches
+ the complete symbol name;
+ - alphabetical order: when two symbols do not match exactly,
+ they are sorted in alphabetical order (in the user's current
+ locale).
+ For example: ^ATH.K matches:
+ ATH5K ATH9K ATH5K_AHB ATH5K_DEBUG [...] ATH6KL ATH6KL_DEBUG
+ [...] ATH9K_AHB ATH9K_BTCOEX_SUPPORT ATH9K_COMMON [...]
+ of which only ATH5K and ATH9K match exactly and so are sorted
+ first (and in alphabetical order), then come all other symbols,
+ sorted in alphabetical order.
+
______________________________________________________________________
User interface options for 'menuconfig'
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index ab8f4c8..387d554 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -954,38 +954,98 @@ const char *sym_escape_string_value(const char *in)
return res;
}
+struct sym_match {
+ struct symbol *sym;
+ off_t so, eo;
+};
+
+/* Compare matched symbols as thus:
+ * - first, symbols that match exactly
+ * - then, alphabetical sort
+ */
+static int sym_rel_comp( const void *sym1, const void *sym2 )
+{
+ struct sym_match *s1 = *(struct sym_match **)sym1;
+ struct sym_match *s2 = *(struct sym_match **)sym2;
+ int l1, l2;
+
+ /* Exact match:
+ * - if matched length on symbol s1 is the length of that symbol,
+ * then this symbol should come first;
+ * - if matched length on symbol s2 is the length of that symbol,
+ * then this symbol should come first.
+ * Note: since the search can be a regexp, both symbols may match
+ * exactly; if this is the case, we can't decide which comes first,
+ * and we fallback to sorting alphabetically.
+ */
+ l1 = s1->eo - s1->so;
+ l2 = s2->eo - s2->so;
+ if (l1 == strlen(s1->sym->name) && l2 != strlen(s2->sym->name))
+ return -1;
+ if (l1 != strlen(s1->sym->name) && l2 == strlen(s2->sym->name))
+ return 1;
+
+ /* As a fallback, sort symbols alphabetically */
+ return strcmp(s1->sym->name, s2->sym->name);
+}
+
struct symbol **sym_re_search(const char *pattern)
{
struct symbol *sym, **sym_arr = NULL;
+ struct sym_match **sym_match_arr = NULL;
int i, cnt, size;
regex_t re;
+ regmatch_t match[1];
cnt = size = 0;
/* Skip if empty */
if (strlen(pattern) == 0)
return NULL;
- if (regcomp(&re, pattern, REG_EXTENDED|REG_NOSUB|REG_ICASE))
+ if (regcomp(&re, pattern, REG_EXTENDED|REG_ICASE))
return NULL;
for_all_symbols(i, sym) {
+ struct sym_match *tmp_sym_match;
if (sym->flags & SYMBOL_CONST || !sym->name)
continue;
- if (regexec(&re, sym->name, 0, NULL, 0))
+ if (regexec(&re, sym->name, 1, match, 0))
continue;
if (cnt + 1 >= size) {
- void *tmp = sym_arr;
+ void *tmp;
size += 16;
- sym_arr = realloc(sym_arr, size * sizeof(struct symbol *));
- if (!sym_arr) {
- free(tmp);
- return NULL;
+ tmp = realloc(sym_match_arr, size * sizeof(struct sym_match *));
+ if (!tmp) {
+ goto sym_re_search_free;
}
+ sym_match_arr = tmp;
}
sym_calc_value(sym);
- sym_arr[cnt++] = sym;
+ tmp_sym_match = (struct sym_match*)malloc(sizeof(struct sym_match));
+ if (!tmp_sym_match)
+ goto sym_re_search_free;
+ tmp_sym_match->sym = sym;
+ /* As regexec return 0, we know we have a match, so
+ * we can use match[0].rm_[se]o without further checks
+ */
+ tmp_sym_match->so = match[0].rm_so;
+ tmp_sym_match->eo = match[0].rm_eo;
+ sym_match_arr[cnt++] = tmp_sym_match;
}
- if (sym_arr)
+ if (sym_match_arr) {
+ qsort(sym_match_arr, cnt, sizeof(struct sym_match*), sym_rel_comp);
+ sym_arr = malloc((cnt+1) * sizeof(struct symbol));
+ if (!sym_arr)
+ goto sym_re_search_free;
+ for (i = 0; i < cnt; i++)
+ sym_arr[i] = sym_match_arr[i]->sym;
sym_arr[cnt] = NULL;
+ }
+sym_re_search_free:
+ if (sym_match_arr) {
+ for (i = 0; i < cnt; i++)
+ free(sym_match_arr[i]);
+ free(sym_match_arr);
+ }
regfree(&re);
return sym_arr;
--
1.8.1.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 12/15] kconfig: sort found symbols by relevance
2013-06-24 18:11 ` [PATCH 12/15] kconfig: sort found symbols by relevance Yann E. MORIN
@ 2013-07-08 11:19 ` Jean Delvare
2013-07-08 17:35 ` Yann E. MORIN
0 siblings, 1 reply; 36+ messages in thread
From: Jean Delvare @ 2013-07-08 11:19 UTC (permalink / raw)
To: Yann E. MORIN
Cc: linux-kbuild, linux-kernel, Michal Marek, Roland Eggner, Wang YanQing
Hi Yann,
Le Monday 24 June 2013 à 20:11 +0200, Yann E. MORIN a écrit :
> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
>
> When searching for symbols, return the symbols sorted by relevance.
>
> Sorting is done as thus:
> - first, symbols that match exactly
> - then, alphabetical sort
I like it, this is simple and efficient.
> Since the search can be a regexp, it is possible that more than one symbol
> matches exactly. In this case, we can't decide which to sort first, so we
> fallback to alphabeticall sort.
Typo: alphabetical.
> Explain this (new!) sorting heuristic in the documentation.
It's more of a rule than a heuristic.
> Reported-by: Jean Delvare <jdelvare@suse.de>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Jean Delvare <jdelvare@suse.de>
> Cc: Michal Marek <mmarek@suse.cz>
> Cc: Roland Eggner <edvx1@systemanalysen.net>
> Cc: Wang YanQing <udknight@gmail.com>
I tested it and it works fine, thanks!
Tested-by: Jean Delvare <jdelvare@suse.de>
Now comes my late review. Overall I like the idea and the code but a few
things could be improved:
> --
> Changes v1->v2:
> - drop the previous, complex heuristic in favour of a simpler heuristic
> that is both easier to understand, *and* to maintain (Jean)
> - explain sorting heuristic in the doc (Jean)
> ---
> Documentation/kbuild/kconfig.txt | 13 +++++++
> scripts/kconfig/symbol.c | 78 +++++++++++++++++++++++++++++++++++-----
> 2 files changed, 82 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/kbuild/kconfig.txt b/Documentation/kbuild/kconfig.txt
> index 3f429ed..e9f9e76 100644
> --- a/Documentation/kbuild/kconfig.txt
> +++ b/Documentation/kbuild/kconfig.txt
> @@ -174,6 +174,19 @@ Searching in menuconfig:
>
> /^hotplug
>
> + When searching, symbols are sorted thus:
> + - exact match first: an exact match is when the search matches
> + the complete symbol name;
The use of singular seems to imply there can be only one, while there
could be several as you explained above.
> + - alphabetical order: when two symbols do not match exactly,
> + they are sorted in alphabetical order (in the user's current
> + locale).
I think it would be better explained that way:
- first, exact matches, sorted alphabetically (an exact match
is when the search matches the complete symbol name);
- then, other matches, sorted alphabetically.
This is more concise and easier to grasp, methinks. I don't think the
reference to the user's locale is needed, as there's no surprise here,
and it probably doesn't matter anyway for kernel symbols.
BTW I was wondering if it would add value to explicitly print group
header labels "Exact matches" and "Other matches" in the search results.
What do you think?
> + For example: ^ATH.K matches:
> + ATH5K ATH9K ATH5K_AHB ATH5K_DEBUG [...] ATH6KL ATH6KL_DEBUG
> + [...] ATH9K_AHB ATH9K_BTCOEX_SUPPORT ATH9K_COMMON [...]
> + of which only ATH5K and ATH9K match exactly and so are sorted
> + first (and in alphabetical order), then come all other symbols,
> + sorted in alphabetical order.
Very clear example :)
> ______________________________________________________________________
> User interface options for 'menuconfig'
>
> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
> index ab8f4c8..387d554 100644
> --- a/scripts/kconfig/symbol.c
> +++ b/scripts/kconfig/symbol.c
> @@ -954,38 +954,98 @@ const char *sym_escape_string_value(const char *in)
> return res;
> }
>
> +struct sym_match {
> + struct symbol *sym;
> + off_t so, eo;
> +};
> +
> +/* Compare matched symbols as thus:
> + * - first, symbols that match exactly
> + * - then, alphabetical sort
> + */
> +static int sym_rel_comp( const void *sym1, const void *sym2 )
Coding style says no space after/before parentheses.
> +{
> + struct sym_match *s1 = *(struct sym_match **)sym1;
> + struct sym_match *s2 = *(struct sym_match **)sym2;
You shouldn't need these casts.
> + int l1, l2;
> +
> + /* Exact match:
> + * - if matched length on symbol s1 is the length of that symbol,
> + * then this symbol should come first;
> + * - if matched length on symbol s2 is the length of that symbol,
> + * then this symbol should come first.
> + * Note: since the search can be a regexp, both symbols may match
> + * exactly; if this is the case, we can't decide which comes first,
> + * and we fallback to sorting alphabetically.
> + */
> + l1 = s1->eo - s1->so;
> + l2 = s2->eo - s2->so;
> + if (l1 == strlen(s1->sym->name) && l2 != strlen(s2->sym->name))
> + return -1;
> + if (l1 != strlen(s1->sym->name) && l2 == strlen(s2->sym->name))
> + return 1;
Strlen is expensive so I would avoid calling it twice on the same symbol
name. You could store the result, together with the result of the
comparison, while you're at it. Something like:
int exact1, exact2;
exact1 = l1 == strlen(s1->sym->name);
exact2 = l2 == strlen(s2->sym->name);
if (exact1 && !exact2)
return -1;
if (!exact1 && exact2)
return 1;
You may even no longer need to introduce l1 and l2 as you would use them
only once.
It might be even faster to store the symbol length in struct sym_match,
but this would increase the structure size and consequently memory
consumption, and it is questionable if the speed gain is worth it.
Probably not.
> +
> + /* As a fallback, sort symbols alphabetically */
> + return strcmp(s1->sym->name, s2->sym->name);
> +}
> +
> struct symbol **sym_re_search(const char *pattern)
> {
> struct symbol *sym, **sym_arr = NULL;
> + struct sym_match **sym_match_arr = NULL;
> int i, cnt, size;
> regex_t re;
> + regmatch_t match[1];
>
> cnt = size = 0;
> /* Skip if empty */
> if (strlen(pattern) == 0)
> return NULL;
> - if (regcomp(&re, pattern, REG_EXTENDED|REG_NOSUB|REG_ICASE))
> + if (regcomp(&re, pattern, REG_EXTENDED|REG_ICASE))
> return NULL;
>
> for_all_symbols(i, sym) {
> + struct sym_match *tmp_sym_match;
> if (sym->flags & SYMBOL_CONST || !sym->name)
> continue;
> - if (regexec(&re, sym->name, 0, NULL, 0))
> + if (regexec(&re, sym->name, 1, match, 0))
> continue;
> if (cnt + 1 >= size) {
I think the "+ 1" can be dropped because the new array is not
NULL-terminated.
> - void *tmp = sym_arr;
> + void *tmp;
> size += 16;
> - sym_arr = realloc(sym_arr, size * sizeof(struct symbol *));
> - if (!sym_arr) {
> - free(tmp);
> - return NULL;
> + tmp = realloc(sym_match_arr, size * sizeof(struct sym_match *));
> + if (!tmp) {
> + goto sym_re_search_free;
> }
Unnecessary curly braces (as reported by checkpatch.) Checkpatch reports
a few other issues BTW, you should run it and fix them.
> + sym_match_arr = tmp;
> }
> sym_calc_value(sym);
> - sym_arr[cnt++] = sym;
> + tmp_sym_match = (struct sym_match*)malloc(sizeof(struct sym_match));
Cast not needed.
In fact I don't think this allocation is needed in the first place.
Calling malloc for every match is rather costly. If you would have
allocated an array of struct sym_match (rather than only pointers
thereto) before, you would not need this per-symbol malloc. Struct
sym_match is small enough to not warrant an extra level of allocation
and indirection IMHO.
> + if (!tmp_sym_match)
> + goto sym_re_search_free;
> + tmp_sym_match->sym = sym;
> + /* As regexec return 0, we know we have a match, so
> + * we can use match[0].rm_[se]o without further checks
> + */
> + tmp_sym_match->so = match[0].rm_so;
> + tmp_sym_match->eo = match[0].rm_eo;
> + sym_match_arr[cnt++] = tmp_sym_match;
> }
> - if (sym_arr)
> + if (sym_match_arr) {
> + qsort(sym_match_arr, cnt, sizeof(struct sym_match*), sym_rel_comp);
> + sym_arr = malloc((cnt+1) * sizeof(struct symbol));
> + if (!sym_arr)
> + goto sym_re_search_free;
> + for (i = 0; i < cnt; i++)
> + sym_arr[i] = sym_match_arr[i]->sym;
> sym_arr[cnt] = NULL;
> + }
> +sym_re_search_free:
> + if (sym_match_arr) {
> + for (i = 0; i < cnt; i++)
> + free(sym_match_arr[i]);
> + free(sym_match_arr);
> + }
> regfree(&re);
>
> return sym_arr;
--
Jean Delvare
Suse L3
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 12/15] kconfig: sort found symbols by relevance
2013-07-08 11:19 ` Jean Delvare
@ 2013-07-08 17:35 ` Yann E. MORIN
2013-07-10 20:46 ` Yann E. MORIN
2013-07-12 8:57 ` Jean Delvare
0 siblings, 2 replies; 36+ messages in thread
From: Yann E. MORIN @ 2013-07-08 17:35 UTC (permalink / raw)
To: Jean Delvare
Cc: linux-kbuild, linux-kernel, Michal Marek, Roland Eggner, Wang YanQing
Jean, All,
On 2013-07-08 13:19 +0200, Jean Delvare spake thusly:
> Le Monday 24 June 2013 à 20:11 +0200, Yann E. MORIN a écrit :
> > From: "Yann E. MORIN" <yann.morin.1998@free.fr>
[--SNIP--]
> > Since the search can be a regexp, it is possible that more than one symbol
> > matches exactly. In this case, we can't decide which to sort first, so we
> > fallback to alphabeticall sort.
>
> Typo: alphabetical.
OK.
> > Explain this (new!) sorting heuristic in the documentation.
>
> It's more of a rule than a heuristic.
OK.
> > Reported-by: Jean Delvare <jdelvare@suse.de>
> > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > Cc: Jean Delvare <jdelvare@suse.de>
> > Cc: Michal Marek <mmarek@suse.cz>
> > Cc: Roland Eggner <edvx1@systemanalysen.net>
> > Cc: Wang YanQing <udknight@gmail.com>
>
> I tested it and it works fine, thanks!
>
> Tested-by: Jean Delvare <jdelvare@suse.de>
>
> Now comes my late review. Overall I like the idea and the code but a few
> things could be improved:
Since this is already in Michal's tree, on should I proceed?
- send an updated patch that replaces that one, or
- send another additional patch with your proposed changes?
> > + When searching, symbols are sorted thus:
> > + - exact match first: an exact match is when the search matches
> > + the complete symbol name;
>
> The use of singular seems to imply there can be only one, while there
> could be several as you explained above.
OK.
> > + - alphabetical order: when two symbols do not match exactly,
> > + they are sorted in alphabetical order (in the user's current
> > + locale).
>
> I think it would be better explained that way:
>
> - first, exact matches, sorted alphabetically (an exact match
> is when the search matches the complete symbol name);
> - then, other matches, sorted alphabetically.
OK.
> This is more concise and easier to grasp, methinks. I don't think the
> reference to the user's locale is needed, as there's no surprise here,
> and it probably doesn't matter anyway for kernel symbols.
Yes it may matter even for kernel symbols, since some locales may
consider '_' as a character to sort by, while other locales may not.
> BTW I was wondering if it would add value to explicitly print group
> header labels "Exact matches" and "Other matches" in the search results.
> What do you think?
It is not trivial to do, since the search function only returns a single
array, so there's no way for frontends (which do the display) to
differentiate which part of the array are exact matches, and which are
only partial matches.
It is much more involved, and I don't think it would be easy to
implement.
> > ______________________________________________________________________
> > User interface options for 'menuconfig'
> >
> > diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
> > index ab8f4c8..387d554 100644
> > --- a/scripts/kconfig/symbol.c
> > +++ b/scripts/kconfig/symbol.c
> > @@ -954,38 +954,98 @@ const char *sym_escape_string_value(const char *in)
> > return res;
> > }
> >
> > +struct sym_match {
> > + struct symbol *sym;
> > + off_t so, eo;
> > +};
> > +
> > +/* Compare matched symbols as thus:
> > + * - first, symbols that match exactly
> > + * - then, alphabetical sort
> > + */
> > +static int sym_rel_comp( const void *sym1, const void *sym2 )
>
> Coding style says no space after/before parentheses.
OK.
> > +{
> > + struct sym_match *s1 = *(struct sym_match **)sym1;
> > + struct sym_match *s2 = *(struct sym_match **)sym2;
>
> You shouldn't need these casts.
Probably not, indeed, but I like to write (and read) what I expect to
happen, and pointer arithmetics is always something I dread to foobar.
OK.
> > + int l1, l2;
> > +
> > + /* Exact match:
> > + * - if matched length on symbol s1 is the length of that symbol,
> > + * then this symbol should come first;
> > + * - if matched length on symbol s2 is the length of that symbol,
> > + * then this symbol should come first.
> > + * Note: since the search can be a regexp, both symbols may match
> > + * exactly; if this is the case, we can't decide which comes first,
> > + * and we fallback to sorting alphabetically.
> > + */
> > + l1 = s1->eo - s1->so;
> > + l2 = s2->eo - s2->so;
> > + if (l1 == strlen(s1->sym->name) && l2 != strlen(s2->sym->name))
> > + return -1;
> > + if (l1 != strlen(s1->sym->name) && l2 == strlen(s2->sym->name))
> > + return 1;
>
> Strlen is expensive so I would avoid calling it twice on the same symbol
> name. You could store the result, together with the result of the
> comparison, while you're at it. Something like:
>
> int exact1, exact2;
>
> exact1 = l1 == strlen(s1->sym->name);
> exact2 = l2 == strlen(s2->sym->name);
> if (exact1 && !exact2)
> return -1;
> if (!exact1 && exact2)
> return 1;
>
> You may even no longer need to introduce l1 and l2 as you would use them
> only once.
OK.
> It might be even faster to store the symbol length in struct sym_match,
> but this would increase the structure size and consequently memory
> consumption, and it is questionable if the speed gain is worth it.
> Probably not.
I intended this structure to only hold the result of the regexp match,
and nothing more. The symbol length does not belong there, IMHO.
Besides, it's easy to get back, since the symbol struct is available.
OTOH, we would gain by computing strlen at regexp match, instead of
every time in the sorting function.
But that's micro-optimisation, methinks. Searching for the example
^ATH.K took less than me focusing from the RETURN key to the screen. ;-)
[--SNIP--]
> > for_all_symbols(i, sym) {
> > + struct sym_match *tmp_sym_match;
> > if (sym->flags & SYMBOL_CONST || !sym->name)
> > continue;
> > - if (regexec(&re, sym->name, 0, NULL, 0))
> > + if (regexec(&re, sym->name, 1, match, 0))
> > continue;
> > if (cnt + 1 >= size) {
>
> I think the "+ 1" can be dropped because the new array is not
> NULL-terminated.
I'll look into that.
> > - void *tmp = sym_arr;
> > + void *tmp;
> > size += 16;
> > - sym_arr = realloc(sym_arr, size * sizeof(struct symbol *));
> > - if (!sym_arr) {
> > - free(tmp);
> > - return NULL;
> > + tmp = realloc(sym_match_arr, size * sizeof(struct sym_match *));
> > + if (!tmp) {
> > + goto sym_re_search_free;
> > }
>
> Unnecessary curly braces (as reported by checkpatch.) Checkpatch reports
> a few other issues BTW, you should run it and fix them.
My fault, as I always use curly braces, even around single statements,
in many other projects. ( /me believes this kernel coding style is bad,
but will abide by the rules! ;-) ).
OK.
> > + sym_match_arr = tmp;
> > }
> > sym_calc_value(sym);
> > - sym_arr[cnt++] = sym;
> > + tmp_sym_match = (struct sym_match*)malloc(sizeof(struct sym_match));
>
> Cast not needed.
OK.
> In fact I don't think this allocation is needed in the first place.
> Calling malloc for every match is rather costly. If you would have
> allocated an array of struct sym_match (rather than only pointers
> thereto) before, you would not need this per-symbol malloc. Struct
> sym_match is small enough to not warrant an extra level of allocation
> and indirection IMHO.
OK, will look into that.
Thank you for the review! :-)
Regards,
Yann E. MORIN.
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 12/15] kconfig: sort found symbols by relevance
2013-07-08 17:35 ` Yann E. MORIN
@ 2013-07-10 20:46 ` Yann E. MORIN
2013-07-12 9:07 ` Jean Delvare
2013-07-12 8:57 ` Jean Delvare
1 sibling, 1 reply; 36+ messages in thread
From: Yann E. MORIN @ 2013-07-10 20:46 UTC (permalink / raw)
To: Jean Delvare, Michal Marek
Cc: linux-kbuild, linux-kernel, Roland Eggner, Wang YanQing
Michal, All,
On 2013-07-08 19:35 +0200, Yann E. MORIN spake thusly:
> On 2013-07-08 13:19 +0200, Jean Delvare spake thusly:
> > Le Monday 24 June 2013 à 20:11 +0200, Yann E. MORIN a écrit :
> > > From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> [--SNIP--]
> > > Since the search can be a regexp, it is possible that more than one symbol
> > > matches exactly. In this case, we can't decide which to sort first, so we
> > > fallback to alphabeticall sort.
[--SNIP--]
> > > Reported-by: Jean Delvare <jdelvare@suse.de>
> > > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > > Cc: Jean Delvare <jdelvare@suse.de>
> > > Cc: Michal Marek <mmarek@suse.cz>
> > > Cc: Roland Eggner <edvx1@systemanalysen.net>
> > > Cc: Wang YanQing <udknight@gmail.com>
> >
> > I tested it and it works fine, thanks!
> >
> > Tested-by: Jean Delvare <jdelvare@suse.de>
> >
> > Now comes my late review. Overall I like the idea and the code but a few
> > things could be improved:
>
> Since this is already in Michal's tree, on should I proceed?
> - send an updated patch that replaces that one, or
> - send another additional patch with your proposed changes?
OK, since Michal already sent his pull-request to Linus, I'll prepare a
corrective patch I'll submit before the end of the week. Is that OK with
you, Michal?
[--SNIP--]
> > > +static int sym_rel_comp( const void *sym1, const void *sym2 )
> > > +{
> > > + struct sym_match *s1 = *(struct sym_match **)sym1;
> > > + struct sym_match *s2 = *(struct sym_match **)sym2;
> >
> > You shouldn't need these casts.
>
> Probably not, indeed, but I like to write (and read) what I expect to
> happen, and pointer arithmetics is always something I dread to foobar.
In fact, we need the cast, otherwise gcc whines about const/non-const.
[--SNIP--]
> > > for_all_symbols(i, sym) {
> > > + struct sym_match *tmp_sym_match;
> > > if (sym->flags & SYMBOL_CONST || !sym->name)
> > > continue;
> > > - if (regexec(&re, sym->name, 0, NULL, 0))
> > > + if (regexec(&re, sym->name, 1, match, 0))
> > > continue;
> > > if (cnt + 1 >= size) {
> >
> > I think the "+ 1" can be dropped because the new array is not
> > NULL-terminated.
Indeed.
> > > + sym_match_arr = tmp;
> > > }
> > > sym_calc_value(sym);
> > > - sym_arr[cnt++] = sym;
> > > + tmp_sym_match = (struct sym_match*)malloc(sizeof(struct sym_match));
> >
> > Cast not needed.
>
> OK.
>
> > In fact I don't think this allocation is needed in the first place.
> > Calling malloc for every match is rather costly. If you would have
> > allocated an array of struct sym_match (rather than only pointers
> > thereto) before, you would not need this per-symbol malloc. Struct
> > sym_match is small enough to not warrant an extra level of allocation
> > and indirection IMHO.
Indeed, it makes for cleaner code.
Thank you again! :-)
Regards,
Yann E. MORIN.
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 12/15] kconfig: sort found symbols by relevance
2013-07-10 20:46 ` Yann E. MORIN
@ 2013-07-12 9:07 ` Jean Delvare
2013-07-13 13:06 ` Yann E. MORIN
0 siblings, 1 reply; 36+ messages in thread
From: Jean Delvare @ 2013-07-12 9:07 UTC (permalink / raw)
To: Yann E. MORIN
Cc: Michal Marek, linux-kbuild, linux-kernel, Roland Eggner, Wang YanQing
Yann, Michal,
Le Wednesday 10 July 2013 à 22:46 +0200, Yann E. MORIN a écrit :
> Michal, All,
>
> On 2013-07-08 19:35 +0200, Yann E. MORIN spake thusly:
> > On 2013-07-08 13:19 +0200, Jean Delvare spake thusly:
> > > Le Monday 24 June 2013 à 20:11 +0200, Yann E. MORIN a écrit :
> > > > From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > [--SNIP--]
> > > > Since the search can be a regexp, it is possible that more than one symbol
> > > > matches exactly. In this case, we can't decide which to sort first, so we
> > > > fallback to alphabeticall sort.
> [--SNIP--]
> > > > Reported-by: Jean Delvare <jdelvare@suse.de>
> > > > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > > > Cc: Jean Delvare <jdelvare@suse.de>
> > > > Cc: Michal Marek <mmarek@suse.cz>
> > > > Cc: Roland Eggner <edvx1@systemanalysen.net>
> > > > Cc: Wang YanQing <udknight@gmail.com>
> > >
> > > I tested it and it works fine, thanks!
> > >
> > > Tested-by: Jean Delvare <jdelvare@suse.de>
> > >
> > > Now comes my late review. Overall I like the idea and the code but a few
> > > things could be improved:
> >
> > Since this is already in Michal's tree, on should I proceed?
> > - send an updated patch that replaces that one, or
> > - send another additional patch with your proposed changes?
>
> OK, since Michal already sent his pull-request to Linus, I'll prepare a
> corrective patch I'll submit before the end of the week. Is that OK with
> you, Michal?
Sounds good to me at least. I'll do my best to test and review it
quickly this time.
> [--SNIP--]
> > > > +static int sym_rel_comp( const void *sym1, const void *sym2 )
> > > > +{
> > > > + struct sym_match *s1 = *(struct sym_match **)sym1;
> > > > + struct sym_match *s2 = *(struct sym_match **)sym2;
> > >
> > > You shouldn't need these casts.
> >
> > Probably not, indeed, but I like to write (and read) what I expect to
> > happen, and pointer arithmetics is always something I dread to foobar.
>
> In fact, we need the cast, otherwise gcc whines about const/non-const.
And quite rightly so, as you are taking const pointers (i.e. the caller
told you you are _not_ allowed to change the contents) and making them
non-const pointers (i.e. you give yourself the right to still change the
contents.) It happens that your function doesn't actually change the
contents, so no harm done, but this is still conceptually wrong.
Preserving the const nature of pointers down the call chain lets the
compiler warn you if a function changes data it was not supposed to.
So what you want to do is:
static int sym_rel_comp(const void *sym1, const void *sym2)
{
const struct sym_match *s1 = sym1;
const struct sym_match *s2 = sym2;
This is both concise and correct, and it makes gcc happy.
Thanks,
--
Jean Delvare
Suse L3
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 12/15] kconfig: sort found symbols by relevance
2013-07-12 9:07 ` Jean Delvare
@ 2013-07-13 13:06 ` Yann E. MORIN
0 siblings, 0 replies; 36+ messages in thread
From: Yann E. MORIN @ 2013-07-13 13:06 UTC (permalink / raw)
To: Jean Delvare
Cc: Michal Marek, linux-kbuild, linux-kernel, Roland Eggner, Wang YanQing
Jean, All,
On 2013-07-12 11:07 +0200, Jean Delvare spake thusly:
[--SNIP--]
> > > > > +static int sym_rel_comp( const void *sym1, const void *sym2 )
> > > > > +{
> > > > > + struct sym_match *s1 = *(struct sym_match **)sym1;
> > > > > + struct sym_match *s2 = *(struct sym_match **)sym2;
> > > >
> > > > You shouldn't need these casts.
> > >
> > > Probably not, indeed, but I like to write (and read) what I expect to
> > > happen, and pointer arithmetics is always something I dread to foobar.
> >
> > In fact, we need the cast, otherwise gcc whines about const/non-const.
>
> And quite rightly so, as you are taking const pointers (i.e. the caller
> told you you are _not_ allowed to change the contents) and making them
> non-const pointers (i.e. you give yourself the right to still change the
> contents.) It happens that your function doesn't actually change the
> contents, so no harm done, but this is still conceptually wrong.
> Preserving the const nature of pointers down the call chain lets the
> compiler warn you if a function changes data it was not supposed to.
>
> So what you want to do is:
>
> static int sym_rel_comp(const void *sym1, const void *sym2)
> {
> const struct sym_match *s1 = sym1;
> const struct sym_match *s2 = sym2;
>
> This is both concise and correct, and it makes gcc happy.
Yes, that's what I thought to, and what I was about to do. Thanks for
confirming this! :-)
Regards,
Yann E. MORIN.
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 12/15] kconfig: sort found symbols by relevance
2013-07-08 17:35 ` Yann E. MORIN
2013-07-10 20:46 ` Yann E. MORIN
@ 2013-07-12 8:57 ` Jean Delvare
1 sibling, 0 replies; 36+ messages in thread
From: Jean Delvare @ 2013-07-12 8:57 UTC (permalink / raw)
To: Yann E. MORIN
Cc: linux-kbuild, linux-kernel, Michal Marek, Roland Eggner, Wang YanQing
Hi Yann,
Sorry again for the late reply, busy...
Le Monday 08 July 2013 à 19:35 +0200, Yann E. MORIN a écrit :
> On 2013-07-08 13:19 +0200, Jean Delvare spake thusly:
> > This is more concise and easier to grasp, methinks. I don't think the
> > reference to the user's locale is needed, as there's no surprise here,
> > and it probably doesn't matter anyway for kernel symbols.
>
> Yes it may matter even for kernel symbols, since some locales may
> consider '_' as a character to sort by, while other locales may not.
I didn't know that, thanks for the information. That being said I still
believe mentioning the user's locale is not needed ;-)
> > BTW I was wondering if it would add value to explicitly print group
> > header labels "Exact matches" and "Other matches" in the search results.
> > What do you think?
>
> It is not trivial to do, since the search function only returns a single
> array, so there's no way for frontends (which do the display) to
> differentiate which part of the array are exact matches, and which are
> only partial matches.
>
> It is much more involved, and I don't think it would be easy to
> implement.
I understand, and I agree it's not worth the effort.
> > > +{
> > > + struct sym_match *s1 = *(struct sym_match **)sym1;
> > > + struct sym_match *s2 = *(struct sym_match **)sym2;
> >
> > You shouldn't need these casts.
>
> Probably not, indeed, but I like to write (and read) what I expect to
> happen, and pointer arithmetics is always something I dread to foobar.
I hear this argument every now and then but I do not think it holds. If
you always forcibly cast pointers even when you don't have to, then gcc
has no chance to warn you if you get it wrong.
More on this later as I reply to your next post...
> > It might be even faster to store the symbol length in struct sym_match,
> > but this would increase the structure size and consequently memory
> > consumption, and it is questionable if the speed gain is worth it.
> > Probably not.
>
> I intended this structure to only hold the result of the regexp match,
> and nothing more. The symbol length does not belong there, IMHO.
> Besides, it's easy to get back, since the symbol struct is available.
>
> OTOH, we would gain by computing strlen at regexp match, instead of
> every time in the sorting function.
>
> But that's micro-optimisation, methinks. Searching for the example
> ^ATH.K took less than me focusing from the RETURN key to the screen. ;-)
OK, fair enough. I always pay attention to algorithmic complexity
because not everyone is running brand new and powerful machines (I am
not, to start with.)
I agree that the number of items returned by the search should generally
be small enough so it shouldn't be an issue in practice. If you want to
test your code on extreme cases though, you could search for "A" or even
".". It takes about 20 seconds for "A" on my machine and close to 1
minute for ".", which is quite slow. That being said that was already
the case before your patch, so I'm not blaming your changes for it.
> (...)
> Thank you for the review! :-)
You're welcome :-)
--
Jean Delvare
Suse L3
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 13/15] kconfig/[mn]conf: make it explicit in the search box that a regexp is possible
2013-06-24 18:11 [pull request v2] Pull request for branch yem-kconfig-for-next Yann E. MORIN
` (11 preceding siblings ...)
2013-06-24 18:11 ` [PATCH 12/15] kconfig: sort found symbols by relevance Yann E. MORIN
@ 2013-06-24 18:11 ` Yann E. MORIN
2013-07-08 11:25 ` Jean Delvare
2013-06-24 18:11 ` [PATCH 14/15] kconfig: loop as long as we changed some symbols in randconfig Yann E. MORIN
` (2 subsequent siblings)
15 siblings, 1 reply; 36+ messages in thread
From: Yann E. MORIN @ 2013-06-24 18:11 UTC (permalink / raw)
To: linux-kbuild; +Cc: linux-kernel, Michal Marek, Yann E. MORIN, Jean Delvare
From: "Yann E. MORIN" <yann.morin.1998@free.fr>
Reported-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Jean Delvare <jdelvare@suse.de>
Cc: Michal Marek <mmarek@suse.cz>
---
scripts/kconfig/mconf.c | 2 +-
scripts/kconfig/nconf.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c
index 6ee4aae..18d3dc9 100644
--- a/scripts/kconfig/mconf.c
+++ b/scripts/kconfig/mconf.c
@@ -401,7 +401,7 @@ static void search_conf(void)
struct subtitle_part stpart;
title = str_new();
- str_printf( &title, _("Enter %s (sub)string to search for "
+ str_printf( &title, _("Enter %s (sub)string or regexp to search for "
"(with or without \"%s\")"), CONFIG_, CONFIG_);
again:
diff --git a/scripts/kconfig/nconf.c b/scripts/kconfig/nconf.c
index 0183153..7975d8d 100644
--- a/scripts/kconfig/nconf.c
+++ b/scripts/kconfig/nconf.c
@@ -695,7 +695,7 @@ static void search_conf(void)
int dres;
title = str_new();
- str_printf( &title, _("Enter %s (sub)string to search for "
+ str_printf( &title, _("Enter %s (sub)string or regexp to search for "
"(with or without \"%s\")"), CONFIG_, CONFIG_);
again:
--
1.8.1.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 13/15] kconfig/[mn]conf: make it explicit in the search box that a regexp is possible
2013-06-24 18:11 ` [PATCH 13/15] kconfig/[mn]conf: make it explicit in the search box that a regexp is possible Yann E. MORIN
@ 2013-07-08 11:25 ` Jean Delvare
2013-07-08 17:37 ` Yann E. MORIN
0 siblings, 1 reply; 36+ messages in thread
From: Jean Delvare @ 2013-07-08 11:25 UTC (permalink / raw)
To: Yann E. MORIN; +Cc: linux-kbuild, linux-kernel, Michal Marek
Hi Yann,
Le Monday 24 June 2013 à 20:11 +0200, Yann E. MORIN a écrit :
> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
>
> Reported-by: Jean Delvare <jdelvare@suse.de>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Jean Delvare <jdelvare@suse.de>
> Cc: Michal Marek <mmarek@suse.cz>
> ---
> scripts/kconfig/mconf.c | 2 +-
> scripts/kconfig/nconf.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c
> index 6ee4aae..18d3dc9 100644
> --- a/scripts/kconfig/mconf.c
> +++ b/scripts/kconfig/mconf.c
> @@ -401,7 +401,7 @@ static void search_conf(void)
> struct subtitle_part stpart;
>
> title = str_new();
> - str_printf( &title, _("Enter %s (sub)string to search for "
> + str_printf( &title, _("Enter %s (sub)string or regexp to search for "
> "(with or without \"%s\")"), CONFIG_, CONFIG_);
While clearer, this also makes the title span over two lines when it
used to fit on a single one. I would like to suggest the following:
str_printf(&title, _("Enter (sub)string or regexp to search for "
"(with or without \"%s\")"), CONFIG_);
Rationale: the "(with or without CONFIG_)" makes things clear enough
IMHO, making the first occurrence redundant.
But if you don't like this proposal then let's just go with yours, it's
up to you.
>
> again:
> diff --git a/scripts/kconfig/nconf.c b/scripts/kconfig/nconf.c
> index 0183153..7975d8d 100644
> --- a/scripts/kconfig/nconf.c
> +++ b/scripts/kconfig/nconf.c
> @@ -695,7 +695,7 @@ static void search_conf(void)
> int dres;
>
> title = str_new();
> - str_printf( &title, _("Enter %s (sub)string to search for "
> + str_printf( &title, _("Enter %s (sub)string or regexp to search for "
> "(with or without \"%s\")"), CONFIG_, CONFIG_);
>
> again:
--
Jean Delvare
Suse L3
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 13/15] kconfig/[mn]conf: make it explicit in the search box that a regexp is possible
2013-07-08 11:25 ` Jean Delvare
@ 2013-07-08 17:37 ` Yann E. MORIN
2013-07-16 14:31 ` Jean Delvare
0 siblings, 1 reply; 36+ messages in thread
From: Yann E. MORIN @ 2013-07-08 17:37 UTC (permalink / raw)
To: Jean Delvare; +Cc: linux-kbuild, linux-kernel, Michal Marek
Jean, All,
On 2013-07-08 13:25 +0200, Jean Delvare spake thusly:
> Le Monday 24 June 2013 à 20:11 +0200, Yann E. MORIN a écrit :
> > From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> >
> > Reported-by: Jean Delvare <jdelvare@suse.de>
> > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > Cc: Jean Delvare <jdelvare@suse.de>
> > Cc: Michal Marek <mmarek@suse.cz>
> > ---
> > scripts/kconfig/mconf.c | 2 +-
> > scripts/kconfig/nconf.c | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c
> > index 6ee4aae..18d3dc9 100644
> > --- a/scripts/kconfig/mconf.c
> > +++ b/scripts/kconfig/mconf.c
> > @@ -401,7 +401,7 @@ static void search_conf(void)
> > struct subtitle_part stpart;
> >
> > title = str_new();
> > - str_printf( &title, _("Enter %s (sub)string to search for "
> > + str_printf( &title, _("Enter %s (sub)string or regexp to search for "
> > "(with or without \"%s\")"), CONFIG_, CONFIG_);
>
> While clearer, this also makes the title span over two lines when it
> used to fit on a single one. I would like to suggest the following:
>
> str_printf(&title, _("Enter (sub)string or regexp to search for "
> "(with or without \"%s\")"), CONFIG_);
>
> Rationale: the "(with or without CONFIG_)" makes things clear enough
> IMHO, making the first occurrence redundant.
OK, I'll send another patch with the text updated with your suggestion.
Thank you! :-)
Regards,
Yann E. MORIN.
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 13/15] kconfig/[mn]conf: make it explicit in the search box that a regexp is possible
2013-07-08 17:37 ` Yann E. MORIN
@ 2013-07-16 14:31 ` Jean Delvare
2013-07-16 14:39 ` Yann E. MORIN
0 siblings, 1 reply; 36+ messages in thread
From: Jean Delvare @ 2013-07-16 14:31 UTC (permalink / raw)
To: Yann E. MORIN; +Cc: linux-kbuild, linux-kernel, Michal Marek
Hi Yann,
Le Monday 08 July 2013 à 19:37 +0200, Yann E. MORIN a écrit :
> Jean, All,
>
> On 2013-07-08 13:25 +0200, Jean Delvare spake thusly:
> > Le Monday 24 June 2013 à 20:11 +0200, Yann E. MORIN a écrit :
> > > From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > >
> > > Reported-by: Jean Delvare <jdelvare@suse.de>
> > > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > > Cc: Jean Delvare <jdelvare@suse.de>
> > > Cc: Michal Marek <mmarek@suse.cz>
> > > ---
> > > scripts/kconfig/mconf.c | 2 +-
> > > scripts/kconfig/nconf.c | 2 +-
> > > 2 files changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c
> > > index 6ee4aae..18d3dc9 100644
> > > --- a/scripts/kconfig/mconf.c
> > > +++ b/scripts/kconfig/mconf.c
> > > @@ -401,7 +401,7 @@ static void search_conf(void)
> > > struct subtitle_part stpart;
> > >
> > > title = str_new();
> > > - str_printf( &title, _("Enter %s (sub)string to search for "
> > > + str_printf( &title, _("Enter %s (sub)string or regexp to search for "
> > > "(with or without \"%s\")"), CONFIG_, CONFIG_);
> >
> > While clearer, this also makes the title span over two lines when it
> > used to fit on a single one. I would like to suggest the following:
> >
> > str_printf(&title, _("Enter (sub)string or regexp to search for "
> > "(with or without \"%s\")"), CONFIG_);
> >
> > Rationale: the "(with or without CONFIG_)" makes things clear enough
> > IMHO, making the first occurrence redundant.
>
> OK, I'll send another patch with the text updated with your suggestion.
Did you? I can't remember seeing it.
--
Jean Delvare
Suse L3
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 13/15] kconfig/[mn]conf: make it explicit in the search box that a regexp is possible
2013-07-16 14:31 ` Jean Delvare
@ 2013-07-16 14:39 ` Yann E. MORIN
0 siblings, 0 replies; 36+ messages in thread
From: Yann E. MORIN @ 2013-07-16 14:39 UTC (permalink / raw)
To: Jean Delvare; +Cc: linux-kbuild, linux-kernel, Michal Marek
Jean, All,
On Tuesday 16 July 2013 16:31:20 Jean Delvare wrote:
> Le Monday 08 July 2013 à 19:37 +0200, Yann E. MORIN a écrit :
> > On 2013-07-08 13:25 +0200, Jean Delvare spake thusly:
> > > Le Monday 24 June 2013 à 20:11 +0200, Yann E. MORIN a écrit :
[--SNIP--]
> > > > - str_printf( &title, _("Enter %s (sub)string to search for "
> > > > + str_printf( &title, _("Enter %s (sub)string or regexp to search for "
> > > > "(with or without \"%s\")"), CONFIG_, CONFIG_);
> > >
> > > While clearer, this also makes the title span over two lines when it
> > > used to fit on a single one. I would like to suggest the following:
> > >
> > > str_printf(&title, _("Enter (sub)string or regexp to search for "
> > > "(with or without \"%s\")"), CONFIG_);
> > >
> > > Rationale: the "(with or without CONFIG_)" makes things clear enough
> > > IMHO, making the first occurrence redundant.
> >
> > OK, I'll send another patch with the text updated with your suggestion.
>
> Did you? I can't remember seeing it.
Doh! I forgot that one... Will do.
Regards,
Yann E. MORIN.
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +0/33 662376056 | Software Designer | \ / CAMPAIGN | ^ |
| --==< O_o >==-- '------------.-------: X AGAINST | /e\ There is no |
| http://ymorin.is-a-geek.org/ | (*_*) | / \ HTML MAIL | """ conspiracy. |
'------------------------------'-------'------------------'--------------------'
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 14/15] kconfig: loop as long as we changed some symbols in randconfig
2013-06-24 18:11 [pull request v2] Pull request for branch yem-kconfig-for-next Yann E. MORIN
` (12 preceding siblings ...)
2013-06-24 18:11 ` [PATCH 13/15] kconfig/[mn]conf: make it explicit in the search box that a regexp is possible Yann E. MORIN
@ 2013-06-24 18:11 ` Yann E. MORIN
2013-06-24 18:11 ` [PATCH 15/15] kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG Yann E. MORIN
2013-06-24 21:42 ` [pull request v2] Pull request for branch yem-kconfig-for-next Michal Marek
15 siblings, 0 replies; 36+ messages in thread
From: Yann E. MORIN @ 2013-06-24 18:11 UTC (permalink / raw)
To: linux-kbuild; +Cc: linux-kernel, Michal Marek, Yann E. MORIN
From: "Yann E. MORIN" <yann.morin.1998@free.fr>
Because of choice-in-a-choice constructs, it can happen that not all
symbols are assigned a value during randconfig, leading in rare cases
to this situation:
---8<--- choice-in-choice.in
choice
bool "A/B/C"
config A
bool "A"
config B
bool "B"
if B
choice
bool "E/F"
config E
bool "E"
config F
bool "F"
endchoice
endif # B
config C
bool "C"
endchoice
---8<---
$ ./scripts/kconfig/conf --randconfig choice-in-choice.in
[--SNIP--]
$ ./scripts/kconfig/conf --silentoldconfig choice-in-choice.in </dev/null
[--SNIP--]
A/B/C
1. A (A)
> 2. B (B)
3. C (C)
choice[1-3]: 2
E/F
> 1. E (E) (NEW)
2. F (F) (NEW)
choice[1-2]: aborted!
Console input/output is redirected. Run 'make oldconfig' to update
configuration.
Fix this by looping in randconfig for as long as some symbol gets assigned
a value.
Note: this was spotted with the USB EHCI Debug Device Gadget (USB_G_DBGP),
which uses this choice-in-a-choice construct, and exhibits this problem.
The example above is just a stripped-down minimalist test-case.
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
scripts/kconfig/conf.c | 3 ++-
scripts/kconfig/confdata.c | 18 ++++++++++++++----
scripts/kconfig/lkc.h | 2 +-
3 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
index 38616c1..d19944f 100644
--- a/scripts/kconfig/conf.c
+++ b/scripts/kconfig/conf.c
@@ -654,7 +654,8 @@ int main(int ac, char **av)
conf_set_all_new_symbols(def_default);
break;
case randconfig:
- conf_set_all_new_symbols(def_random);
+ /* Really nothing to do in this loop */
+ while (conf_set_all_new_symbols(def_random)) ;
break;
case defconfig:
conf_set_all_new_symbols(def_default);
diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index d36bc1f..c55c227 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -1040,7 +1040,7 @@ void conf_set_changed_callback(void (*fn)(void))
conf_changed_callback = fn;
}
-static void randomize_choice_values(struct symbol *csym)
+static bool randomize_choice_values(struct symbol *csym)
{
struct property *prop;
struct symbol *sym;
@@ -1053,7 +1053,7 @@ static void randomize_choice_values(struct symbol *csym)
* In both cases stop.
*/
if (csym->curr.tri != yes)
- return;
+ return false;
prop = sym_get_choice_prop(csym);
@@ -1084,6 +1084,8 @@ static void randomize_choice_values(struct symbol *csym)
csym->flags |= SYMBOL_DEF_USER;
/* clear VALID to get value calculated */
csym->flags &= ~(SYMBOL_VALID);
+
+ return true;
}
void set_all_choice_values(struct symbol *csym)
@@ -1106,7 +1108,7 @@ void set_all_choice_values(struct symbol *csym)
csym->flags &= ~(SYMBOL_VALID | SYMBOL_NEED_SET_CHOICE_VALUES);
}
-void conf_set_all_new_symbols(enum conf_def_mode mode)
+bool conf_set_all_new_symbols(enum conf_def_mode mode)
{
struct symbol *sym, *csym;
int i, cnt, pby, pty, ptm; /* pby: probability of boolean = y
@@ -1154,6 +1156,7 @@ void conf_set_all_new_symbols(enum conf_def_mode mode)
exit( 1 );
}
}
+ bool has_changed = false;
for_all_symbols(i, sym) {
if (sym_has_value(sym) || (sym->flags & SYMBOL_VALID))
@@ -1161,6 +1164,7 @@ void conf_set_all_new_symbols(enum conf_def_mode mode)
switch (sym_get_type(sym)) {
case S_BOOLEAN:
case S_TRISTATE:
+ has_changed = true;
switch (mode) {
case def_yes:
sym->def[S_DEF_USER].tri = yes;
@@ -1219,6 +1223,12 @@ void conf_set_all_new_symbols(enum conf_def_mode mode)
sym_calc_value(csym);
if (mode == def_random)
- randomize_choice_values(csym);
+ has_changed = randomize_choice_values(csym);
+ else {
+ set_all_choice_values(csym);
+ has_changed = true;
+ }
}
+
+ return has_changed;
}
diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
index 0c8d419..09f4edf 100644
--- a/scripts/kconfig/lkc.h
+++ b/scripts/kconfig/lkc.h
@@ -86,7 +86,7 @@ const char *conf_get_autoconfig_name(void);
char *conf_get_default_confname(void);
void sym_set_change_count(int count);
void sym_add_change_count(int count);
-void conf_set_all_new_symbols(enum conf_def_mode mode);
+bool conf_set_all_new_symbols(enum conf_def_mode mode);
void set_all_choice_values(struct symbol *csym);
struct conf_printer {
--
1.8.1.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 15/15] kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG
2013-06-24 18:11 [pull request v2] Pull request for branch yem-kconfig-for-next Yann E. MORIN
` (13 preceding siblings ...)
2013-06-24 18:11 ` [PATCH 14/15] kconfig: loop as long as we changed some symbols in randconfig Yann E. MORIN
@ 2013-06-24 18:11 ` Yann E. MORIN
2013-06-25 8:01 ` Yann E. MORIN
2013-06-25 20:58 ` Yann E. MORIN
2013-06-24 21:42 ` [pull request v2] Pull request for branch yem-kconfig-for-next Michal Marek
15 siblings, 2 replies; 36+ messages in thread
From: Yann E. MORIN @ 2013-06-24 18:11 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Michal Marek, Yann E. MORIN, Thomas Petazzoni,
Sam Ravnborg, Sedat Dilek, Arnd Bergmann, Stephen Rothwell
From: "Yann E. MORIN" <yann.morin.1998@free.fr>
Currently, randconfig does randomise choice entries, unless KCONFIG_ALLCONFIG
is specified.
For example, given those two files (Thomas' test-case):
---8<--- Config.test.in
config OPTIONA
bool "Option A"
choice
prompt "This is a choice"
config CHOICE_OPTIONA
bool "Choice Option A"
config CHOICE_OPTIONB
bool "Choice Option B"
endchoice
config OPTIONB
bool "Option B"
---8<--- Config.test.in
---8<--- config.defaults
CONFIG_OPTIONA=y
---8<--- config.defaults
And running:
./scripts/kconfig/conf --randconfig Config.test.in
does properly randomise the two choice symbols (and the two booleans).
However, running:
KCONFIG_ALLCONFIG=config.defaults \
./scripts/kconfig/conf --randconfig Config.test.in
does *not* reandomise the two choice entries, and only CHOICE_OPTIONA
will ever be selected. (OPTIONA will always be set (expected), and
OPTIONB will be be properly randomised (expected).)
This patch defers setting that a choice has a value until a symbol for
that choice is indeed set, so that choices are properly randomised when
KCONFIG_ALLCONFIG is set, but not if a symbol for that choice is set.
Reported-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Michal Marek <mmarek@suse.cz>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Sedat Dilek <sedat.dilek@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Stephen Rothwell <sfr@canb.auug.org.au>
---
Changes v3 -> v4
- fix previous issue where some choices would not be set, which would
cause silentoldconfig to ask for them and was then breaking this
workflow (as reported by Arnd and Sedat):
KCONFIG_ALLCONFIG=foo.defconfig make randconfig
make silentoldconfig </dev/nullo
which I have tested (3h28min!) with:
touch defconfig
for(( i=0; i<10000; i++ )); do
KCONFIG_ALLCONFIG=$(pwd)/defconfig make randconfig >/dev/null 2>&1
make silentoldconfig </dev/null >/dev/null 2>&1 || break
done
which did not break at all.
- change done in v3 (below) is already fixed by a previous patch
Changes v2 -> v3
- ensure only one symbol is set in a choice
Changes v1 -> v2:
- further postpone setting that a choice has a value until
one is indeed set
- do not print symbols that are part of an invisible choice
---
scripts/kconfig/confdata.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index c55c227..3e39208 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -288,8 +288,6 @@ load:
for_all_symbols(i, sym) {
sym->flags |= SYMBOL_CHANGED;
sym->flags &= ~(def_flags|SYMBOL_VALID);
- if (sym_is_choice(sym))
- sym->flags |= def_flags;
switch (sym->type) {
case S_INT:
case S_HEX:
@@ -379,13 +377,13 @@ setsym:
case mod:
if (cs->def[def].tri == yes) {
conf_warning("%s creates inconsistent choice state", sym->name);
- cs->flags &= ~def_flags;
}
break;
case yes:
if (cs->def[def].tri != no)
conf_warning("override: %s changes choice state", sym->name);
cs->def[def].val = sym;
+ cs->flags |= def_flags;
break;
}
cs->def[def].tri = EXPR_OR(cs->def[def].tri, sym->def[def].tri);
@@ -791,6 +789,8 @@ int conf_write(const char *name)
sym_calc_value(sym);
if (!(sym->flags & SYMBOL_WRITE))
goto next;
+ if (sym_is_choice_value(sym) && !menu_is_visible(menu->parent))
+ goto next;
sym->flags &= ~SYMBOL_WRITE;
conf_write_symbol(out, sym, &kconfig_printer_cb, NULL);
--
1.8.1.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 15/15] kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG
2013-06-24 18:11 ` [PATCH 15/15] kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG Yann E. MORIN
@ 2013-06-25 8:01 ` Yann E. MORIN
2013-06-25 8:10 ` Sedat Dilek
2013-06-25 20:58 ` Yann E. MORIN
1 sibling, 1 reply; 36+ messages in thread
From: Yann E. MORIN @ 2013-06-25 8:01 UTC (permalink / raw)
To: linux-kbuild, Michal Marek
Cc: linux-kernel, Thomas Petazzoni, Sam Ravnborg, Sedat Dilek,
Arnd Bergmann, Stephen Rothwell, Fengguang Wu
Michal, All,
On Monday 24 June 2013 20:11:59 Yann E. MORIN wrote:
> Currently, randconfig does randomise choice entries, unless KCONFIG_ALLCONFIG
> is specified.
[--SNIP--]
> This patch defers setting that a choice has a value until a symbol for
> that choice is indeed set, so that choices are properly randomised when
> KCONFIG_ALLCONFIG is set, but not if a symbol for that choice is set.
I've just been informed by Fengguang that this patch breaks yet another
use-case of his, but this time KCONFIG_ALLCONFIG is not involved.
I'll investigate this issue when I'm back home tonight (ie. ~16:00 UTC).
Sorry for the breakage (again...). :-(
We really need a known, shared test-suite for kconfig... :-/
Regards,
Yann E. MORIN.
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +0/33 662376056 | Software Designer | \ / CAMPAIGN | ^ |
| --==< O_o >==-- '------------.-------: X AGAINST | /e\ There is no |
| http://ymorin.is-a-geek.org/ | (*_*) | / \ HTML MAIL | """ conspiracy. |
'------------------------------'-------'------------------'--------------------'
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 15/15] kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG
2013-06-25 8:01 ` Yann E. MORIN
@ 2013-06-25 8:10 ` Sedat Dilek
0 siblings, 0 replies; 36+ messages in thread
From: Sedat Dilek @ 2013-06-25 8:10 UTC (permalink / raw)
To: Yann E. MORIN
Cc: linux-kbuild, Michal Marek, linux-kernel, Thomas Petazzoni,
Sam Ravnborg, Arnd Bergmann, Stephen Rothwell, Fengguang Wu,
Alexander Kriegisch
On Tue, Jun 25, 2013 at 10:01 AM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> Michal, All,
>
> On Monday 24 June 2013 20:11:59 Yann E. MORIN wrote:
>> Currently, randconfig does randomise choice entries, unless KCONFIG_ALLCONFIG
>> is specified.
> [--SNIP--]
>> This patch defers setting that a choice has a value until a symbol for
>> that choice is indeed set, so that choices are properly randomised when
>> KCONFIG_ALLCONFIG is set, but not if a symbol for that choice is set.
>
> I've just been informed by Fengguang that this patch breaks yet another
> use-case of his, but this time KCONFIG_ALLCONFIG is not involved.
>
> I'll investigate this issue when I'm back home tonight (ie. ~16:00 UTC).
> Sorry for the breakage (again...). :-(
>
> We really need a known, shared test-suite for kconfig... :-/
>
[ CC Alexander ]
A-propos, "testing Kconfig" I recommended to take into account the
script of Alexander.
In PM we discussed on where to place such scripts and that Alexander's
patch needs some refreshing in your eyes.
Maybe, Alexander can/will help as he is the original author.
As said, this script helped in the Freetz router project to eliminate
dozens of (logically) wrong Kconfig settings.
IMHO there exist a lot of more, as there are "double/triple" checks in
the Freetz-buildsystem, but hey noone is perfect :-).
- Sedat -
[1] http://freetz.org/browser/trunk/tools/developer/create-kconfig-warnings
> Regards,
> Yann E. MORIN.
>
> --
> .-----------------.--------------------.------------------.--------------------.
> | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
> | +0/33 662376056 | Software Designer | \ / CAMPAIGN | ^ |
> | --==< O_o >==-- '------------.-------: X AGAINST | /e\ There is no |
> | http://ymorin.is-a-geek.org/ | (*_*) | / \ HTML MAIL | """ conspiracy. |
> '------------------------------'-------'------------------'--------------------'
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 15/15] kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG
2013-06-24 18:11 ` [PATCH 15/15] kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG Yann E. MORIN
2013-06-25 8:01 ` Yann E. MORIN
@ 2013-06-25 20:58 ` Yann E. MORIN
2013-06-25 21:37 ` [PATCH] Revert "kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG" Yann E. MORIN
1 sibling, 1 reply; 36+ messages in thread
From: Yann E. MORIN @ 2013-06-25 20:58 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Michal Marek, Thomas Petazzoni, Sam Ravnborg,
Sedat Dilek, Arnd Bergmann, Stephen Rothwell, Fengguang Wu,
Alexandre Bounine, Matt Porter
Michal, All,
On 2013-06-24 20:11 +0200, Yann E. MORIN spake thusly:
> Currently, randconfig does randomise choice entries, unless KCONFIG_ALLCONFIG
> is specified.
[--SNIP--]
> This patch defers setting that a choice has a value until a symbol for
> that choice is indeed set, so that choices are properly randomised when
> KCONFIG_ALLCONFIG is set, but not if a symbol for that choice is set.
OK, so this second attempt also broke standard workflows. Fixing it is
more involved than expected. Let's get rid of it before too many people
complain, and before it hits mainline.
Michal, can you strip this from your tree (since it's the HEAD of your
kconfig branch), or do you want me to submit a revert patch?
Regards,
Yann E. MORIN.
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH] Revert "kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG"
2013-06-25 20:58 ` Yann E. MORIN
@ 2013-06-25 21:37 ` Yann E. MORIN
2013-06-26 13:48 ` Michal Marek
0 siblings, 1 reply; 36+ messages in thread
From: Yann E. MORIN @ 2013-06-25 21:37 UTC (permalink / raw)
To: linux-kbuild
Cc: Yann E. MORIN, Michal Marek, Fengguang Wu, Sedat Dilek,
Sam Ravnborg, Stephen Rothwell, Alexandre Bounine, Matt Porter
From: "Yann E. MORIN" <yann.morin.1998@free.fr>
This reverts commit 8357b48549e17b3e4e402c7f977b65708922e60f.
It breaks more stuff than it fixes.
Reported-by: Fengguang Wu <fengguang.wu@intel.com>
Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Michal Marek <mmarek@suse.cz>
Cc: Fengguang Wu <fengguang.wu@intel.com>
Cc: Sedat Dilek <sedat.dilek@gmail.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Stephen Rothwell <sfr@canb.auug.org.au>
Cc: Alexandre Bounine <alexandre.bounine@idt.com>
Cc: Matt Porter <mporter@kernel.crashing.org>
---
Michal, here is the revert patch if you want it.
Regards,
Yann E. MORIN.
---
scripts/kconfig/confdata.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index 3e39208..c55c227 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -288,6 +288,8 @@ load:
for_all_symbols(i, sym) {
sym->flags |= SYMBOL_CHANGED;
sym->flags &= ~(def_flags|SYMBOL_VALID);
+ if (sym_is_choice(sym))
+ sym->flags |= def_flags;
switch (sym->type) {
case S_INT:
case S_HEX:
@@ -377,13 +379,13 @@ setsym:
case mod:
if (cs->def[def].tri == yes) {
conf_warning("%s creates inconsistent choice state", sym->name);
+ cs->flags &= ~def_flags;
}
break;
case yes:
if (cs->def[def].tri != no)
conf_warning("override: %s changes choice state", sym->name);
cs->def[def].val = sym;
- cs->flags |= def_flags;
break;
}
cs->def[def].tri = EXPR_OR(cs->def[def].tri, sym->def[def].tri);
@@ -789,8 +791,6 @@ int conf_write(const char *name)
sym_calc_value(sym);
if (!(sym->flags & SYMBOL_WRITE))
goto next;
- if (sym_is_choice_value(sym) && !menu_is_visible(menu->parent))
- goto next;
sym->flags &= ~SYMBOL_WRITE;
conf_write_symbol(out, sym, &kconfig_printer_cb, NULL);
--
1.8.1.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH] Revert "kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG"
2013-06-25 21:37 ` [PATCH] Revert "kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG" Yann E. MORIN
@ 2013-06-26 13:48 ` Michal Marek
2013-06-26 21:44 ` Yann E. MORIN
0 siblings, 1 reply; 36+ messages in thread
From: Michal Marek @ 2013-06-26 13:48 UTC (permalink / raw)
To: Yann E. MORIN
Cc: linux-kbuild, Fengguang Wu, Sedat Dilek, Sam Ravnborg,
Stephen Rothwell, Alexandre Bounine, Matt Porter
On Tue, Jun 25, 2013 at 11:37:44PM +0200, Yann E. MORIN wrote:
> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
>
> This reverts commit 8357b48549e17b3e4e402c7f977b65708922e60f.
>
> It breaks more stuff than it fixes.
Thanks, I applied it to kbuild.git#kconfig, after verifying that the
issue is gone.
Michal
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] Revert "kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG"
2013-06-26 13:48 ` Michal Marek
@ 2013-06-26 21:44 ` Yann E. MORIN
2013-06-27 6:23 ` Sedat Dilek
0 siblings, 1 reply; 36+ messages in thread
From: Yann E. MORIN @ 2013-06-26 21:44 UTC (permalink / raw)
To: Michal Marek; +Cc: linux-kbuild, Sedat Dilek, Sam Ravnborg
Michal, All,
On 2013-06-26 15:48 +0200, Michal Marek spake thusly:
> On Tue, Jun 25, 2013 at 11:37:44PM +0200, Yann E. MORIN wrote:
> > From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> >
> > This reverts commit 8357b48549e17b3e4e402c7f977b65708922e60f.
> >
> > It breaks more stuff than it fixes.
>
> Thanks, I applied it to kbuild.git#kconfig, after verifying that the
> issue is gone.
Yes, thanks. I'm sorry this second attempt was a (slightly different!)
failure again. :-(
The fix is working for boolean choices, but exposes a latent bug on
tristate changes. Obviously, we must fix that latent bug first, but I
can't come to a sane solution.
I'll let it mature in a corner of my head for one more night. Maybe I'll
see things clearer tomorrow. ;-)
Regards,
Yann E. MORIN.
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] Revert "kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG"
2013-06-26 21:44 ` Yann E. MORIN
@ 2013-06-27 6:23 ` Sedat Dilek
2013-06-28 17:19 ` Yann E. MORIN
0 siblings, 1 reply; 36+ messages in thread
From: Sedat Dilek @ 2013-06-27 6:23 UTC (permalink / raw)
To: Yann E. MORIN; +Cc: Michal Marek, linux-kbuild, Sam Ravnborg
On Wed, Jun 26, 2013 at 11:44 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> Michal, All,
>
> On 2013-06-26 15:48 +0200, Michal Marek spake thusly:
>> On Tue, Jun 25, 2013 at 11:37:44PM +0200, Yann E. MORIN wrote:
>> > From: "Yann E. MORIN" <yann.morin.1998@free.fr>
>> >
>> > This reverts commit 8357b48549e17b3e4e402c7f977b65708922e60f.
>> >
>> > It breaks more stuff than it fixes.
>>
>> Thanks, I applied it to kbuild.git#kconfig, after verifying that the
>> issue is gone.
>
> Yes, thanks. I'm sorry this second attempt was a (slightly different!)
> failure again. :-(
>
> The fix is working for boolean choices, but exposes a latent bug on
> tristate changes. Obviously, we must fix that latent bug first, but I
> can't come to a sane solution.
>
> I'll let it mature in a corner of my head for one more night. Maybe I'll
> see things clearer tomorrow. ;-)
>
Just as a side-effect:
Yesterday, I did some git-bisecting which has shown a char-misc-next issue.
Unfortunately, I had to revert this change in several runs to do a
sane bisecting.
NOTE:
Can you please write up some cooler changelog next time why this
revert was done?
In one of your emails you did some nice analysis.
Such informations belong IMHO into the revert's changelog.
- Sedat -
> Regards,
> Yann E. MORIN.
>
> --
> .-----------------.--------------------.------------------.--------------------.
> | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
> | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
> | +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no |
> | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
> '------------------------------^-------^------------------^--------------------'
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] Revert "kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG"
2013-06-27 6:23 ` Sedat Dilek
@ 2013-06-28 17:19 ` Yann E. MORIN
0 siblings, 0 replies; 36+ messages in thread
From: Yann E. MORIN @ 2013-06-28 17:19 UTC (permalink / raw)
To: Sedat Dilek; +Cc: Michal Marek, linux-kbuild, Sam Ravnborg
Sedat, All,
On 2013-06-27 08:23 +0200, Sedat Dilek spake thusly:
> On Wed, Jun 26, 2013 at 11:44 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> > Michal, All,
> >
> > On 2013-06-26 15:48 +0200, Michal Marek spake thusly:
> >> On Tue, Jun 25, 2013 at 11:37:44PM +0200, Yann E. MORIN wrote:
> >> > From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> >> >
> >> > This reverts commit 8357b48549e17b3e4e402c7f977b65708922e60f.
> >> >
> >> > It breaks more stuff than it fixes.
> >>
> >> Thanks, I applied it to kbuild.git#kconfig, after verifying that the
> >> issue is gone.
> >
> > Yes, thanks. I'm sorry this second attempt was a (slightly different!)
> > failure again. :-(
[--SNIP--]
> Can you please write up some cooler changelog next time why this
> revert was done?
> In one of your emails you did some nice analysis.
> Such informations belong IMHO into the revert's changelog.
Yes, you are right, I'll do next time I'll have to send a revert patch
(which I hope not to have to for some time! ;-) )
Regards,
Yann E. MORIN.
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [pull request v2] Pull request for branch yem-kconfig-for-next
2013-06-24 18:11 [pull request v2] Pull request for branch yem-kconfig-for-next Yann E. MORIN
` (14 preceding siblings ...)
2013-06-24 18:11 ` [PATCH 15/15] kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG Yann E. MORIN
@ 2013-06-24 21:42 ` Michal Marek
15 siblings, 0 replies; 36+ messages in thread
From: Michal Marek @ 2013-06-24 21:42 UTC (permalink / raw)
To: Yann E. MORIN; +Cc: linux-kbuild, linux-kernel
Dne 24.6.2013 20:11, Yann E. MORIN napsal(a):
> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
>
> Hello Michal, All!
> │
> Michal, please pull these patches against kconfig that I have accumulated
> for 3.11.
>
> Note-whorthy this time:
> - fix values of tristates that are selected by boolean choices (Arve)
> - fix choice randomisation in presence of KCONFIG_ALLCONFIG (me)
> - fix choice randomisation selecting more than one value in
> a choice (but only if it is conditional) (me)
> - fix choice-in-a-choice randomisation not selecting any value
> for the inner-most choice (me)
>
> Also, some code-cleanups and eye-candy:
> - mconf and nconf code cleanups (Dirk, Sedat)
> - mconf and nconf eye-candy (Dirk, me)
> - scripts/config script-name in help text (Clément)
> - heuristic to sort found symbols by relevance (me)
> - more randconfig debugging help (me)
>
> Changes v1 -> v2:
> - simplify sorting heuristic (Jean)
> - make it clear in mconf/nconf that the search expression can
> be a regexp (Jean)
Pulled, thanks a lot!
Michal
^ permalink raw reply [flat|nested] 36+ messages in thread