Currently when kdb_read() needs to reposition the cursor it uses copy and
paste code that works by injecting an '\0' at the cursor position before
delivering a carriage-return and reprinting the line (which stops at the
'\0').
Tidy up the code by hoisting the copy and paste code into an appropriately
named function. Additionally let's replace the '\0' injection with a
proper field width parameter so that the string will be abridged during
formatting instead.
Cc: [email protected] # Not a bug fix but it is needed for later bug fixes
Signed-off-by: Daniel Thompson <[email protected]>
---
kernel/debug/kdb/kdb_io.c | 34 ++++++++++++++--------------------
1 file changed, 14 insertions(+), 20 deletions(-)
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 06dfbccb10336..a42607e4d1aba 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -184,6 +184,13 @@ char kdb_getchar(void)
unreachable();
}
+static void kdb_position_cursor(char *prompt, char *buffer, char *cp)
+{
+ kdb_printf("\r%s", kdb_prompt_str);
+ if (cp > buffer)
+ kdb_printf("%.*s", (int)(cp - buffer), buffer);
+}
+
/*
* kdb_read
*
@@ -249,12 +256,8 @@ static char *kdb_read(char *buffer, size_t bufsize)
}
*(--lastchar) = '\0';
--cp;
- kdb_printf("\b%s \r", cp);
- tmp = *cp;
- *cp = '\0';
- kdb_printf(kdb_prompt_str);
- kdb_printf("%s", buffer);
- *cp = tmp;
+ kdb_printf("\b%s ", cp);
+ kdb_position_cursor(kdb_prompt_str, buffer, cp);
}
break;
case 10: /* linefeed */
@@ -272,19 +275,14 @@ static char *kdb_read(char *buffer, size_t bufsize)
memcpy(tmpbuffer, cp+1, lastchar - cp - 1);
memcpy(cp, tmpbuffer, lastchar - cp - 1);
*(--lastchar) = '\0';
- kdb_printf("%s \r", cp);
- tmp = *cp;
- *cp = '\0';
- kdb_printf(kdb_prompt_str);
- kdb_printf("%s", buffer);
- *cp = tmp;
+ kdb_printf("%s ", cp);
+ kdb_position_cursor(kdb_prompt_str, buffer, cp);
}
break;
case 1: /* Home */
if (cp > buffer) {
- kdb_printf("\r");
- kdb_printf(kdb_prompt_str);
cp = buffer;
+ kdb_position_cursor(kdb_prompt_str, buffer, cp);
}
break;
case 5: /* End */
@@ -390,13 +388,9 @@ static char *kdb_read(char *buffer, size_t bufsize)
memcpy(cp+1, tmpbuffer, lastchar - cp);
*++lastchar = '\0';
*cp = key;
- kdb_printf("%s\r", cp);
+ kdb_printf("%s", cp);
++cp;
- tmp = *cp;
- *cp = '\0';
- kdb_printf(kdb_prompt_str);
- kdb_printf("%s", buffer);
- *cp = tmp;
+ kdb_position_cursor(kdb_prompt_str, buffer, cp);
} else {
*++lastchar = '\0';
*cp++ = key;
--
2.43.0
Hi,
On Mon, Apr 22, 2024 at 9:37 AM Daniel Thompson
<[email protected]> wrote:
>
> Currently when kdb_read() needs to reposition the cursor it uses copy and
> paste code that works by injecting an '\0' at the cursor position before
> delivering a carriage-return and reprinting the line (which stops at the
> '\0').
>
> Tidy up the code by hoisting the copy and paste code into an appropriately
> named function. Additionally let's replace the '\0' injection with a
> proper field width parameter so that the string will be abridged during
> formatting instead.
>
> Cc: [email protected] # Not a bug fix but it is needed for later bug fixes
> Signed-off-by: Daniel Thompson <[email protected]>
> ---
> kernel/debug/kdb/kdb_io.c | 34 ++++++++++++++--------------------
> 1 file changed, 14 insertions(+), 20 deletions(-)
Looks like a nice fix, but I think this'll create a compile warning on
some compilers. The variable "tmp" is no longer used, I think.
Once the "tmp" variable is deleted, feel free to add my Reviewed-by.
NOTE: patch #7 in your series re-adds a user of "tmp", but since this
one is "Cc: stable" you will need to delete it here and then re-add it
in patch #7.
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 06dfbccb10336..a42607e4d1aba 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -184,6 +184,13 @@ char kdb_getchar(void)
> unreachable();
> }
>
> +static void kdb_position_cursor(char *prompt, char *buffer, char *cp)
> +{
> + kdb_printf("\r%s", kdb_prompt_str);
> + if (cp > buffer)
> + kdb_printf("%.*s", (int)(cp - buffer), buffer);
nit: personally, I'd take the "if" statement out. I'm nearly certain
that kdb_printf() can handle zero-length for the width argument and
"buffer" can never be _after_ cp (so you can't get negative).
-Doug
On Mon, Apr 22, 2024 at 04:52:04PM -0700, Doug Anderson wrote:
> Hi,
>
> On Mon, Apr 22, 2024 at 9:37 AM Daniel Thompson
> <[email protected]> wrote:
> >
> > Currently when kdb_read() needs to reposition the cursor it uses copy and
> > paste code that works by injecting an '\0' at the cursor position before
> > delivering a carriage-return and reprinting the line (which stops at the
> > '\0').
> >
> > Tidy up the code by hoisting the copy and paste code into an appropriately
> > named function. Additionally let's replace the '\0' injection with a
> > proper field width parameter so that the string will be abridged during
> > formatting instead.
> >
> > Cc: [email protected] # Not a bug fix but it is needed for later bug fixes
> > Signed-off-by: Daniel Thompson <[email protected]>
> > ---
> > kernel/debug/kdb/kdb_io.c | 34 ++++++++++++++--------------------
> > 1 file changed, 14 insertions(+), 20 deletions(-)
>
> Looks like a nice fix, but I think this'll create a compile warning on
> some compilers. The variable "tmp" is no longer used, I think.
>
> Once the "tmp" variable is deleted, feel free to add my Reviewed-by.
Good spot. I'll fix that.
> NOTE: patch #7 in your series re-adds a user of "tmp", but since this
> one is "Cc: stable" you will need to delete it here and then re-add it
> in patch #7.
>
>
> > diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> > index 06dfbccb10336..a42607e4d1aba 100644
> > --- a/kernel/debug/kdb/kdb_io.c
> > +++ b/kernel/debug/kdb/kdb_io.c
> > @@ -184,6 +184,13 @@ char kdb_getchar(void)
> > unreachable();
> > }
> >
> > +static void kdb_position_cursor(char *prompt, char *buffer, char *cp)
> > +{
> > + kdb_printf("\r%s", kdb_prompt_str);
> > + if (cp > buffer)
> > + kdb_printf("%.*s", (int)(cp - buffer), buffer);
>
> nit: personally, I'd take the "if" statement out. I'm nearly certain
> that kdb_printf() can handle zero-length for the width argument and
> "buffer" can never be _after_ cp (so you can't get negative).
The kernel will correctly format zero-width fields... but kdb_printf()
will also inject the empty string into the log if running with
LOGGING=1. It is true the dmesg output with LOGGING=1 is pretty nasty
when doing line editing but I still didn't want to make it worse!
Oh... and we can't combine into one call to kdb_printf() since that
renders into a fixed length static buffer and we could get unwanted
truncation. I might just add a comment about that.
Daniel.