Re: [PATCH v4 1/4] kdb: Re-factor kdb_printf() message write code
On Wed, 3 Jun 2020 at 03:02, Doug Anderson wrote: > > Hi, > > On Fri, May 29, 2020 at 4:27 AM Sumit Garg wrote: > > > > Re-factor kdb_printf() message write code in order to avoid duplication > > of code and thereby increase readability. > > > > Signed-off-by: Sumit Garg > > --- > > kernel/debug/kdb/kdb_io.c | 61 > > +-- > > 1 file changed, 32 insertions(+), 29 deletions(-) > > > > diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c > > index 924bc92..e46f33e 100644 > > --- a/kernel/debug/kdb/kdb_io.c > > +++ b/kernel/debug/kdb/kdb_io.c > > @@ -542,6 +542,33 @@ static int kdb_search_string(char *searched, char > > *searchfor) > > return 0; > > } > > > > +static void kdb_io_write(char *cp, int len) > > nit: "const char *" just to make it obvious that we don't modify the string? > > > > +{ > > + if (len == 0) > > + return; > > Remove the above check. It's double-overkill. Not only did you just > check in kdb_msg_write() but also the while loop below will do a > "no-op" just fine even without your check. > I will get rid of kdb_io_write() as per Daniel's comment on patch #4. > > > + > > + while (len--) { > > + dbg_io_ops->write_char(*cp); > > + cp++; > > + } > > +} > > + > > +static void kdb_msg_write(char *msg, int msg_len) > > nit: "const char *" just to make it obvious that we don't modify the string? > Okay. > > Other than those small things, this looks nice to me. Feel free to > add my Reviewed-by tag once small things are fixed. > Thanks. -Sumit > > -Doug
Re: [PATCH v4 1/4] kdb: Re-factor kdb_printf() message write code
Hi, On Fri, May 29, 2020 at 4:27 AM Sumit Garg wrote: > > Re-factor kdb_printf() message write code in order to avoid duplication > of code and thereby increase readability. > > Signed-off-by: Sumit Garg > --- > kernel/debug/kdb/kdb_io.c | 61 > +-- > 1 file changed, 32 insertions(+), 29 deletions(-) > > diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c > index 924bc92..e46f33e 100644 > --- a/kernel/debug/kdb/kdb_io.c > +++ b/kernel/debug/kdb/kdb_io.c > @@ -542,6 +542,33 @@ static int kdb_search_string(char *searched, char > *searchfor) > return 0; > } > > +static void kdb_io_write(char *cp, int len) nit: "const char *" just to make it obvious that we don't modify the string? > +{ > + if (len == 0) > + return; Remove the above check. It's double-overkill. Not only did you just check in kdb_msg_write() but also the while loop below will do a "no-op" just fine even without your check. > + > + while (len--) { > + dbg_io_ops->write_char(*cp); > + cp++; > + } > +} > + > +static void kdb_msg_write(char *msg, int msg_len) nit: "const char *" just to make it obvious that we don't modify the string? Other than those small things, this looks nice to me. Feel free to add my Reviewed-by tag once small things are fixed. -Doug
[PATCH v4 1/4] kdb: Re-factor kdb_printf() message write code
Re-factor kdb_printf() message write code in order to avoid duplication of code and thereby increase readability. Signed-off-by: Sumit Garg --- kernel/debug/kdb/kdb_io.c | 61 +-- 1 file changed, 32 insertions(+), 29 deletions(-) diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c index 924bc92..e46f33e 100644 --- a/kernel/debug/kdb/kdb_io.c +++ b/kernel/debug/kdb/kdb_io.c @@ -542,6 +542,33 @@ static int kdb_search_string(char *searched, char *searchfor) return 0; } +static void kdb_io_write(char *cp, int len) +{ + if (len == 0) + return; + + while (len--) { + dbg_io_ops->write_char(*cp); + cp++; + } +} + +static void kdb_msg_write(char *msg, int msg_len) +{ + struct console *c; + + if (msg_len == 0) + return; + + if (dbg_io_ops && !dbg_io_ops->is_console) + kdb_io_write(msg, msg_len); + + for_each_console(c) { + c->write(c, msg, msg_len); + touch_nmi_watchdog(); + } +} + int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap) { int diag; @@ -553,7 +580,6 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap) int this_cpu, old_cpu; char *cp, *cp2, *cphold = NULL, replaced_byte = ' '; char *moreprompt = "more> "; - struct console *c; unsigned long uninitialized_var(flags); /* Serialize kdb_printf if multiple cpus try to write at once. @@ -687,22 +713,11 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap) */ retlen = strlen(kdb_buffer); cp = (char *) printk_skip_headers(kdb_buffer); - if (!dbg_kdb_mode && kgdb_connected) { + if (!dbg_kdb_mode && kgdb_connected) gdbstub_msg_write(cp, retlen - (cp - kdb_buffer)); - } else { - if (dbg_io_ops && !dbg_io_ops->is_console) { - len = retlen - (cp - kdb_buffer); - cp2 = cp; - while (len--) { - dbg_io_ops->write_char(*cp2); - cp2++; - } - } - for_each_console(c) { - c->write(c, cp, retlen - (cp - kdb_buffer)); - touch_nmi_watchdog(); - } - } + else + kdb_msg_write(cp, retlen - (cp - kdb_buffer)); + if (logging) { saved_loglevel = console_loglevel; console_loglevel = CONSOLE_LOGLEVEL_SILENT; @@ -751,19 +766,7 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap) moreprompt = "more> "; kdb_input_flush(); - - if (dbg_io_ops && !dbg_io_ops->is_console) { - len = strlen(moreprompt); - cp = moreprompt; - while (len--) { - dbg_io_ops->write_char(*cp); - cp++; - } - } - for_each_console(c) { - c->write(c, moreprompt, strlen(moreprompt)); - touch_nmi_watchdog(); - } + kdb_msg_write(moreprompt, strlen(moreprompt)); if (logging) printk("%s", moreprompt); -- 2.7.4