linux-kernel - Re: [PATCH] kdb: Simplify kdb commands registration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 25 Jan 2021 12:05:32 +0530
From:   Sumit Garg <sumit.garg@...aro.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     kgdb-bugreport@...ts.sourceforge.net,
        Jason Wessel <jason.wessel@...driver.com>,
        Daniel Thompson <daniel.thompson@...aro.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] kdb: Simplify kdb commands registration

Thanks Doug for your review.

On Sat, 23 Jan 2021 at 00:12, Doug Anderson <dianders@...omium.org> wrote:
>
> Hi,
>
> On Tue, Jan 19, 2021 at 2:50 AM Sumit Garg <sumit.garg@...aro.org> wrote:
> >
> > Simplify kdb commands registration via using linked list instead of
> > static array for commands storage.
> >
> > Signed-off-by: Sumit Garg <sumit.garg@...aro.org>
> > ---
> >  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