Messages in this thread | | | Date | Wed, 9 Oct 2019 10:30:49 +0100 | From | Daniel Thompson <> | Subject | Re: [PATCH v2 3/5] kdb: Remove special case logic from kdb_read() |
| |
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.
| |