[PATCH v2 0/5] kdb: Cleanup code to read user input and handle escape sequences

[PATCH v2 0/5] kdb: Cleanup code to read user input and handle escape sequences

[Daniel Thompson]

I've been meaning to repost this for some time, and inspired by
having someone keen to review it, I dug it out again!

I split this as carefully as I could into small pieces but the original
code was complex so even in small bits it doesn't make for light
reading.  Things do make more sense once you realize/remember that
escape_delay is a count down timer that expires the escape sequences!

Most of the patches are simple tidy ups although patches 4 and 5
introduce new behaviours. Patch 4 shouldn't be controversial but
perhaps patch 5 is (although hopefully not ;-) ).

Mostly this is auto tested, see here:
https://github.com/daniel-thompson/kgdbtest/commit/c65e28d99357c2df6dac2cebe195574e634d04dc

Changes in v2:

 - Improve comment in patch 4 to better describe what is happening
 - Rebase on v5.4-rc2

Daniel Thompson (5):
  kdb: Tidy up code to handle escape sequences
  kdb: Simplify code to fetch characters from console
  kdb: Remove special case logic from kdb_read()
  kdb: Improve handling of characters from different input sources
  kdb: Tweak escape handling for vi users

 kernel/debug/kdb/kdb_io.c | 222 ++++++++++++++++++--------------------
 1 file changed, 103 insertions(+), 119 deletions(-)

--
2.21.0

[PATCH v2 1/5] kdb: Tidy up code to handle escape sequences

[Daniel Thompson]

kdb_read_get_key() has extremely complex break/continue control flow
managed by state variables and is very hard to review or modify. In
particular the way the escape sequence handling interacts with the
general control flow is hard to follow. Separate out the escape key
handling, without changing the control flow. This makes the main body of
the code easier to review.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
---
 kernel/debug/kdb/kdb_io.c | 127 ++++++++++++++++++++------------------
 1 file changed, 66 insertions(+), 61 deletions(-)

diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 3a5184eb6977..68e2c29f14f5 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -49,6 +49,63 @@ static int kgdb_transition_check(char *buffer)
 	return 0;
 }
 
+/*
+ * kdb_read_handle_escape
+ *
+ * Run a validity check on an accumulated escape sequence.
+ *
+ * Returns -1 if the escape sequence is unwanted, 0 if it is incomplete,
+ * otherwise it returns a mapped key value to pass to the upper layers.
+ */
+static int kdb_read_handle_escape(char *buf, size_t sz)
+{
+	char *lastkey = buf + sz - 1;
+
+	switch (sz) {
+	case 1:
+		if (*lastkey == '\e')
+			return 0;
+		break;
+
+	case 2: /* \e<something> */
+		if (*lastkey == '[')
+			return 0;
+		break;
+
+	case 3:
+		switch (*lastkey) {
+		case 'A': /* \e[A, up arrow */
+			return 16;
+		case 'B': /* \e[B, down arrow */
+			return 14;
+		case 'C': /* \e[C, right arrow */
+			return 6;
+		case 'D': /* \e[D, left arrow */
+			return 2;
+		case '1': /* \e[<1,3,4>], may be home, del, end */
+		case '3':
+		case '4':
+			return 0;
+		}
+		break;
+
+	case 4:
+		if (*lastkey == '~') {
+			switch (buf[2]) {
+			case '1': /* \e[1~, home */
+				return 1;
+			case '3': /* \e[3~, del */
+				return 4;
+			case '4': /* \e[4~, end */
+				return 5;
+			}
+		}
+		break;
+	}
+
+	return -1;
+}
+
 static int kdb_read_get_key(char *buffer, size_t bufsize)
 {
 #define ESCAPE_UDELAY 1000
@@ -102,68 +159,16 @@ static int kdb_read_get_key(char *buffer, size_t bufsize)
 				escape_delay = 2;
 				continue;
 			}
-			if (ped - escape_data == 1) {
-				/* \e */
-				continue;
-			} else if (ped - escape_data == 2) {
-				/* \e<something> */
-				if (key != '[')
-					escape_delay = 2;
-				continue;
-			} else if (ped - escape_data == 3) {
-				/* \e[<something> */
-				int mapkey = 0;
-				switch (key) {
-				case 'A': /* \e[A, up arrow */
-					mapkey = 16;
-					break;
-				case 'B': /* \e[B, down arrow */
-					mapkey = 14;
-					break;
-				case 'C': /* \e[C, right arrow */
-					mapkey = 6;
-					break;
-				case 'D': /* \e[D, left arrow */
-					mapkey = 2;
-					break;
-				case '1': /* dropthrough */
-				case '3': /* dropthrough */
-				/* \e[<1,3,4>], may be home, del, end */
-				case '4':
-					mapkey = -1;
-					break;
-				}
-				if (mapkey != -1) {
-					if (mapkey > 0) {
-						escape_data[0] = mapkey;
-						escape_data[1] = '\0';
-					}
-					escape_delay = 2;
-				}
-				continue;
-			} else if (ped - escape_data == 4) {
-				/* \e[<1,3,4><something> */
-				int mapkey = 0;
-				if (key == '~') {
-					switch (escape_data[2]) {
-					case '1': /* \e[1~, home */
-						mapkey = 1;
-						break;
-					case '3': /* \e[3~, del */
-						mapkey = 4;
-						break;
-					case '4': /* \e[4~, end */
-						mapkey = 5;
-						break;
-					}
-				}
-				if (mapkey > 0) {
-					escape_data[0] = mapkey;
-					escape_data[1] = '\0';
-				}
-				escape_delay = 2;
-				continue;
+
+			key = kdb_read_handle_escape(escape_data,
+						     ped - escape_data);
+			if (key > 0) {
+				escape_data[0] = key;
+				escape_data[1] = '\0';
 			}
+			if (key)
+				escape_delay = 2;
+			continue;
 		}
 		break;	/* A key to process */
 	}
