linux-kernel - Re: [PATCH] kdb: Refactor kdb

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:   Tue, 23 Mar 2021 11:43:50 +0530
From:   Sumit Garg <sumit.garg@...aro.org>
To:     Daniel Thompson <daniel.thompson@...aro.org>
Cc:     kgdb-bugreport@...ts.sourceforge.net,
        Jason Wessel <jason.wessel@...driver.com>,
        Douglas Anderson <dianders@...omium.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] kdb: Refactor kdb_defcmd implementation

On Fri, 19 Mar 2021 at 22:47, Daniel Thompson
<daniel.thompson@...aro.org> wrote:
>
> On Tue, Mar 09, 2021 at 05:47:47PM +0530, Sumit Garg wrote:
> > Switch to use kdbtab_t instead of separate struct defcmd_set since
> > now we have kdb_register_table() to register pre-allocated kdb commands.
>
> This needs rewriting. I've been struggling for some time to figure out
> what it actually means means and how it related to the patch. I'm
> starting to conclude that this might not be my fault!
>

Okay.

>
> > Also, switch to use a linked list for sub-commands instead of dynamic
> > array which makes traversing the sub-commands list simpler.
>
> We can't call these things sub-commands! These days a sub-commands
> implies something like `git subcommand` and kdb doesn't have anything
> like that.
>

To me, defcmd_set implied that we are defining a kdb command which
will run a list of other kdb commands which I termed as sub-commands
here. But yes I agree with you that these don't resemble `git
subcommand`.

>
> > +struct kdb_subcmd {
> > +     char    *scmd_name;             /* Sub-command name */
> > +     struct  list_head list_node;    /* Sub-command node */
> > +};
> > +
> >  /* The KDB shell command table */
> >  typedef struct _kdbtab {
> >       char    *cmd_name;              /* Command name */
> > @@ -175,6 +181,7 @@ typedef struct _kdbtab {
> >       kdb_cmdflags_t cmd_flags;       /* Command behaviour flags */
> >       struct list_head list_node;     /* Command list */
> >       bool    is_dynamic;             /* Command table allocation type */
> > +     struct list_head kdb_scmds_head; /* Sub-commands list */
> >  } kdbtab_t;
>
> Perhaps this should be more like:
>
> struct defcmd_set {
>         kdbtab_t cmd;
>         struct list_head commands;
>
> };
>
> This still gets registers using kdb_register_table() but it keeps the
> macro code all in once place:
>
> kdb_register_table(&macro->cmd, 1);
>
> I think that is what I *meant* to suggest ;-) . It also avoids having to
> talk about sub-commands!

Okay, I will use this struct instead.

> BTW I'm open to giving defcmd_set a better name
> (kdb_macro?)
>

kdb_macro sounds more appropriate.

> but I don't see why we want to give all commands a macro
> list.

I am not sure if I follow you here but I think it's better to
distinguish between a normal kdb command and a kdb command which is a
super-set (or macro) representing a list of other kdb commands.

-Sumit

>
> Daniel.