[PATCH] kdb: Fix incorrect naming of history arrow keys in code

[PATCH] kdb: Fix incorrect naming of history arrow keys in code

[Nir Lichtman]

Problem: The kdb CLI code that handles the history up and down
navigation incorrectly names the up and down arrows as ctrl p and n.

Details: This could be some kind of left over legacy.
(maybe inspired by ddb which only reacts to ctrl p and n for history nav).
kdb doesn't react to ctrl p and n, and following the code flow with GDB
reveals that these values map to the up and down arrows.

Solution: Rename the macros accordingly and rename the function name
to reflect that it relates to arrows and not ctrl commands.

Signed-off-by: Nir Lichtman <nir@lichtman.org>
---
 kernel/debug/kdb/kdb_main.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index f5f7d7fb5936..d4b407afb888 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -1123,22 +1123,22 @@ int kdb_parse(const char *cmdstr)
 }
 
 
-static int handle_ctrl_cmd(char *cmd)
+static int handle_arrow_cmd(char *cmd)
 {
-#define CTRL_P	16
-#define CTRL_N	14
+#define ARROW_UP	16
+#define ARROW_DOWN	14
 
 	/* initial situation */
 	if (cmd_head == cmd_tail)
 		return 0;
 	switch (*cmd) {
-	case CTRL_P:
+	case ARROW_UP:
 		if (cmdptr != cmd_tail)
 			cmdptr = (cmdptr + KDB_CMD_HISTORY_COUNT - 1) %
 				 KDB_CMD_HISTORY_COUNT;
 		strscpy(cmd_cur, cmd_hist[cmdptr], CMD_BUFLEN);
 		return 1;
-	case CTRL_N:
+	case ARROW_DOWN:
 		if (cmdptr != cmd_head)
 			cmdptr = (cmdptr+1) % KDB_CMD_HISTORY_COUNT;
 		strscpy(cmd_cur, cmd_hist[cmdptr], CMD_BUFLEN);
@@ -1351,7 +1351,7 @@ static int kdb_local(kdb_reason_t reason, int error, struct pt_regs *regs,
 					*(cmd_hist[cmd_head] +
 					  strlen(cmd_hist[cmd_head])-1) = '\0';
 				}
-				if (!handle_ctrl_cmd(cmdbuf))
+				if (!handle_arrow_cmd(cmdbuf))
 					*(cmd_cur+strlen(cmd_cur)-1) = '\0';
 				cmdbuf = cmd_cur;
 				goto do_full_getstr;
-- 
2.39.2

Re: [PATCH] kdb: Fix incorrect naming of history arrow keys in code

[Doug Anderson]

Hi,

On Thu, Oct 31, 2024 at 12:23 PM Nir Lichtman <nir@lichtman.org> wrote:
>
> Problem: The kdb CLI code that handles the history up and down
> navigation incorrectly names the up and down arrows as ctrl p and n.
>
> Details: This could be some kind of left over legacy.
> (maybe inspired by ddb which only reacts to ctrl p and n for history nav).
> kdb doesn't react to ctrl p and n, and following the code flow with GDB
> reveals that these values map to the up and down arrows.

Really? kdb reacts to "ctrl-P" and "ctrl-N" for me. It also reacts to
"ctrl-F" and "ctrl-B".

I think the code as-is is fine. The arrow key code just converts to
the arrow keys to the (also working) ctrl keys and by the time we're
here we just need to react to Ctrl keys. Ctrl keys are 64 less than
non-ctrl keys and so the definitions of Ctrl-N and Ctrl-P are correct:

>>> ord('N') - 64
14

>>> ord('P') - 64
16

-Doug

Re: [PATCH] kdb: Fix incorrect naming of history arrow keys in code

[Nir Lichtman]

On Thu, Oct 31, 2024 at 04:06:03PM -0700, Doug Anderson wrote:
> >
> > kdb doesn't react to ctrl p and n, and following the code flow with GDB
> > reveals that these values map to the up and down arrows.
> 
> Really? kdb reacts to "ctrl-P" and "ctrl-N" for me. It also reacts to
> "ctrl-F" and "ctrl-B".
> 

Interesting, how do you run kdb? I use the kgdboc=kbd kernel boot param.
I haven't checked with serial as the console since I work with the keyboard,
but if serial does go through this using ctrl+p/n then the code in the
current state is misleading since the keys change depending on the I/O method.

Evidence in the code for usage of arrow keys in the case of keyboard can
be seen by examining kdb_read in kernel/debug/kdb/kdb_io.c, in the /* Down */
and /* Up */ cases the values 14 and 16 can be seen.

Do you suggest to keep as is or to work on a patch with a more generic name that
would fit both?

Thanks,
Nir

> 
> -Doug

Re: [PATCH] kdb: Fix incorrect naming of history arrow keys in code

[Nir Lichtman]

On Fri, Nov 01, 2024 at 12:26:12AM +0000, Nir Lichtman wrote:
> 
> Evidence in the code for usage of arrow keys in the case of keyboard can
> be seen by examining kdb_read in kernel/debug/kdb/kdb_io.c, in the /* Down */
> and /* Up */ cases the values 14 and 16 can be seen.
> 

Correction: The evidence can be seen in the kdb_keyboard.c file
in the function kdb_get_kbd_char which gets the scan codes from the keyboard.
The conversion between the up and down arrow scan codes to the 16 and 14 values
is at lines 138 until 141.

I am thinking maybe a good solution for this confusing passing of magics
is to export the magics to macro definitions,
or maybe to make the keyboard flow also actually use CTRL chords?

Thanks,
Nir

Re: [PATCH] kdb: Fix incorrect naming of history arrow keys in code

[Doug Anderson]

Hi,

On Thu, Oct 31, 2024 at 5:26 PM Nir Lichtman <nir@lichtman.org> wrote:
>
> On Thu, Oct 31, 2024 at 04:06:03PM -0700, Doug Anderson wrote:
> > >
> > > kdb doesn't react to ctrl p and n, and following the code flow with GDB
> > > reveals that these values map to the up and down arrows.
> >
> > Really? kdb reacts to "ctrl-P" and "ctrl-N" for me. It also reacts to
> > "ctrl-F" and "ctrl-B".
> >
>
> Interesting, how do you run kdb? I use the kgdboc=kbd kernel boot param.
> I haven't checked with serial as the console since I work with the keyboard,
> but if serial does go through this using ctrl+p/n then the code in the
> current state is misleading since the keys change depending on the I/O method.

Wow, I've never used the keyboard method since I've never run kdb on a
machine that supports it. :-P


> Evidence in the code for usage of arrow keys in the case of keyboard can
> be seen by examining kdb_read in kernel/debug/kdb/kdb_io.c, in the /* Down */
> and /* Up */ cases the values 14 and 16 can be seen.

Right. Essentially the logic is converting the Up and Down sequences
to the characters Ctrl-P and Ctrl-N. ...so by the time we get to
handle_ctrl_cmd() the characters really are Ctrl commands, not arrow
commands. Thus handle_ctrl_cmd() is correct as is.


> Do you suggest to keep as is or to work on a patch with a more generic name that
> would fit both?

IMO it's a bug that the keyboard code isn't properly reporting Ctrl-N
and Ctrl-P. Does it handle other Ctrl characters? Ctrl-A should go to
the start of the line and Ctrl-E the end. Maybe you can track down why
this isn't happening?

-Doug

Re: [PATCH] kdb: Fix incorrect naming of history arrow keys in code

[Daniel Thompson]

On Fri, Nov 01, 2024 at 08:29:39AM -0700, Doug Anderson wrote:
> Hi,
>
> On Thu, Oct 31, 2024 at 5:26 PM Nir Lichtman <nir@lichtman.org> wrote:
> >
> > On Thu, Oct 31, 2024 at 04:06:03PM -0700, Doug Anderson wrote:
> > > >
> > > > kdb doesn't react to ctrl p and n, and following the code flow with GDB
> > > > reveals that these values map to the up and down arrows.
> > >
> > > Really? kdb reacts to "ctrl-P" and "ctrl-N" for me. It also reacts to
> > > "ctrl-F" and "ctrl-B".
> > >
> >
> > Interesting, how do you run kdb? I use the kgdboc=kbd kernel boot param.
> > I haven't checked with serial as the console since I work with the keyboard,
> > but if serial does go through this using ctrl+p/n then the code in the
> > current state is misleading since the keys change depending on the I/O method.
>
> Wow, I've never used the keyboard method since I've never run kdb on a
> machine that supports it. :-P
>
>
> > Evidence in the code for usage of arrow keys in the case of keyboard can
> > be seen by examining kdb_read in kernel/debug/kdb/kdb_io.c, in the /* Down */
> > and /* Up */ cases the values 14 and 16 can be seen.
>
> Right. Essentially the logic is converting the Up and Down sequences
> to the characters Ctrl-P and Ctrl-N. ...so by the time we get to
> handle_ctrl_cmd() the characters really are Ctrl commands, not arrow
> commands. Thus handle_ctrl_cmd() is correct as is.

Those comments, which I'm pretty sure I added, are arguably more a
reminder about how the input systems map up/down (which on serial are
multi-byte escape sequences) into single character control codes.


> > Do you suggest to keep as is or to work on a patch with a more generic name that
> > would fit both?
>
> IMO it's a bug that the keyboard code isn't properly reporting Ctrl-N
> and Ctrl-P. Does it handle other Ctrl characters? Ctrl-A should go to
> the start of the line and Ctrl-E the end. Maybe you can track down why
> this isn't happening?

I'm with Doug on this one. What values does the kdb code generate
Ctrl-<letter>? IMHO it should be generating 14 for ctrl-N and 16 for
ctrl-P.

BTW ctrl-N and ctrl-P are very common keystrokes for command line
history (try them in any readline program such as bash).


Daniel.

Re: [PATCH] kdb: Fix incorrect naming of history arrow keys in code

[Nir Lichtman]

On Fri, Nov 01, 2024 at 06:34:18PM +0000, Daniel Thompson wrote:
> On Fri, Nov 01, 2024 at 08:29:39AM -0700, Doug Anderson wrote:
> > On Thu, Oct 31, 2024 at 5:26 PM Nir Lichtman <nir@lichtman.org> wrote:
> > >
> > > On Thu, Oct 31, 2024 at 04:06:03PM -0700, Doug Anderson wrote:
> > > > >
> > > > > kdb doesn't react to ctrl p and n, and following the code flow with GDB
> > > > > reveals that these values map to the up and down arrows.
> > > >
> > > > Really? kdb reacts to "ctrl-P" and "ctrl-N" for me. It also reacts to
> > > > "ctrl-F" and "ctrl-B".
> > > >
> > >
> > > Interesting, how do you run kdb? I use the kgdboc=kbd kernel boot param.
> > > I haven't checked with serial as the console since I work with the keyboard,
> > > but if serial does go through this using ctrl+p/n then the code in the
> > > current state is misleading since the keys change depending on the I/O method.
> >
> > > Evidence in the code for usage of arrow keys in the case of keyboard can
> > > be seen by examining kdb_read in kernel/debug/kdb/kdb_io.c, in the /* Down */
> > > and /* Up */ cases the values 14 and 16 can be seen.
> >
> > Right. Essentially the logic is converting the Up and Down sequences
> > to the characters Ctrl-P and Ctrl-N. ...so by the time we get to
> > handle_ctrl_cmd() the characters really are Ctrl commands, not arrow
> > commands. Thus handle_ctrl_cmd() is correct as is.
> 
> Those comments, which I'm pretty sure I added, are arguably more a
> reminder about how the input systems map up/down (which on serial are
> multi-byte escape sequences) into single character control codes.
> 
> 
> > > Do you suggest to keep as is or to work on a patch with a more generic name that
> > > would fit both?
> >
> > IMO it's a bug that the keyboard code isn't properly reporting Ctrl-N
> > and Ctrl-P. Does it handle other Ctrl characters? Ctrl-A should go to
> > the start of the line and Ctrl-E the end. Maybe you can track down why
> > this isn't happening?
> 
> I'm with Doug on this one. What values does the kdb code generate
> Ctrl-<letter>? IMHO it should be generating 14 for ctrl-N and 16 for
> ctrl-P.
> 
> BTW ctrl-N and ctrl-P are very common keystrokes for command line
> history (try them in any readline program such as bash).
> 

Hi Doug and Daniel,

In the current state using KDB in keyboard mode doesn't react to expected
CTRL chords such as CTRL+N/P/A/E, but does react to arrow keys,
Following this, I have also inspected the serial mode and over there I see
that both arrows and CTRL chords work.
From what I understand going forward, the best solution would be to add support
for the CTRL chords in the keyboard mode as well to be in line with serial?

BTW I originally wanted to add support also for CTRL+U and CTRL+W what is your
opinion about that as a feature as well?

Thanks,
Nir

Re: [PATCH] kdb: Fix incorrect naming of history arrow keys in code

[Doug Anderson]

Hi,

On Fri, Nov 1, 2024 at 11:57 AM Nir Lichtman <nir@lichtman.org> wrote:
>
> In the current state using KDB in keyboard mode doesn't react to expected
> CTRL chords such as CTRL+N/P/A/E, but does react to arrow keys,
> Following this, I have also inspected the serial mode and over there I see
> that both arrows and CTRL chords work.
> From what I understand going forward, the best solution would be to add support
> for the CTRL chords in the keyboard mode as well to be in line with serial?

Sounds right to me.


> BTW I originally wanted to add support also for CTRL+U and CTRL+W what is your
> opinion about that as a feature as well?

That sounds reasonable.

-Doug