-- 
2.21.0

Re: [PATCH v2 1/5] kdb: Tidy up code to handle escape sequences

[Doug Anderson]

Hi,

On Tue, Oct 8, 2019 at 6:21 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> kdb_read_get_key() has extremely complex break/continue control flow
> managed by state variables and is very hard to review or modify. In
> particular the way the escape sequence handling interacts with the
> general control flow is hard to follow. Separate out the escape key
> handling, without changing the control flow. This makes the main body of
> the code easier to review.
>
> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> ---
>  kernel/debug/kdb/kdb_io.c | 127 ++++++++++++++++++++------------------
>  1 file changed, 66 insertions(+), 61 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 3a5184eb6977..68e2c29f14f5 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -49,6 +49,63 @@ static int kgdb_transition_check(char *buffer)
>         return 0;
>  }
>
> +/*
> + * kdb_read_handle_escape
> + *
> + * Run a validity check on an accumulated escape sequence.
> + *
> + * Returns -1 if the escape sequence is unwanted, 0 if it is incomplete,
> + * otherwise it returns a mapped key value to pass to the upper layers.
> + */
> +static int kdb_read_handle_escape(char *buf, size_t sz)
> +{
> +       char *lastkey = buf + sz - 1;
> +
> +       switch (sz) {
> +       case 1:
> +               if (*lastkey == '\e')
> +                       return 0;

Technically the "if" here isn't needed, at least not until a later
patch in the series.  The only way we could get here is if *lastkey ==
'\e'...

...but I suppose it's fine to keep it here in preparation for the last patch.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

[PATCH v2 2/5] kdb: Simplify code to fetch characters from console

[Daniel Thompson]

Currently kdb_read_get_key() contains complex control flow that, on
close inspection, turns out to be unnecessary. In particular:

1. It is impossible to enter the branch conditioned on (escape_delay == 1)
   except when the loop enters with (escape_delay == 2) allowing us to
   combine the branches.

2. Most of the code conditioned on (escape_delay == 2) simply modifies
   local data and then breaks out of the loop causing the function to
   return escape_data[0].

3. Based on #2 there is not actually any need to ever explicitly set
   escape_delay to 2 because we it is much simpler to directly return
   escape_data[0] instead.

4. escape_data[0] is, for all but one exit path, known to be '\e'.

Simplify the code based on these observations.

There is a subtle (and harmless) change of behaviour resulting from this
simplification: instead of letting the escape timeout after ~1998
milliseconds we now timeout after ~2000 milliseconds

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
---
 kernel/debug/kdb/kdb_io.c | 38 ++++++++++++++------------------------
 1 file changed, 14 insertions(+), 24 deletions(-)

diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 68e2c29f14f5..78cb6e339408 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -122,25 +122,18 @@ static int kdb_read_get_key(char *buffer, size_t bufsize)
 			touch_nmi_watchdog();
 			f = &kdb_poll_funcs[0];
 		}
-		if (escape_delay == 2) {
-			*ped = '\0';
-			ped = escape_data;
-			--escape_delay;
-		}
-		if (escape_delay == 1) {
-			key = *ped++;
-			if (!*ped)
-				--escape_delay;
-			break;
-		}
+
 		key = (*f)();
+
 		if (key == -1) {
 			if (escape_delay) {
 				udelay(ESCAPE_UDELAY);
-				--escape_delay;
+				if (--escape_delay == 0)
+					return '\e';
 			}
 			continue;
 		}
+
 		if (bufsize <= 2) {
 			if (key == '\r')
 				key = '\n';
@@ -148,28 +141,25 @@ static int kdb_read_get_key(char *buffer, size_t bufsize)
 			*buffer = '\0';
 			return -1;
 		}
+
 		if (escape_delay == 0 && key == '\e') {
 			escape_delay = ESCAPE_DELAY;
 			ped = escape_data;
 			f_escape = f;
 		}
 		if (escape_delay) {
-			*ped++ = key;
-			if (f_escape != f) {
-				escape_delay = 2;
-				continue;
-			}
+			if (f_escape != f)
+				return '\e';
 
+			*ped++ = key;
 			key = kdb_read_handle_escape(escape_data,
 						     ped - escape_data);
-			if (key > 0) {
-				escape_data[0] = key;
-				escape_data[1] = '\0';
-			}
-			if (key)
-				escape_delay = 2;
-			continue;
+			if (key < 0)
+				return '\e';
+			if (key == 0)
+				continue;
 		}
+
 		break;	/* A key to process */
 	}
 	return key;
-- 
2.21.0

Re: [PATCH v2 2/5] kdb: Simplify code to fetch characters from console

[Doug Anderson]

Hi,

