Re: [RFC] nconf bug fixes and improvements

[Sam Ravnborg]

Hi Cheng.

On Mon, Aug 29, 2011 at 02:09:59AM -0700, Cheng Renquan wrote:
> bug fixes:
> 1) char dialog_input_result[256]; is not enough for config item like:
>    CONFIG_EXTRA_FIRMWARE="iwlwifi-100-5.ucode iwlwifi-1000-3.ucode
> iwlwifi-3945-2.ucode iwlwifi-4965-2.ucode iwlwifi-5000-1.ucode
> iwlwifi-5000-2.ucode iwlwifi-5150-2.ucode iwlwifi-6000-4.ucode
> iwlwifi-6000g2a-5.ucode iwlwifi-6000g2b-5.ucode iwlwifi-6050-4.ucode
> iwlwifi-6050-5.ucode radeon/CEDAR_me.bin radeon/CEDAR_pfp.bin
> radeon/CEDAR_rlc.bin radeon/CYPRESS_me.bin radeon/CYPRESS_pfp.bin
> radeon/CYPRESS_rlc.bin radeon/JUNIPER_me.bin radeon/JUNIPER_pfp.bin
> radeon/JUNIPER_rlc.bin radeon/R600_rlc.bin radeon/R700_rlc.bin
> radeon/REDWOOD_me.bin radeon/REDWOOD_pfp.bin radeon/REDWOOD_rlc.bin"
> 
>    the original nconf just stack overflow / crashed when dealing with
> longer than 256 bytes strings; Since the original menuconfig also just
> uses a fixed length buffer [MAX_LEN=2048] which works for the years,
> here I just append a 0 make it work in the easiest way; if required,
> it could also be changed to a dynamically allocated buffer;
> 
>    char dialog_input_result[MAX_LEN + 1];

Please do not repeat errors from the past.
Fix this to allocate the string so we handle strings of
unlimited length.

> 2) memmove's 3rd argument should be len-cursor_position+1, the
> original len+1 may cause segment fault in theory;
>  				memmove(&result[cursor_position+1],
>  						&result[cursor_position],
> -						len+1);
> +						len-cursor_position+1);
Looks OK - but I did not check in detail.

> 
> 3) typo:
> 
> -				mvprintw(0, 0, "unknow key: %d\n", res);
> +				mvprintw(0, 0, "unknown key: %d\n", res);
> 
ACK

> 
> improvement:
> 1) its original conf_string doesn't work with longer string values
> (longer than its dialog box width), not at all
>    (or may work in an invisible way if anyone has tried that)
>    Here I added a new variable cursor_form_win to record that new state:
>    cursor of the input box; and make it fun:
>    when you move cursor to almost left/right edge, it auto adjust text
> to center, by half prompt box width;
Sounds resonable - but did not try it.

> 
> 2) Remove KEY_UP as RIGHT and KEY_DOWN as LEFT,
Why?
>    Add Home/End to locate the string begin/end;
OK
>    Emacs-like key bind (C-a/C-e as Home/End, C-f/C-b as forward/backward);
No other part of nconf is emacs-lke - so please no.

> --- /mnt/static/usr/src/linux-3.0-gentoo-r1/scripts/kconfig/nconf.gui.c	2011-07-21
> 19:17:23.000000000 -0700
> +++ /mnt/dynamic/usr/src/linux-3.0-gentoo-r1/scripts/kconfig/nconf.gui.c	2011-08-28
> 18:00:56.441862565 -0700
> @@ -367,7 +367,15 @@ int dialog_inputbox(WINDOW *main_window,
>  	int i, x, y;
>  	int res = -1;
>  	int cursor_position = strlen(init);
> +	int cursor_form_win;
> 
> +	if (strlen(init) +80 > result_len) {
No hardcoded number please. And be consistent with use of spaces.

> +		(void) attrset(attributes[FUNCTION_HIGHLIGHT] | A_BLINK);
> +		mvprintw(0, 0, "result_len(%d) not enough to contain
> init_strlen(%d) in %s:%d:%s\n",
> +			 result_len, strlen(init),
> +			 __FILE__, __LINE__, __func__);
> +		flash();
> +	}
What is the purpose of this statement - does not look clear to me.

Did not look at the rest of the patch - awiting a split-up version.

	Sam
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/