Re: [PATCH v2] kdb: Simplify kdb commands registration

[Doug Anderson]

Hi,

On Sun, Jan 24, 2021 at 11:06 PM Sumit Garg <sumit.garg@xxxxxxxxxx> wrote:
>
> Simplify kdb commands registration via using linked list instead of
> static array for commands storage.
>
> Signed-off-by: Sumit Garg <sumit.garg@xxxxxxxxxx>
> ---
>
> 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 <linux/kallsyms.h>
>  #include <linux/kgdb.h>
>  #include <linux/kdb.h>
> +#include <linux/list.h>
>  #include <linux/notifier.h>
>  #include <linux/interrupt.h>
>  #include <linux/delay.h>
> @@ -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, &kdb_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 <dianders@xxxxxxxxxxxx>