On Tue, Oct 8, 2019 at 6:21 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> Currently kdb_read_get_key() contains complex control flow that, on
> close inspection, turns out to be unnecessary. In particular:
>
> 1. It is impossible to enter the branch conditioned on (escape_delay == 1)
>    except when the loop enters with (escape_delay == 2) allowing us to
>    combine the branches.
>
> 2. Most of the code conditioned on (escape_delay == 2) simply modifies
>    local data and then breaks out of the loop causing the function to
>    return escape_data[0].
>
> 3. Based on #2 there is not actually any need to ever explicitly set
>    escape_delay to 2 because we it is much simpler to directly return
>    escape_data[0] instead.
>
> 4. escape_data[0] is, for all but one exit path, known to be '\e'.
>
> Simplify the code based on these observations.
>
> There is a subtle (and harmless) change of behaviour resulting from this
> simplification: instead of letting the escape timeout after ~1998
> milliseconds we now timeout after ~2000 milliseconds
>
> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> ---
>  kernel/debug/kdb/kdb_io.c | 38 ++++++++++++++------------------------
>  1 file changed, 14 insertions(+), 24 deletions(-)

Looks great.  My only comment is that since this was the 2nd patch in
the series I spent a whole bunch of time deducing all these same
things when reviewing the first patch.  :-P

Reviewed-by: Douglas Anderson <dianders@chromium.org>

[PATCH v2 3/5] kdb: Remove special case logic from kdb_read()

[Daniel Thompson]

kdb_read() contains special case logic to force it exit after reading
a single character. We can remove all the special case logic by directly
calling the function to read a single character instead. This also
allows us to tidy up the function prototype which, because it now matches
getchar(), we can also rename in order to make its role clearer.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
---
 kernel/debug/kdb/kdb_io.c | 56 ++++++++++++++++-----------------------
 1 file changed, 23 insertions(+), 33 deletions(-)

diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 78cb6e339408..a9e73bc9d1c3 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -106,7 +106,19 @@ static int kdb_read_handle_escape(char *buf, size_t sz)
 	return -1;
 }
 
-static int kdb_read_get_key(char *buffer, size_t bufsize)
+/*
+ * kdb_getchar
+ *
+ * Read a single character from kdb console (or consoles).
+ *
+ * An escape key could be the start of a vt100 control sequence such as \e[D
+ * (left arrow) or it could be a character in its own right.  The standard
+ * method for detecting the difference is to wait for 2 seconds to see if there
+ * are any other characters.  kdb is complicated by the lack of a timer service
+ * (interrupts are off), by multiple input sources. Escape sequence processing
+ * has to be done as states in the polling loop.
+ */
+static int kdb_getchar(void)
 {
 #define ESCAPE_UDELAY 1000
 #define ESCAPE_DELAY (2*1000000/ESCAPE_UDELAY) /* 2 seconds worth of udelays */
@@ -124,7 +136,6 @@ static int kdb_read_get_key(char *buffer, size_t bufsize)
 		}
 
 		key = (*f)();
