Re: [PATCH v2] kdb: unify CMD_BUFLEN definition into kdb_private.h

[Naveen Kumar Chaudhary]

Thanks Doug for the review. Apologies, I missed to realize that my
config for kgdb_kdb was not enabled and hence missed that compilation
error. Have taken care this time with this new patch.

One concern I should mention about `cmd_hist[32][200]` earlier which was
32 entries × 200 = 6,400 bytes of static storage. Bumping to 256 would
make it 8,192 bytes — a ~28% increase in static memory. Though this
should be ok for a debugger. Alternatively, I was thinking to rename
these two differently so that they don't clash, plus we won't have this
memory bump. But it has its own quirks.

Assuming that 1.8KB is meaningless for an optional debugger and a single
definition is impossible to get wrong, the unified approach is the better
long-term choice. Please correct me in case I am wrong.

Regards,
Naveen

On Wed 17 Jun 07:58 AM, Naveen Kumar Chaudhary wrote:
> CMD_BUFLEN was defined separately in kdb_io.c (256) and kdb_main.c
> (200), causing kdb_main.c to use the wrong size when formatting the
> prompt string into kdb_prompt_str (which is 256 bytes).
> 
> Move CMD_BUFLEN (256) into kdb_private.h so all users share a single
> consistent definition, and remove the local definitions from both
> files.
> 
> Fixes: 5d5314d6795f ("kdb: core for kgdb back end (1 of 2)")
> Signed-off-by: Naveen Kumar Chaudhary <naveen.osdev@xxxxxxxxx>
> ---
>  kernel/debug/kdb/kdb_io.c      | 1 -
>  kernel/debug/kdb/kdb_main.c    | 6 ++----
>  kernel/debug/kdb/kdb_private.h | 3 ++-
>  3 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index c399f11740ef..f5b1b7d4c9c8 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -22,7 +22,6 @@
>  #include <linux/kallsyms.h>
>  #include "kdb_private.h"
>  
> -#define CMD_BUFLEN 256
>  char kdb_prompt_str[CMD_BUFLEN];
>  
>  int kdb_trap_printk;
> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> index ddce56b47b25..ca0126db9850 100644
> --- a/kernel/debug/kdb/kdb_main.c
> +++ b/kernel/debug/kdb/kdb_main.c
> @@ -783,8 +783,6 @@ static int kdb_exec_defcmd(int argc, const char **argv)
>  
>  /* Command history */
>  #define KDB_CMD_HISTORY_COUNT	32
> -#define CMD_BUFLEN		200	/* kdb_printf: max printline
> -					 * size == 256 */
>  static unsigned int cmd_head, cmd_tail;
>  static unsigned int cmdptr;
>  static char cmd_hist[KDB_CMD_HISTORY_COUNT][CMD_BUFLEN];
> @@ -1265,8 +1263,8 @@ static int kdb_local(kdb_reason_t reason, int error, struct pt_regs *regs,
>  
>  do_full_getstr:
>  		/* PROMPT can only be set if we have MEM_READ permission. */
> -		snprintf(kdb_prompt_str, CMD_BUFLEN, kdbgetenv("PROMPT"),
> -			 raw_smp_processor_id());
> +		snprintf(kdb_prompt_str, CMD_BUFLEN,
> +			 kdbgetenv("PROMPT"), raw_smp_processor_id());
>  
>  		/*
>  		 * Fetch command from keyboard
> diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h
> index 92a28b8ab604..722e8aa50724 100644
> --- a/kernel/debug/kdb/kdb_private.h
> +++ b/kernel/debug/kdb/kdb_private.h
> @@ -225,7 +225,8 @@ extern void kdb_kbd_cleanup_state(void);
>  #define kdb_kbd_cleanup_state()
>  #endif /* ! CONFIG_KDB_KEYBOARD */
>  
> -extern char kdb_prompt_str[];
> +#define CMD_BUFLEN 256
> +extern char kdb_prompt_str[CMD_BUFLEN];
>  
>  #define	KDB_WORD_SIZE	((int)sizeof(unsigned long))
>  
> -- 
> 2.43.0
>