| Message ID | 20191008132043.7966-4-daniel.thompson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | kdb: Cleanup code to read user input and handle escape sequences | expand |
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
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.
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
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.
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");
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(-) -- 2.21.0