Re: [PATCH v2] kdb: Simplify kdb commands registration
On Mon, 25 Jan 2021 at 21:32, Doug Anderson wrote: > > Hi, > > On Sun, Jan 24, 2021 at 11:06 PM Sumit Garg wrote: > > > > Simplify kdb commands registration via using linked list instead of > > static array for commands storage. > > > > Signed-off-by: Sumit Garg > > --- > > > > Changes in v2: > > - Remove redundant NULL check for "cmd_name". > > - Incorporate misc. comment. > > > > kernel/debug/kdb/kdb_main.c| 119 > > - > > kernel/debug/kdb/kdb_private.h | 1 + > > 2 files changed, 34 insertions(+), 86 deletions(-) > > > > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c > > index 930ac1b..a0989a0 100644 > > --- a/kernel/debug/kdb/kdb_main.c > > +++ b/kernel/debug/kdb/kdb_main.c > > @@ -33,6 +33,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -84,15 +85,8 @@ static unsigned int kdb_continue_catastrophic = > > static unsigned int kdb_continue_catastrophic; > > #endif > > > > -/* kdb_commands describes the available commands. */ > > -static kdbtab_t *kdb_commands; > > -#define KDB_BASE_CMD_MAX 50 > > -static int kdb_max_commands = KDB_BASE_CMD_MAX; > > -static kdbtab_t kdb_base_commands[KDB_BASE_CMD_MAX]; > > -#define for_each_kdbcmd(cmd, num) \ > > - for ((cmd) = kdb_base_commands, (num) = 0; \ > > -num < kdb_max_commands;\ > > -num++, num == KDB_BASE_CMD_MAX ? cmd = kdb_commands : cmd++) > > +/* kdb_cmds_head describes the available commands. */ > > +static LIST_HEAD(kdb_cmds_head); > > > > typedef struct _kdbmsg { > > int km_diag;/* kdb diagnostic */ > > @@ -921,7 +915,7 @@ int kdb_parse(const char *cmdstr) > > char *cp; > > char *cpp, quoted; > > kdbtab_t *tp; > > - int i, escaped, ignore_errors = 0, check_grep = 0; > > + int escaped, ignore_errors = 0, check_grep = 0; > > > > /* > > * First tokenize the command string. > > @@ -1011,25 +1005,18 @@ int kdb_parse(const char *cmdstr) > > ++argv[0]; > > } > > > > - for_each_kdbcmd(tp, i) { > > - if (tp->cmd_name) { > > - /* > > -* If this command is allowed to be abbreviated, > > -* check to see if this is it. > > -*/ > > - > > - if (tp->cmd_minlen > > -&& (strlen(argv[0]) <= tp->cmd_minlen)) { > > - if (strncmp(argv[0], > > - tp->cmd_name, > > - tp->cmd_minlen) == 0) { > > - break; > > - } > > - } > > - > > - if (strcmp(argv[0], tp->cmd_name) == 0) > > + list_for_each_entry(tp, _cmds_head, list_node) { > > + /* > > +* If this command is allowed to be abbreviated, > > +* check to see if this is it. > > +*/ > > + if (tp->cmd_minlen && (strlen(argv[0]) <= tp->cmd_minlen)) { > > + if (strncmp(argv[0], tp->cmd_name, tp->cmd_minlen) > > == 0) > > break; > > } > > The old code had the same problem, but since you're touching it you could fix? > > if (a) { > if (b) > break; > } > > ...is the same as: > > if (a && b) > break; > Sure, I will fix it in the next version. > In any case, this looks like quite a nice cleanup, so: > > Reviewed-by: Douglas Anderson Thanks, Sumit
Re: [PATCH v2] kdb: Simplify kdb commands registration
Hi, On Sun, Jan 24, 2021 at 11:06 PM Sumit Garg wrote: > > Simplify kdb commands registration via using linked list instead of > static array for commands storage. > > Signed-off-by: Sumit Garg > --- > > Changes in v2: > - Remove redundant NULL check for "cmd_name". > - Incorporate misc. comment. > > kernel/debug/kdb/kdb_main.c| 119 > - > kernel/debug/kdb/kdb_private.h | 1 + > 2 files changed, 34 insertions(+), 86 deletions(-) > > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c > index 930ac1b..a0989a0 100644 > --- a/kernel/debug/kdb/kdb_main.c > +++ b/kernel/debug/kdb/kdb_main.c > @@ -33,6 +33,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -84,15 +85,8 @@ static unsigned int kdb_continue_catastrophic = > static unsigned int kdb_continue_catastrophic; > #endif > > -/* kdb_commands describes the available commands. */ > -static kdbtab_t *kdb_commands; > -#define KDB_BASE_CMD_MAX 50 > -static int kdb_max_commands = KDB_BASE_CMD_MAX; > -static kdbtab_t kdb_base_commands[KDB_BASE_CMD_MAX]; > -#define for_each_kdbcmd(cmd, num) \ > - for ((cmd) = kdb_base_commands, (num) = 0; \ > -num < kdb_max_commands;\ > -num++, num == KDB_BASE_CMD_MAX ? cmd = kdb_commands : cmd++) > +/* kdb_cmds_head describes the available commands. */ > +static LIST_HEAD(kdb_cmds_head); > > typedef struct _kdbmsg { > int km_diag;/* kdb diagnostic */ > @@ -921,7 +915,7 @@ int kdb_parse(const char *cmdstr) > char *cp; > char *cpp, quoted; > kdbtab_t *tp; > - int i, escaped, ignore_errors = 0, check_grep = 0; > + int escaped, ignore_errors = 0, check_grep = 0; > > /* > * First tokenize the command string. > @@ -1011,25 +1005,18 @@ int kdb_parse(const char *cmdstr) > ++argv[0]; > } > > - for_each_kdbcmd(tp, i) { > - if (tp->cmd_name) { > - /* > -* If this command is allowed to be abbreviated, > -* check to see if this is it. > -*/ > - > - if (tp->cmd_minlen > -&& (strlen(argv[0]) <= tp->cmd_minlen)) { > - if (strncmp(argv[0], > - tp->cmd_name, > - tp->cmd_minlen) == 0) { > - break; > - } > - } > - > - if (strcmp(argv[0], tp->cmd_name) == 0) > + list_for_each_entry(tp, _cmds_head, list_node) { > + /* > +* If this command is allowed to be abbreviated, > +* check to see if this is it. > +*/ > + if (tp->cmd_minlen && (strlen(argv[0]) <= tp->cmd_minlen)) { > + if (strncmp(argv[0], tp->cmd_name, tp->cmd_minlen) == > 0) > break; > } The old code had the same problem, but since you're touching it you could fix? if (a) { if (b) break; } ...is the same as: if (a && b) break; In any case, this looks like quite a nice cleanup, so: Reviewed-by: Douglas Anderson
[PATCH v2] kdb: Simplify kdb commands registration
Simplify kdb commands registration via using linked list instead of static array for commands storage. Signed-off-by: Sumit Garg --- Changes in v2: - Remove redundant NULL check for "cmd_name". - Incorporate misc. comment. kernel/debug/kdb/kdb_main.c| 119 - kernel/debug/kdb/kdb_private.h | 1 + 2 files changed, 34 insertions(+), 86 deletions(-) diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c index 930ac1b..a0989a0 100644 --- a/kernel/debug/kdb/kdb_main.c +++ b/kernel/debug/kdb/kdb_main.c @@ -33,6 +33,7 @@ #include #include #include +#include #include #include #include @@ -84,15 +85,8 @@ static unsigned int kdb_continue_catastrophic = static unsigned int kdb_continue_catastrophic; #endif -/* kdb_commands describes the available commands. */ -static kdbtab_t *kdb_commands; -#define KDB_BASE_CMD_MAX 50 -static int kdb_max_commands = KDB_BASE_CMD_MAX; -static kdbtab_t kdb_base_commands[KDB_BASE_CMD_MAX]; -#define for_each_kdbcmd(cmd, num) \ - for ((cmd) = kdb_base_commands, (num) = 0; \ -num < kdb_max_commands;\ -num++, num == KDB_BASE_CMD_MAX ? cmd = kdb_commands : cmd++) +/* kdb_cmds_head describes the available commands. */ +static LIST_HEAD(kdb_cmds_head); typedef struct _kdbmsg { int km_diag;/* kdb diagnostic */ @@ -921,7 +915,7 @@ int kdb_parse(const char *cmdstr) char *cp; char *cpp, quoted; kdbtab_t *tp; - int i, escaped, ignore_errors = 0, check_grep = 0; + int escaped, ignore_errors = 0, check_grep = 0; /* * First tokenize the command string. @@ -1011,25 +1005,18 @@ int kdb_parse(const char *cmdstr) ++argv[0]; } - for_each_kdbcmd(tp, i) { - if (tp->cmd_name) { - /* -* If this command is allowed to be abbreviated, -* check to see if this is it. -*/ - - if (tp->cmd_minlen -&& (strlen(argv[0]) <= tp->cmd_minlen)) { - if (strncmp(argv[0], - tp->cmd_name, - tp->cmd_minlen) == 0) { - break; - } - } - - if (strcmp(argv[0], tp->cmd_name) == 0) + list_for_each_entry(tp, _cmds_head, list_node) { + /* +* If this command is allowed to be abbreviated, +* check to see if this is it. +*/ + if (tp->cmd_minlen && (strlen(argv[0]) <= tp->cmd_minlen)) { + if (strncmp(argv[0], tp->cmd_name, tp->cmd_minlen) == 0) break; } + + if (strcmp(argv[0], tp->cmd_name) == 0) + break; } /* @@ -1037,19 +1024,15 @@ int kdb_parse(const char *cmdstr) * few characters of this match any of the known commands. * e.g., md1c20 should match md. */ - if (i == kdb_max_commands) { - for_each_kdbcmd(tp, i) { - if (tp->cmd_name) { - if (strncmp(argv[0], - tp->cmd_name, - strlen(tp->cmd_name)) == 0) { - break; - } - } + if (list_entry_is_head(tp, _cmds_head, list_node)) { + list_for_each_entry(tp, _cmds_head, list_node) { + if (strncmp(argv[0], tp->cmd_name, + strlen(tp->cmd_name)) == 0) + break; } } - if (i < kdb_max_commands) { + if (!list_entry_is_head(tp, _cmds_head, list_node)) { int result; if (!kdb_check_flags(tp->cmd_flags, kdb_cmd_enabled, argc <= 1)) @@ -2428,17 +2411,14 @@ static int kdb_kgdb(int argc, const char **argv) static int kdb_help(int argc, const char **argv) { kdbtab_t *kt; - int i; kdb_printf("%-15.15s %-20.20s %s\n", "Command", "Usage", "Description"); kdb_printf("-" "-\n"); - for_each_kdbcmd(kt, i) { + list_for_each_entry(kt, _cmds_head, list_node) { char *space = ""; if (KDB_FLAG(CMD_INTERRUPT)) return 0; - if (!kt->cmd_name) - continue; if (!kdb_check_flags(kt->cmd_flags, kdb_cmd_enabled, true)) continue; if