On Fri, 19 Mar 2021 at 22:47, Daniel Thompson
<daniel.thomp...@linaro.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(¯o->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.
