[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAFA6WYMxUNwAB2sJBeS_Uvy0O43JqNPe_7-+ORGiHB7gBKjzYg@mail.gmail.com>
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(¯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.
Powered by blists - more mailing lists