-
 		if (key == -1) {
 			if (escape_delay) {
 				udelay(ESCAPE_UDELAY);
@@ -134,14 +145,6 @@ static int kdb_read_get_key(char *buffer, size_t bufsize)
 			continue;
 		}
 
-		if (bufsize <= 2) {
-			if (key == '\r')
-				key = '\n';
-			*buffer++ = key;
-			*buffer = '\0';
-			return -1;
-		}
-
 		if (escape_delay == 0 && key == '\e') {
 			escape_delay = ESCAPE_DELAY;
 			ped = escape_data;
@@ -183,17 +186,7 @@ static int kdb_read_get_key(char *buffer, size_t bufsize)
  *	function.  It is not reentrant - it relies on the fact
  *	that while kdb is running on only one "master debug" cpu.
  * Remarks:
- *
- * The buffer size must be >= 2.  A buffer size of 2 means that the caller only
- * wants a single key.
- *
- * An escape key could be the start of a vt100 control sequence such as \e[D
- * (left arrow) or it could be a character in its own right.  The standard
- * method for detecting the difference is to wait for 2 seconds to see if there
- * are any other characters.  kdb is complicated by the lack of a timer service
- * (interrupts are off), by multiple input sources and by the need to sometimes
- * return after just one key.  Escape sequence processing has to be done as
- * states in the polling loop.
+ *	The buffer size must be >= 2.
  */
 
 static char *kdb_read(char *buffer, size_t bufsize)
@@ -228,9 +221,7 @@ static char *kdb_read(char *buffer, size_t bufsize)
 	*cp = '\0';
 	kdb_printf("%s", buffer);
 poll_again:
-	key = kdb_read_get_key(buffer, bufsize);
-	if (key == -1)
-		return buffer;
+	key = kdb_getchar();
 	if (key != 9)
 		tab = 0;
 	switch (key) {
@@ -741,7 +732,7 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
 
 	/* check for having reached the LINES number of printed lines */
 	if (kdb_nextline >= linecount) {
-		char buf1[16] = "";
+		char ch;
 
 		/* Watch out for recursion here.  Any routine that calls
 		 * kdb_printf will come back through here.  And kdb_read
@@ -776,39 +767,38 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
 		if (logging)
 			printk("%s", moreprompt);
 
-		kdb_read(buf1, 2); /* '2' indicates to return
-				    * immediately after getting one key. */
+		ch = kdb_getchar();
 		kdb_nextline = 1;	/* Really set output line 1 */
 
 		/* empty and reset the buffer: */
 		kdb_buffer[0] = '\0';
 		next_avail = kdb_buffer;
 		size_avail = sizeof(kdb_buffer);
-		if ((buf1[0] == 'q') || (buf1[0] == 'Q')) {
+		if ((ch == 'q') || (ch == 'Q')) {
 			/* user hit q or Q */
 			KDB_FLAG_SET(CMD_INTERRUPT); /* command interrupted */
 			KDB_STATE_CLEAR(PAGER);
 			/* end of command output; back to normal mode */
 			kdb_grepping_flag = 0;
 			kdb_printf("\n");
-		} else if (buf1[0] == ' ') {
+		} else if (ch == ' ') {
 			kdb_printf("\r");
 			suspend_grep = 1; /* for this recursion */
-		} else if (buf1[0] == '\n') {
+		} else if (ch == '\n' || ch == '\r') {
 			kdb_nextline = linecount - 1;
 			kdb_printf("\r");
 			suspend_grep = 1; /* for this recursion */
-		} else if (buf1[0] == '/' && !kdb_grepping_flag) {
+		} else if (ch == '/' && !kdb_grepping_flag) {
 			kdb_printf("\r");
 			kdb_getstr(kdb_grep_string, KDB_GREP_STRLEN,
 				   kdbgetenv("SEARCHPROMPT") ?: "search> ");
 			*strchrnul(kdb_grep_string, '\n') = '\0';
 			kdb_grepping_flag += KDB_GREPPING_FLAG_SEARCH;
 			suspend_grep = 1; /* for this recursion */
-		} else if (buf1[0] && buf1[0] != '\n') {
+		} else if (ch && ch != '\n') {
 			/* user hit something other than enter */
 			suspend_grep = 1; /* for this recursion */
-			if (buf1[0] != '/')
+			if (ch != '/')
 				kdb_printf(
 				    "\nOnly 'q', 'Q' or '/' are processed at "
 				    "more prompt, input ignored\n");
-- 
2.21.0

Re: [PATCH v2 3/5] kdb: Remove special case logic from kdb_read()

[Doug Anderson]

Hi,

On Tue, Oct 8, 2019 at 6:21 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> kdb_read() contains special case logic to force it exit after reading
> a single character. We can remove all the special case logic by directly
> calling the function to read a single character instead. This also
> allows us to tidy up the function prototype which, because it now matches
> getchar(), we can also rename in order to make its role clearer.

nit: since you're doing the rename, should you rename
kdb_read_handle_escape() to match?


> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> ---
>  kernel/debug/kdb/kdb_io.c | 56 ++++++++++++++++-----------------------
>  1 file changed, 23 insertions(+), 33 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 78cb6e339408..a9e73bc9d1c3 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -106,7 +106,19 @@ static int kdb_read_handle_escape(char *buf, size_t sz)
>         return -1;
>  }
>
> -static int kdb_read_get_key(char *buffer, size_t bufsize)
> +/*
> + * kdb_getchar
> + *
> + * Read a single character from kdb console (or consoles).

nit: should we start moving to the standard kernel convention of
kernel-doc style comments?  See
"Documentation/doc-guide/kernel-doc.rst"


> + *
> + * An escape key could be the start of a vt100 control sequence such as \e[D
> + * (left arrow) or it could be a character in its own right.  The standard
> + * method for detecting the difference is to wait for 2 seconds to see if there
> + * are any other characters.  kdb is complicated by the lack of a timer service
> + * (interrupts are off), by multiple input sources. Escape sequence processing
> + * has to be done as states in the polling loop.

Before your paragraph, maybe add: "Most of the work of this function
is dealing with escape sequences." to give it a little bit of context.


> + */
> +static int kdb_getchar(void)

Is "int" the right return type here, or "unsigned char"?  You never
return EOF, right?  Always a valid character?  NOTE: if you do change
this to "unsigned char" I think you still need to keep the local "key"
variable as an "int" since -1 shouldn't be confused with the character
255.


>  {
>  #define ESCAPE_UDELAY 1000
>  #define ESCAPE_DELAY (2*1000000/ESCAPE_UDELAY) /* 2 seconds worth of udelays */
> @@ -124,7 +136,6 @@ static int kdb_read_get_key(char *buffer, size_t bufsize)
>                 }
>
>                 key = (*f)();
> -
>                 if (key == -1) {
>                         if (escape_delay) {
>                                 udelay(ESCAPE_UDELAY);
> @@ -134,14 +145,6 @@ static int kdb_read_get_key(char *buffer, size_t bufsize)
>                         continue;
>                 }
>
> -               if (bufsize <= 2) {
> -                       if (key == '\r')
> -                               key = '\n';
> -                       *buffer++ = key;
> -                       *buffer = '\0';
> -                       return -1;
> -               }
> -
>                 if (escape_delay == 0 && key == '\e') {
>                         escape_delay = ESCAPE_DELAY;
>                         ped = escape_data;
> @@ -183,17 +186,7 @@ static int kdb_read_get_key(char *buffer, size_t bufsize)
>   *     function.  It is not reentrant - it relies on the fact
>   *     that while kdb is running on only one "master debug" cpu.
>   * Remarks:
> - *
> - * The buffer size must be >= 2.  A buffer size of 2 means that the caller only
> - * wants a single key.

By removing this you broke "BTAPROMPT".  So doing:

set BTAPROMPT=1
bta

It's now impossible to quit out.  Not that I've ever used BTAPROMPT,
but seems like we should either get rid of it or keep it working.


> - *
> - * An escape key could be the start of a vt100 control sequence such as \e[D
> - * (left arrow) or it could be a character in its own right.  The standard
> - * method for detecting the difference is to wait for 2 seconds to see if there
> - * are any other characters.  kdb is complicated by the lack of a timer service
> - * (interrupts are off), by multiple input sources and by the need to sometimes
> - * return after just one key.  Escape sequence processing has to be done as
> - * states in the polling loop.
> + *     The buffer size must be >= 2.
>   */
>
>  static char *kdb_read(char *buffer, size_t bufsize)
> @@ -228,9 +221,7 @@ static char *kdb_read(char *buffer, size_t bufsize)
>         *cp = '\0';
>         kdb_printf("%s", buffer);
>  poll_again:
> -       key = kdb_read_get_key(buffer, bufsize);
> -       if (key == -1)
> -               return buffer;
> +       key = kdb_getchar();
>         if (key != 9)
>                 tab = 0;
>         switch (key) {
> @@ -741,7 +732,7 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
>
>         /* check for having reached the LINES number of printed lines */
>         if (kdb_nextline >= linecount) {
> -               char buf1[16] = "";
> +               char ch;

The type of "ch" should be the same as returned by kdb_getchar()?
Either "int" if you're keeping it "int" or "unsigned char"?


>                 /* Watch out for recursion here.  Any routine that calls
>                  * kdb_printf will come back through here.  And kdb_read
> @@ -776,39 +767,38 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
>                 if (logging)
>                         printk("%s", moreprompt);
>
> -               kdb_read(buf1, 2); /* '2' indicates to return
> -                                   * immediately after getting one key. */
> +               ch = kdb_getchar();
>                 kdb_nextline = 1;       /* Really set output line 1 */
>
>                 /* empty and reset the buffer: */
>                 kdb_buffer[0] = '\0';
>                 next_avail = kdb_buffer;
>                 size_avail = sizeof(kdb_buffer);
> -               if ((buf1[0] == 'q') || (buf1[0] == 'Q')) {
> +               if ((ch == 'q') || (ch == 'Q')) {
>                         /* user hit q or Q */
>                         KDB_FLAG_SET(CMD_INTERRUPT); /* command interrupted */
>                         KDB_STATE_CLEAR(PAGER);
>                         /* end of command output; back to normal mode */
>                         kdb_grepping_flag = 0;
>                         kdb_printf("\n");
> -               } else if (buf1[0] == ' ') {
> +               } else if (ch == ' ') {
>                         kdb_printf("\r");
>                         suspend_grep = 1; /* for this recursion */
> -               } else if (buf1[0] == '\n') {
> +               } else if (ch == '\n' || ch == '\r') {
>                         kdb_nextline = linecount - 1;
>                         kdb_printf("\r");
>                         suspend_grep = 1; /* for this recursion */
> -               } else if (buf1[0] == '/' && !kdb_grepping_flag) {
> +               } else if (ch == '/' && !kdb_grepping_flag) {
>                         kdb_printf("\r");
>                         kdb_getstr(kdb_grep_string, KDB_GREP_STRLEN,
>                                    kdbgetenv("SEARCHPROMPT") ?: "search> ");
>                         *strchrnul(kdb_grep_string, '\n') = '\0';
>                         kdb_grepping_flag += KDB_GREPPING_FLAG_SEARCH;
>                         suspend_grep = 1; /* for this recursion */
> -               } else if (buf1[0] && buf1[0] != '\n') {
> +               } else if (ch && ch != '\n') {

Remove "&& ch != '\n'".  We would have hit an earlier case in the
if/else anyway.  If you really want to keep it here for some reason, I
guess you should also handle '\r' ?


-Doug

Re: [PATCH v2 3/5] kdb: Remove special case logic from kdb_read()

[Daniel Thompson]

On Tue, Oct 08, 2019 at 03:21:02PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Tue, Oct 8, 2019 at 6:21 AM Daniel Thompson
> <daniel.thompson@linaro.org> wrote:
> >
> > kdb_read() contains special case logic to force it exit after reading
> > a single character. We can remove all the special case logic by directly
> > calling the function to read a single character instead. This also
> > allows us to tidy up the function prototype which, because it now matches
> > getchar(), we can also rename in order to make its role clearer.
> 
> nit: since you're doing the rename, should you rename
> kdb_read_handle_escape() to match?

Will do.


> > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> > ---
> >  kernel/debug/kdb/kdb_io.c | 56 ++++++++++++++++-----------------------
> >  1 file changed, 23 insertions(+), 33 deletions(-)
> >
> > diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> > index 78cb6e339408..a9e73bc9d1c3 100644
> > --- a/kernel/debug/kdb/kdb_io.c
> > +++ b/kernel/debug/kdb/kdb_io.c
> > @@ -106,7 +106,19 @@ static int kdb_read_handle_escape(char *buf, size_t sz)
> >         return -1;
> >  }
> >
> > -static int kdb_read_get_key(char *buffer, size_t bufsize)
> > +/*
> > + * kdb_getchar
> > + *
> > + * Read a single character from kdb console (or consoles).
> 
> nit: should we start moving to the standard kernel convention of
> kernel-doc style comments?  See
> "Documentation/doc-guide/kernel-doc.rst"

It will look a little odd whilst the others are still in the old form
but it seems like a good direction of travel... will update.

> > + *
> > + * An escape key could be the start of a vt100 control sequence such as \e[D
> > + * (left arrow) or it could be a character in its own right.  The standard
> > + * method for detecting the difference is to wait for 2 seconds to see if there
> > + * are any other characters.  kdb is complicated by the lack of a timer service
> > + * (interrupts are off), by multiple input sources. Escape sequence processing
> > + * has to be done as states in the polling loop.
> 
> Before your paragraph, maybe add: "Most of the work of this function
> is dealing with escape sequences." to give it a little bit of context.
> 
> 
> > + */
> > +static int kdb_getchar(void)
> 
> Is "int" the right return type here, or "unsigned char"?  You never
> return EOF, right?  Always a valid character?  NOTE: if you do change
> this to "unsigned char" I think you still need to keep the local "key"
> variable as an "int" since -1 shouldn't be confused with the character
> 255.

unsigned char sounds best.


> >  {
> >  #define ESCAPE_UDELAY 1000
> >  #define ESCAPE_DELAY (2*1000000/ESCAPE_UDELAY) /* 2 seconds worth of udelays */
> > @@ -124,7 +136,6 @@ static int kdb_read_get_key(char *buffer, size_t bufsize)
> >                 }
> >
> >                 key = (*f)();
> > -
> >                 if (key == -1) {
> >                         if (escape_delay) {
> >                                 udelay(ESCAPE_UDELAY);
> > @@ -134,14 +145,6 @@ static int kdb_read_get_key(char *buffer, size_t bufsize)
> >                         continue;
> >                 }
> >
> > -               if (bufsize <= 2) {
> > -                       if (key == '\r')
> > -                               key = '\n';
> > -                       *buffer++ = key;
> > -                       *buffer = '\0';
> > -                       return -1;
> > -               }
> > -
> >                 if (escape_delay == 0 && key == '\e') {
> >                         escape_delay = ESCAPE_DELAY;
> >                         ped = escape_data;
> > @@ -183,17 +186,7 @@ static int kdb_read_get_key(char *buffer, size_t bufsize)
> >   *     function.  It is not reentrant - it relies on the fact
> >   *     that while kdb is running on only one "master debug" cpu.
> >   * Remarks:
> > - *
> > - * The buffer size must be >= 2.  A buffer size of 2 means that the caller only
> > - * wants a single key.
> 
> By removing this you broke "BTAPROMPT".  So doing:
> 
> set BTAPROMPT=1
> bta
> 
> It's now impossible to quit out.  Not that I've ever used BTAPROMPT,
> but seems like we should either get rid of it or keep it working.

Thanks. Just to check I got exactly what you meant I assume this could
also have been phrased as "it looks like you forgot to convert the
kdb_getstr() in kdb_bt1() over to use the new kdb_getchar() function"?

PS I will update kgdbtest to cover this case.


> > - *
> > - * An escape key could be the start of a vt100 control sequence such as \e[D
> > - * (left arrow) or it could be a character in its own right.  The standard
> > - * method for detecting the difference is to wait for 2 seconds to see if there
> > - * are any other characters.  kdb is complicated by the lack of a timer service
> > - * (interrupts are off), by multiple input sources and by the need to sometimes
> > - * return after just one key.  Escape sequence processing has to be done as
> > - * states in the polling loop.
> > + *     The buffer size must be >= 2.
> >   */
> >
> >  static char *kdb_read(char *buffer, size_t bufsize)
> > @@ -228,9 +221,7 @@ static char *kdb_read(char *buffer, size_t bufsize)
> >         *cp = '\0';
> >         kdb_printf("%s", buffer);
> >  poll_again:
> > -       key = kdb_read_get_key(buffer, bufsize);
> > -       if (key == -1)
> > -               return buffer;
> > +       key = kdb_getchar();
> >         if (key != 9)
> >                 tab = 0;
> >         switch (key) {
> > @@ -741,7 +732,7 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
> >
> >         /* check for having reached the LINES number of printed lines */
> >         if (kdb_nextline >= linecount) {
> > -               char buf1[16] = "";
> > +               char ch;
> 
> The type of "ch" should be the same as returned by kdb_getchar()?
> Either "int" if you're keeping it "int" or "unsigned char"?

Probably... although the assumption that kdb strings are char * is burnt
in a lot of places so there will still be further tidy up needed.


> >                 /* Watch out for recursion here.  Any routine that calls
> >                  * kdb_printf will come back through here.  And kdb_read
> > @@ -776,39 +767,38 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
> >                 if (logging)
> >                         printk("%s", moreprompt);
> >
> > -               kdb_read(buf1, 2); /* '2' indicates to return
> > -                                   * immediately after getting one key. */
> > +               ch = kdb_getchar();
> >                 kdb_nextline = 1;       /* Really set output line 1 */
> >
> >                 /* empty and reset the buffer: */
> >                 kdb_buffer[0] = '\0';
> >                 next_avail = kdb_buffer;
> >                 size_avail = sizeof(kdb_buffer);
> > -               if ((buf1[0] == 'q') || (buf1[0] == 'Q')) {
> > +               if ((ch == 'q') || (ch == 'Q')) {
> >                         /* user hit q or Q */
> >                         KDB_FLAG_SET(CMD_INTERRUPT); /* command interrupted */
> >                         KDB_STATE_CLEAR(PAGER);
> >                         /* end of command output; back to normal mode */
> >                         kdb_grepping_flag = 0;
> >                         kdb_printf("\n");
> > -               } else if (buf1[0] == ' ') {
> > +               } else if (ch == ' ') {
> >                         kdb_printf("\r");
> >                         suspend_grep = 1; /* for this recursion */
> > -               } else if (buf1[0] == '\n') {
> > +               } else if (ch == '\n' || ch == '\r') {
> >                         kdb_nextline = linecount - 1;
> >                         kdb_printf("\r");
> >                         suspend_grep = 1; /* for this recursion */
> > -               } else if (buf1[0] == '/' && !kdb_grepping_flag) {
> > +               } else if (ch == '/' && !kdb_grepping_flag) {
> >                         kdb_printf("\r");
> >                         kdb_getstr(kdb_grep_string, KDB_GREP_STRLEN,
> >                                    kdbgetenv("SEARCHPROMPT") ?: "search> ");
> >                         *strchrnul(kdb_grep_string, '\n') = '\0';
> >                         kdb_grepping_flag += KDB_GREPPING_FLAG_SEARCH;
> >                         suspend_grep = 1; /* for this recursion */
> > -               } else if (buf1[0] && buf1[0] != '\n') {
> > +               } else if (ch && ch != '\n') {
> 
> Remove "&& ch != '\n'".  We would have hit an earlier case in the
> if/else anyway.  If you really want to keep it here for some reason, I
> guess you should also handle '\r' ?

Let's remove.


Daniel.

Re: [PATCH v2 3/5] kdb: Remove special case logic from kdb_read()

[Doug Anderson]

Hi,

On Wed, Oct 9, 2019 at 2:30 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> > > @@ -183,17 +186,7 @@ static int kdb_read_get_key(char *buffer, size_t bufsize)
> > >   *     function.  It is not reentrant - it relies on the fact
> > >   *     that while kdb is running on only one "master debug" cpu.
> > >   * Remarks:
> > > - *
> > > - * The buffer size must be >= 2.  A buffer size of 2 means that the caller only
> > > - * wants a single key.
> >
> > By removing this you broke "BTAPROMPT".  So doing:
> >
> > set BTAPROMPT=1
> > bta
> >
> > It's now impossible to quit out.  Not that I've ever used BTAPROMPT,
> > but seems like we should either get rid of it or keep it working.
>
> Thanks. Just to check I got exactly what you meant I assume this could
> also have been phrased as "it looks like you forgot to convert the
> kdb_getstr() in kdb_bt1() over to use the new kdb_getchar() function"?

Right.  Sorry for wording it in a confusing way.  ;-)


> > > @@ -741,7 +732,7 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
> > >
> > >         /* check for having reached the LINES number of printed lines */
> > >         if (kdb_nextline >= linecount) {
> > > -               char buf1[16] = "";
> > > +               char ch;
> >
> > The type of "ch" should be the same as returned by kdb_getchar()?
> > Either "int" if you're keeping it "int" or "unsigned char"?
>
> Probably... although the assumption that kdb strings are char * is burnt
> in a lot of places so there will still be further tidy up needed.

True.  It doesn't matter a whole lot so if you think it's easier to
keep it as char that's OK too.

-Doug

Re: [PATCH v2 3/5] kdb: Remove special case logic from kdb_read()

[Daniel Thompson]

On Wed, Oct 09, 2019 at 10:28:36AM -0700, Doug Anderson wrote:
> On Wed, Oct 9, 2019 at 2:30 AM Daniel Thompson
> <daniel.thompson@linaro.org> wrote:
> > > > @@ -741,7 +732,7 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
> > > >
> > > >         /* check for having reached the LINES number of printed lines */
> > > >         if (kdb_nextline >= linecount) {
> > > > -               char buf1[16] = "";
> > > > +               char ch;
> > >
> > > The type of "ch" should be the same as returned by kdb_getchar()?
> > > Either "int" if you're keeping it "int" or "unsigned char"?
> >
> > Probably... although the assumption that kdb strings are char * is burnt
> > in a lot of places so there will still be further tidy up needed.
> 
> True.  It doesn't matter a whole lot so if you think it's easier to
> keep it as char that's OK too.

After looking at it from a number of angles I think we can have this
match the return type of kdb_getchar()... but the best way to achieve
this is to make kdb_getchar() return a unqualified char.

That ends up consistent across the sub-system and shouldn't do any
narrowing that wouldn't already have been happening inside kdb_read().


Daniel.

[PATCH v2 4/5] kdb: Improve handling of characters from different input sources

[Daniel Thompson]

Currently if an escape timer is interrupted by a character from a
different input source then the new character is discarded and the
function returns '\e' (which will be discarded by the level above).
It is hard to see why this would ever be the desired behaviour.
Fix this to return the new character rather then the '\e'.

This is a bigger refactor that might be expected because the new
character needs to go through escape sequence detection.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
---
 kernel/debug/kdb/kdb_io.c | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index a9e73bc9d1c3..288dd1babf90 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -122,8 +122,8 @@ static int kdb_getchar(void)
 {
 #define ESCAPE_UDELAY 1000
 #define ESCAPE_DELAY (2*1000000/ESCAPE_UDELAY) /* 2 seconds worth of udelays */
-	char escape_data[5];	/* longest vt100 escape sequence is 4 bytes */
-	char *ped = escape_data;
+	char buf[4];	/* longest vt100 escape sequence is 4 bytes */
+	char *pbuf = buf;
 	int escape_delay = 0;
 	get_char_func *f, *f_escape = NULL;
 	int key;
@@ -145,27 +145,26 @@ static int kdb_getchar(void)
 			continue;
 		}
 
-		if (escape_delay == 0 && key == '\e') {
-			escape_delay = ESCAPE_DELAY;
-			ped = escape_data;
+		/*
+		 * When the first character is received (or we get a change
+		 * input source) we set ourselves up to handle an escape
+		 * sequences (just in case).
+		 */
+		if (f_escape != f) {
 			f_escape = f;
-		}
-		if (escape_delay) {
-			if (f_escape != f)
-				return '\e';
-
-			*ped++ = key;
-			key = kdb_read_handle_escape(escape_data,
-						     ped - escape_data);
-			if (key < 0)
-				return '\e';
-			if (key == 0)
-				continue;
+			pbuf = buf;
+			escape_delay = ESCAPE_DELAY;
 		}
 
-		break;	/* A key to process */
+		*pbuf++ = key;
+		key = kdb_read_handle_escape(buf, pbuf - buf);
+		if (key < 0) /* no escape sequence; return first character */
+			return buf[0];
+		if (key > 0)
+			return key;
 	}
-	return key;
+
+	unreachable();
 }
 
 /*
-- 
2.21.0

Re: [PATCH v2 4/5] kdb: Improve handling of characters from different input sources

[Doug Anderson]

Hi,

On Tue, Oct 8, 2019 at 6:21 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> Currently if an escape timer is interrupted by a character from a
> different input source then the new character is discarded and the
> function returns '\e' (which will be discarded by the level above).
> It is hard to see why this would ever be the desired behaviour.

I guess the 2nd input source would be if you enable keyboard input?
Personally I've never used this myself, but your functional change
seems OK to me.


> Fix this to return the new character rather then the '\e'.

s/then/than


> This is a bigger refactor that might be expected because the new
> character needs to go through escape sequence detection.
>
> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> ---
>  kernel/debug/kdb/kdb_io.c | 37 ++++++++++++++++++-------------------
>  1 file changed, 18 insertions(+), 19 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index a9e73bc9d1c3..288dd1babf90 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -122,8 +122,8 @@ static int kdb_getchar(void)
>  {
>  #define ESCAPE_UDELAY 1000
>  #define ESCAPE_DELAY (2*1000000/ESCAPE_UDELAY) /* 2 seconds worth of udelays */
> -       char escape_data[5];    /* longest vt100 escape sequence is 4 bytes */
> -       char *ped = escape_data;
> +       char buf[4];    /* longest vt100 escape sequence is 4 bytes */
> +       char *pbuf = buf;
>         int escape_delay = 0;
>         get_char_func *f, *f_escape = NULL;
>         int key;
> @@ -145,27 +145,26 @@ static int kdb_getchar(void)
>                         continue;
>                 }
>
> -               if (escape_delay == 0 && key == '\e') {
> -                       escape_delay = ESCAPE_DELAY;
> -                       ped = escape_data;
> +               /*
> +                * When the first character is received (or we get a change
> +                * input source) we set ourselves up to handle an escape
> +                * sequences (just in case).
> +                */
> +               if (f_escape != f) {
>                         f_escape = f;

Would it make sense to rename "f_escape" to "f_last" or "f_prev" now?
Essentially this logic now happens every time you change input
sources.


-Doug

[PATCH v2 5/5] kdb: Tweak escape handling for vi users

[Daniel Thompson]

Currently if sequences such as "\ehelp\r" are delivered to the console then
the h gets eaten by the escape handling code. Since pressing escape
becomes something of a nervous twitch for vi users (and that escape doesn't
have much effect at a shell prompt) it is more helpful to emit the 'h' than
the '\e'.

We don't simply choose to emit the final character for all escape sequences
since that will do odd things for unsupported escape sequences (in
other words we retain the existing behaviour once we see '\e[').

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
---
 kernel/debug/kdb/kdb_io.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 288dd1babf90..b3fb88b1ee34 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -158,8 +158,8 @@ static int kdb_getchar(void)
 
 		*pbuf++ = key;
 		key = kdb_read_handle_escape(buf, pbuf - buf);
-		if (key < 0) /* no escape sequence; return first character */
-			return buf[0];
+		if (key < 0) /* no escape sequence; return best character */
+			return buf[pbuf - buf != 2 ? 0 : 1];
 		if (key > 0)
 			return key;
 	}
-- 
2.21.0

Re: [PATCH v2 5/5] kdb: Tweak escape handling for vi users

[Doug Anderson]

Hi,

On Tue, Oct 8, 2019 at 6:21 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> Currently if sequences such as "\ehelp\r" are delivered to the console then
> the h gets eaten by the escape handling code. Since pressing escape
> becomes something of a nervous twitch for vi users (and that escape doesn't
> have much effect at a shell prompt) it is more helpful to emit the 'h' than
> the '\e'.

I have no objection to this change.


> We don't simply choose to emit the final character for all escape sequences
> since that will do odd things for unsupported escape sequences (in
> other words we retain the existing behaviour once we see '\e[').

It's not like it handles unsupported escape sequences terribly well
anyway, of course.  As soon as if finds something it doesn't recognize
then it stops processing the escape sequence and will just interpret
the rest of it verbatim.  Like if I press Ctrl-Home on my keyboard I
see "5H" spit out, for instance.


> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> ---
>  kernel/debug/kdb/kdb_io.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 288dd1babf90..b3fb88b1ee34 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -158,8 +158,8 @@ static int kdb_getchar(void)
>
>                 *pbuf++ = key;
>                 key = kdb_read_handle_escape(buf, pbuf - buf);
> -               if (key < 0) /* no escape sequence; return first character */
> -                       return buf[0];
> +               if (key < 0) /* no escape sequence; return best character */
> +                       return buf[pbuf - buf != 2 ? 0 : 1];

optional nit: for me the inverse is easier to conceptualize, AKA:

buf[pbuf - buf == 2 ? 1 : 0];

-Doug