Re: [PATCH] kdb: Simplify kdb commands registration

[Sumit Garg]

Thanks Doug for your review.

On Sat, 23 Jan 2021 at 00:12, Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
>
> Hi,
>
> On Tue, Jan 19, 2021 at 2:50 AM 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>
> > ---
> >  kernel/debug/kdb/kdb_main.c    | 78 ++++++++++--------------------------------
> >  kernel/debug/kdb/kdb_private.h |  1 +
> >  2 files changed, 20 insertions(+), 59 deletions(-)
>
> Wow, nice.  It should have been done this way from the start!  ;-)
>
>
> > @@ -1011,7 +1005,7 @@ int kdb_parse(const char *cmdstr)
> >                 ++argv[0];
> >         }
> >
> > -       for_each_kdbcmd(tp, i) {
> > +       list_for_each_entry(tp, &kdb_cmds_head, list_node) {
> >                 if (tp->cmd_name) {
>
> So I think here (and elsewhere in this file) you can remove this "if
> (...->cmd_name)" stuff now, right?  That was all there because the old
> "remove" used to just NULL out the name to handle gaps and that's no
> longer possible.  If you are really paranoid you could error-check
> kdb_register_flags()
>

Agree, will get rid of this check.

>
> > --- a/kernel/debug/kdb/kdb_private.h
> > +++ b/kernel/debug/kdb/kdb_private.h
> > @@ -174,6 +174,7 @@ typedef struct _kdbtab {
> >         short    cmd_minlen;            /* Minimum legal # command
> >                                          * chars required */
> >         kdb_cmdflags_t cmd_flags;       /* Command behaviour flags */
> > +       struct list_head list_node;
>
> nit: every other entry in this struct has a comment but not yours..
>

Will add a comment here.

>
> Other than those small nits I think this is a great improvement, thanks!
>

Thanks,
Sumit

> -Doug