lkml.org 
[lkml]   [2019]   [Oct]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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.

\
 
 \ /
  Last update: 2019-10-09 11:31    [W:2.009 / U:0.360 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site