Re: [PATCH] kdb: Refactor kdb_defcmd implementation

[Sumit Garg]

On Fri, 19 Mar 2021 at 22:47, Daniel Thompson
<daniel.thompson@xxxxxxxxxx> 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.