| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJFTR8QzTPzyuKfEpkBP-2f3o5zPCLHvALWk2RFfAfXeiK10Yw@mail.gmail.com>
Date: Fri, 30 Jun 2023 23:57:57 -0400
From: Jesse T <mr.bossman075@...il.com>
To: Masahiro Yamada <masahiroy@...nel.org>
Cc: linux-kbuild@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] kconfig: menuconfig: simplify global jump key assignment
On Thu, Jun 29, 2023 at 12:03 PM Masahiro Yamada <masahiroy@...nel.org> wrote:
>
> Commit 95ac9b3b585d ("menuconfig: Assign jump keys per-page instead
> of globally") injects a lot of hacks to the bottom of the textbox
> infrastructure.
>
> I reverted many of them without changing the behavior. (almost)
> Now, the key markers are inserted when constructing the search result
> instead of updating the text buffer on-the-fly.
>
> The buffer passed to the textbox got back to a constant string.
> The ugly casts from (const char *) to (char *) went away.
>
> A disadvantage is that the same key numbers might be diplayed multiple
> times in the dialog if you use a huge window (but I believe it is
> unlikely to happen).
>
> Signed-off-by: Masahiro Yamada <masahiroy@...nel.org>
> ---
>
> scripts/kconfig/lkc.h | 1 +
> scripts/kconfig/lxdialog/dialog.h | 10 ++--
> scripts/kconfig/lxdialog/textbox.c | 68 +++++++++--------------
> scripts/kconfig/mconf.c | 86 +++++++++++++++++-------------
> scripts/kconfig/menu.c | 22 ++++++--
> 5 files changed, 97 insertions(+), 90 deletions(-)
>
> diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
> index e7118d62a45f..d5c27180ce91 100644
> --- a/scripts/kconfig/lkc.h
> +++ b/scripts/kconfig/lkc.h
> @@ -101,6 +101,7 @@ const char *menu_get_prompt(struct menu *menu);
> struct menu *menu_get_parent_menu(struct menu *menu);
> bool menu_has_help(struct menu *menu);
> const char *menu_get_help(struct menu *menu);
> +int get_jump_key(void);
> struct gstr get_relations_str(struct symbol **sym_arr, struct list_head *head);
> void menu_get_ext_help(struct menu *menu, struct gstr *help);
>
> diff --git a/scripts/kconfig/lxdialog/dialog.h b/scripts/kconfig/lxdialog/dialog.h
> index 347daf25fdc8..cd1b59c24b21 100644
> --- a/scripts/kconfig/lxdialog/dialog.h
> +++ b/scripts/kconfig/lxdialog/dialog.h
> @@ -196,13 +196,9 @@ int first_alpha(const char *string, const char *exempt);
> int dialog_yesno(const char *title, const char *prompt, int height, int width);
> int dialog_msgbox(const char *title, const char *prompt, int height,
> int width, int pause);
> -
> -
> -typedef void (*update_text_fn)(char *buf, size_t start, size_t end, void
> - *_data);
> -int dialog_textbox(const char *title, char *tbuf, int initial_height,
> - int initial_width, int *keys, int *_vscroll, int *_hscroll,
> - update_text_fn update_text, void *data);
> +int dialog_textbox(const char *title, const char *tbuf, int initial_height,
> + int initial_width, int *_vscroll, int *_hscroll,
> + int (*extra_key_cb)(int, int, int, void *), void *data);
> int dialog_menu(const char *title, const char *prompt,
> const void *selected, int *s_scroll);
> int dialog_checklist(const char *title, const char *prompt, int height,
> diff --git a/scripts/kconfig/lxdialog/textbox.c b/scripts/kconfig/lxdialog/textbox.c
> index bc4d4fb1dc75..e6cd7bb83746 100644
> --- a/scripts/kconfig/lxdialog/textbox.c
> +++ b/scripts/kconfig/lxdialog/textbox.c
> @@ -10,8 +10,8 @@
>
> static int hscroll;
> static int begin_reached, end_reached, page_length;
> -static char *buf;
> -static char *page;
> +static const char *buf, *page;
> +static int start, end;
>
> /*
> * Go back 'n' lines in text. Called by dialog_textbox().
> @@ -98,21 +98,10 @@ static void print_line(WINDOW *win, int row, int width)
> /*
> * Print a new page of text.
> */
> -static void print_page(WINDOW *win, int height, int width, update_text_fn
> - update_text, void *data)
> +static void print_page(WINDOW *win, int height, int width)
> {
> int i, passed_end = 0;
>
> - if (update_text) {
> - char *end;
> -
> - for (i = 0; i < height; i++)
> - get_line();
> - end = page;
> - back_lines(height);
> - update_text(buf, page - buf, end - buf, data);
> - }
> -
> page_length = 0;
> for (i = 0; i < height; i++) {
> print_line(win, i, width);
> @@ -142,24 +131,26 @@ static void print_position(WINDOW *win)
> * refresh window content
> */
> static void refresh_text_box(WINDOW *dialog, WINDOW *box, int boxh, int boxw,
> - int cur_y, int cur_x, update_text_fn update_text,
> - void *data)
> + int cur_y, int cur_x)
The change for refresh_text_box is very large.
Is there an easy way to split the change of `refresh_text_box` and
everything else
while still maintaining bisectability?
> {
> - print_page(box, boxh, boxw, update_text, data);
> + start = page - buf;
> +
> + print_page(box, boxh, boxw);
> print_position(dialog);
> wmove(dialog, cur_y, cur_x); /* Restore cursor position */
> wrefresh(dialog);
> +
> + end = page - buf;
> }
>
> /*
> * Display text from a file in a dialog box.
> *
> * keys is a null-terminated array
> - * update_text() may not add or remove any '\n' or '\0' in tbuf
> */
> -int dialog_textbox(const char *title, char *tbuf, int initial_height,
> - int initial_width, int *keys, int *_vscroll, int *_hscroll,
> - update_text_fn update_text, void *data)
> +int dialog_textbox(const char *title, const char *tbuf, int initial_height,
> + int initial_width, int *_vscroll, int *_hscroll,
> + int (*extra_key_cb)(int, int, int, void *), void *data)
> {
> int i, x, y, cur_x, cur_y, key = 0;
> int height, width, boxh, boxw;
> @@ -239,8 +230,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
>
> /* Print first page of text */
> attr_clear(box, boxh, boxw, dlg.dialog.atr);
> - refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x, update_text,
> - data);
> + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
>
> while (!done) {
> key = wgetch(dialog);
> @@ -259,8 +249,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> begin_reached = 1;
> page = buf;
> refresh_text_box(dialog, box, boxh, boxw,
> - cur_y, cur_x, update_text,
> - data);
> + cur_y, cur_x);
> }
> break;
> case 'G': /* Last page */
> @@ -270,8 +259,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> /* point to last char in buf */
> page = buf + strlen(buf);
> back_lines(boxh);
> - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> - cur_x, update_text, data);
> + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> break;
> case 'K': /* Previous line */
> case 'k':
> @@ -280,8 +268,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> break;
>
> back_lines(page_length + 1);
> - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> - cur_x, update_text, data);
> + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> break;
> case 'B': /* Previous page */
> case 'b':
> @@ -290,8 +277,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> if (begin_reached)
> break;
> back_lines(page_length + boxh);
> - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> - cur_x, update_text, data);
> + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> break;
> case 'J': /* Next line */
> case 'j':
> @@ -300,8 +286,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> break;
>
> back_lines(page_length - 1);
> - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> - cur_x, update_text, data);
> + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> break;
> case KEY_NPAGE: /* Next page */
> case ' ':
> @@ -310,8 +295,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> break;
>
> begin_reached = 0;
> - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> - cur_x, update_text, data);
> + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> break;
> case '0': /* Beginning of line */
> case 'H': /* Scroll left */
> @@ -326,8 +310,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> hscroll--;
> /* Reprint current page to scroll horizontally */
> back_lines(page_length);
> - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> - cur_x, update_text, data);
> + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> break;
> case 'L': /* Scroll right */
> case 'l':
> @@ -337,8 +320,7 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> hscroll++;
> /* Reprint current page to scroll horizontally */
> back_lines(page_length);
> - refresh_text_box(dialog, box, boxh, boxw, cur_y,
> - cur_x, update_text, data);
> + refresh_text_box(dialog, box, boxh, boxw, cur_y, cur_x);
> break;
> case KEY_ESC:
> if (on_key_esc(dialog) == KEY_ESC)
> @@ -351,11 +333,9 @@ int dialog_textbox(const char *title, char *tbuf, int initial_height,
> on_key_resize();
> goto do_resize;
> default:
> - for (i = 0; keys[i]; i++) {
> - if (key == keys[i]) {
> - done = true;
> - break;
> - }
> + if (extra_key_cb(key, start, end, data)) {
`extra_key_cb` is null when not used, on the help page this will segfault.
if (extra_key_cb && extra_key_cb(key, start, end, data)) {
> + done = true;
> + break;
> }
> }
> }
> diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c
> index 53d8834d12fe..7adfd6537279 100644
> --- a/scripts/kconfig/mconf.c
> +++ b/scripts/kconfig/mconf.c
> @@ -288,6 +288,7 @@ static int single_menu_mode;
> static int show_all_options;
> static int save_and_exit;
> static int silent;
> +static int jump_key;
>
> static void conf(struct menu *menu, struct menu *active_menu);
>
> @@ -348,19 +349,19 @@ static void reset_subtitle(void)
> set_dialog_subtitles(subtitles);
> }
>
> -static int show_textbox_ext(const char *title, char *text, int r, int c, int
> - *keys, int *vscroll, int *hscroll, update_text_fn
> - update_text, void *data)
> +static int show_textbox_ext(const char *title, const char *text, int r, int c,
> + int *vscroll, int *hscroll,
> + int (*extra_key_cb)(int, int, int, void *),
> + void *data)
> {
> dialog_clear();
> - return dialog_textbox(title, text, r, c, keys, vscroll, hscroll,
> - update_text, data);
> + return dialog_textbox(title, text, r, c, vscroll, hscroll,
> + extra_key_cb, data);
> }
>
> static void show_textbox(const char *title, const char *text, int r, int c)
> {
> - show_textbox_ext(title, (char *) text, r, c, (int []) {0}, NULL, NULL,
> - NULL, NULL);
> + show_textbox_ext(title, text, r, c, NULL, NULL, NULL, NULL);
> }
>
> static void show_helptext(const char *title, const char *text)
> @@ -381,35 +382,51 @@ static void show_help(struct menu *menu)
>
> struct search_data {
> struct list_head *head;
> - struct menu **targets;
> - int *keys;
> + struct menu *target;
> };
>
> -static void update_text(char *buf, size_t start, size_t end, void *_data)
> +static int next_key(int key)
> +{
> + key++;
> +
> + if (key > '9')
> + key = '1';
> +
> + return key;
> +}
> +
> +static int handle_search_keys(int key, int start, int end, void *_data)
> {
> struct search_data *data = _data;
> struct jump_key *pos;
> - int k = 0;
> +
> + if (key < '1' || key > '9')
> + return 0;
>
> list_for_each_entry(pos, data->head, entries) {
> - if (pos->offset >= start && pos->offset < end) {
> - char header[4];
> + if (pos->offset >= start) {
> + if (pos->offset >= end)
> + break;
>
> - if (k < JUMP_NB) {
> - int key = '0' + (pos->index % JUMP_NB) + 1;
> -
> - sprintf(header, "(%c)", key);
> - data->keys[k] = key;
> - data->targets[k] = pos->target;
> - k++;
> - } else {
> - sprintf(header, " ");
> + if (key == '1' + (pos->index % JUMP_NB)) {
> + data->target = pos->target;
> + return 1;
> }
> -
> - memcpy(buf + pos->offset, header, sizeof(header) - 1);
> }
> }
> - data->keys[k] = 0;
> +
> + return 0;
> +}
> +
> +int get_jump_key(void)
> +{
> + int cur_key;
> +
> + cur_key = jump_key;
> +
> + jump_key = next_key(cur_key);
> +
> + return cur_key;
> }
>
> static void search_conf(void)
> @@ -456,26 +473,23 @@ static void search_conf(void)
> sym_arr = sym_re_search(dialog_input);
> do {
> LIST_HEAD(head);
> - struct menu *targets[JUMP_NB];
> - int keys[JUMP_NB + 1], i;
> struct search_data data = {
> .head = &head,
> - .targets = targets,
> - .keys = keys,
> };
> struct jump_key *pos, *tmp;
>
> + jump_key = '1';
> res = get_relations_str(sym_arr, &head);
> set_subtitle();
> dres = show_textbox_ext("Search Results", str_get(&res), 0, 0,
> - keys, &vscroll, &hscroll, &update_text,
> - &data);
> + &vscroll, &hscroll,
> + handle_search_keys, &data);
> again = false;
> - for (i = 0; i < JUMP_NB && keys[i]; i++)
> - if (dres == keys[i]) {
> - conf(targets[i]->parent, targets[i]);
> - again = true;
> - }
> + if (dres >= '1' && dres <= '9') {
> + assert(data.target != NULL);
> + conf(data.target->parent, data.target);
> + again = true;
> + }
> str_free(&res);
> list_for_each_entry_safe(pos, tmp, &head, entries)
> free(pos);
> diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
> index b90fff833588..5578b8bc8a23 100644
> --- a/scripts/kconfig/menu.c
> +++ b/scripts/kconfig/menu.c
> @@ -701,6 +701,11 @@ static void get_dep_str(struct gstr *r, struct expr *expr, const char *prefix)
> }
> }
>
> +int __attribute__((weak)) get_jump_key(void)
This seems like a non-optimal solution, otherwise fine.
Other than the call to null, looks good. After that change.
Reviewed-by: Jesse Taube <Mr.Bossman075@...il.com>
Thanks,
Jesse Taube
> +{
> + return -1;
> +}
> +
> static void get_prompt_str(struct gstr *r, struct property *prop,
> struct list_head *head)
> {
> @@ -743,11 +748,22 @@ static void get_prompt_str(struct gstr *r, struct property *prop,
> }
>
> str_printf(r, " Location:\n");
> - for (j = 4; --i >= 0; j += 2) {
> + for (j = 0; --i >= 0; j++) {
> + int jk = -1;
> + int indent = 2 * j + 4;
> +
> menu = submenu[i];
> - if (jump && menu == location)
> + if (jump && menu == location) {
> jump->offset = strlen(r->s);
> - str_printf(r, "%*c-> %s", j, ' ', menu_get_prompt(menu));
> + jk = get_jump_key();
> + }
> +
> + if (jk >= 0) {
> + str_printf(r, "(%c)", jk);
> + indent -= 3;
> + }
> +
> + str_printf(r, "%*c-> %s", indent, ' ', menu_get_prompt(menu));
> if (menu->sym) {
> str_printf(r, " (%s [=%s])", menu->sym->name ?
> menu->sym->name : "<choice>",
> --
> 2.39.2
>
Powered by blists - more mailing lists