Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 19 Mar 2021 17:17:12 +0000
From:   Daniel Thompson <daniel.thompson@linaro.org>
To:     Sumit Garg <sumit.garg@linaro.org>
Cc:     kgdb-bugreport@lists.sourceforge.net, jason.wessel@windriver.com,
        dianders@chromium.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] kdb: Refactor kdb_defcmd implementation
Message-ID: <20210319171712.vlkgnmp7cbnayxdn@maple.lan>
References: <20210309121747.859823-1-sumit.garg@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210309121747.859823-1-sumit.garg@linaro.org>
Precedence: bulk

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!


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


> +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! BTW I'm open to giving defcmd_set a better name
(kdb_macro?) but I don't see why we want to give all commands a macro
list.


Daniel.