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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD=FV=Vrf4v0DKfZ7BAjW3ODoYeuwDu8L0S1kOMbbG+=WiVOnQ@mail.gmail.com>
Date:   Fri, 22 Jan 2021 10:42:38 -0800
From:   Doug Anderson <dianders@...omium.org>
To:     Sumit Garg <sumit.garg@...aro.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

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()


> --- 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..


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

-Doug