[PATCH] kdb: Refactor kdb_defcmd implementation

[PATCH] kdb: Refactor kdb_defcmd implementation

[Sumit Garg]

Switch to use kdbtab_t instead of separate struct defcmd_set since
now we have kdb_register_table() to register pre-allocated kdb commands.

Also, switch to use a linked list for sub-commands instead of dynamic
array which makes traversing the sub-commands list simpler.

Suggested-by: Daniel Thompson <daniel.thompson@linaro.org>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 kernel/debug/kdb/kdb_main.c    | 136 +++++++++++++++------------------
 kernel/debug/kdb/kdb_private.h |   7 ++
 2 files changed, 70 insertions(+), 73 deletions(-)

diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index 9d69169582c6..2f54e81fd9f7 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -33,7 +33,6 @@
 #include <linux/kallsyms.h>
 #include <linux/kgdb.h>
 #include <linux/kdb.h>
-#include <linux/list.h>
 #include <linux/notifier.h>
 #include <linux/interrupt.h>
 #include <linux/delay.h>
@@ -678,16 +677,7 @@ static void kdb_cmderror(int diag)
  * Returns:
  *	zero for success, a kdb diagnostic if error
  */
-struct defcmd_set {
-	int count;
-	bool usable;
-	char *name;
-	char *usage;
-	char *help;
-	char **command;
-};
-static struct defcmd_set *defcmd_set;
-static int defcmd_set_count;
+static kdbtab_t *defcmd_set;
 static bool defcmd_in_progress;
 
 /* Forward references */
@@ -695,53 +685,52 @@ static int kdb_exec_defcmd(int argc, const char **argv);
 
 static int kdb_defcmd2(const char *cmdstr, const char *argv0)
 {
-	struct defcmd_set *s = defcmd_set + defcmd_set_count - 1;
-	char **save_command = s->command;
+	struct kdb_subcmd *ks;
+
+	if (!defcmd_set)
+		return KDB_NOTIMP;
+
 	if (strcmp(argv0, "endefcmd") == 0) {
 		defcmd_in_progress = false;
-		if (!s->count)
-			s->usable = false;
-		if (s->usable)
-			/* macros are always safe because when executed each
-			 * internal command re-enters kdb_parse() and is
-			 * safety checked individually.
-			 */
-			kdb_register_flags(s->name, kdb_exec_defcmd, s->usage,
-					   s->help, 0,
-					   KDB_ENABLE_ALWAYS_SAFE);
+		if (!list_empty(&defcmd_set->kdb_scmds_head))
+			kdb_register_table(defcmd_set, 1);
 		return 0;
 	}
-	if (!s->usable)
-		return KDB_NOTIMP;
-	s->command = kcalloc(s->count + 1, sizeof(*(s->command)), GFP_KDB);
-	if (!s->command) {
+
+	ks = kmalloc(sizeof(*ks), GFP_KDB);
+	if (!ks) {
 		kdb_printf("Could not allocate new kdb_defcmd table for %s\n",
 			   cmdstr);
-		s->usable = false;
 		return KDB_NOTIMP;
 	}
-	memcpy(s->command, save_command, s->count * sizeof(*(s->command)));
-	s->command[s->count++] = kdb_strdup(cmdstr, GFP_KDB);
-	kfree(save_command);
+
+	ks->scmd_name = kdb_strdup(cmdstr, GFP_KDB);
+	list_add_tail(&ks->list_node, &defcmd_set->kdb_scmds_head);
+
 	return 0;
 }
 
 static int kdb_defcmd(int argc, const char **argv)
 {
-	struct defcmd_set *save_defcmd_set = defcmd_set, *s;
 	if (defcmd_in_progress) {
 		kdb_printf("kdb: nested defcmd detected, assuming missing "
 			   "endefcmd\n");
 		kdb_defcmd2("endefcmd", "endefcmd");
 	}
 	if (argc == 0) {
-		int i;
-		for (s = defcmd_set; s < defcmd_set + defcmd_set_count; ++s) {
-			kdb_printf("defcmd %s \"%s\" \"%s\"\n", s->name,
-				   s->usage, s->help);
-			for (i = 0; i < s->count; ++i)
-				kdb_printf("%s", s->command[i]);
-			kdb_printf("endefcmd\n");
+		kdbtab_t *kp;
+		struct kdb_subcmd *ks;
+
+		list_for_each_entry(kp, &kdb_cmds_head, list_node) {
+			if (kp->cmd_func == kdb_exec_defcmd) {
+				kdb_printf("defcmd %s \"%s\" \"%s\"\n",
+					   kp->cmd_name, kp->cmd_usage,
+					   kp->cmd_help);
+				list_for_each_entry(ks, &kp->kdb_scmds_head,
+						    list_node)
+					kdb_printf("%s", ks->scmd_name);
+				kdb_printf("endefcmd\n");
+			}
 		}
 		return 0;
 	}
@@ -751,45 +740,42 @@ static int kdb_defcmd(int argc, const char **argv)
 		kdb_printf("Command only available during kdb_init()\n");
 		return KDB_NOTIMP;
 	}
-	defcmd_set = kmalloc_array(defcmd_set_count + 1, sizeof(*defcmd_set),
-				   GFP_KDB);
+	defcmd_set = kzalloc(sizeof(*defcmd_set), GFP_KDB);
 	if (!defcmd_set)
 		goto fail_defcmd;
-	memcpy(defcmd_set, save_defcmd_set,
-	       defcmd_set_count * sizeof(*defcmd_set));
-	s = defcmd_set + defcmd_set_count;
-	memset(s, 0, sizeof(*s));
-	s->usable = true;
-	s->name = kdb_strdup(argv[1], GFP_KDB);
-	if (!s->name)
+
+	defcmd_set->cmd_func = kdb_exec_defcmd;
+	defcmd_set->cmd_minlen = 0;
+	defcmd_set->cmd_flags = KDB_ENABLE_ALWAYS_SAFE;
+	defcmd_set->cmd_name = kdb_strdup(argv[1], GFP_KDB);
+	if (!defcmd_set->cmd_name)
 		goto fail_name;
-	s->usage = kdb_strdup(argv[2], GFP_KDB);
-	if (!s->usage)
+	defcmd_set->cmd_usage = kdb_strdup(argv[2], GFP_KDB);
+	if (!defcmd_set->cmd_usage)
 		goto fail_usage;
-	s->help = kdb_strdup(argv[3], GFP_KDB);
-	if (!s->help)
+	defcmd_set->cmd_help = kdb_strdup(argv[3], GFP_KDB);
+	if (!defcmd_set->cmd_help)
 		goto fail_help;
-	if (s->usage[0] == '"') {
-		strcpy(s->usage, argv[2]+1);
-		s->usage[strlen(s->usage)-1] = '\0';
+	if (defcmd_set->cmd_usage[0] == '"') {
+		strcpy(defcmd_set->cmd_usage, argv[2]+1);
+		defcmd_set->cmd_usage[strlen(defcmd_set->cmd_usage)-1] = '\0';
 	}
-	if (s->help[0] == '"') {
-		strcpy(s->help, argv[3]+1);
-		s->help[strlen(s->help)-1] = '\0';
+	if (defcmd_set->cmd_help[0] == '"') {
+		strcpy(defcmd_set->cmd_help, argv[3]+1);
+		defcmd_set->cmd_help[strlen(defcmd_set->cmd_help)-1] = '\0';
 	}
-	++defcmd_set_count;
+
+	INIT_LIST_HEAD(&defcmd_set->kdb_scmds_head);
 	defcmd_in_progress = true;
-	kfree(save_defcmd_set);
 	return 0;
 fail_help:
-	kfree(s->usage);
+	kfree(defcmd_set->cmd_usage);
 fail_usage:
-	kfree(s->name);
+	kfree(defcmd_set->cmd_name);
 fail_name:
 	kfree(defcmd_set);
 fail_defcmd:
 	kdb_printf("Could not allocate new defcmd_set entry for %s\n", argv[1]);
-	defcmd_set = save_defcmd_set;
 	return KDB_NOTIMP;
 }
 
@@ -804,25 +790,29 @@ static int kdb_defcmd(int argc, const char **argv)
  */
 static int kdb_exec_defcmd(int argc, const char **argv)
 {
-	int i, ret;
-	struct defcmd_set *s;
+	int ret;
+	kdbtab_t *kp;
+	struct kdb_subcmd *ks;
+
 	if (argc != 0)
 		return KDB_ARGCOUNT;
-	for (s = defcmd_set, i = 0; i < defcmd_set_count; ++i, ++s) {
-		if (strcmp(s->name, argv[0]) == 0)
+
+	list_for_each_entry(kp, &kdb_cmds_head, list_node) {
+		if (strcmp(kp->cmd_name, argv[0]) == 0)
 			break;
 	}
-	if (i == defcmd_set_count) {
+	if (list_entry_is_head(kp, &kdb_cmds_head, list_node)) {
 		kdb_printf("kdb_exec_defcmd: could not find commands for %s\n",
 			   argv[0]);
 		return KDB_NOTIMP;
 	}
-	for (i = 0; i < s->count; ++i) {
-		/* Recursive use of kdb_parse, do not use argv after
-		 * this point */
+	list_for_each_entry(ks, &kp->kdb_scmds_head, list_node) {
+		/*
+		 * Recursive use of kdb_parse, do not use argv after this point.
+		 */
 		argv = NULL;
-		kdb_printf("[%s]kdb> %s\n", s->name, s->command[i]);
-		ret = kdb_parse(s->command[i]);
+		kdb_printf("[%s]kdb> %s\n", kp->cmd_name, ks->scmd_name);
+		ret = kdb_parse(ks->scmd_name);
 		if (ret)
 			return ret;
 	}
diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h
index ec91d7e02334..a8bda278c9c1 100644
--- a/kernel/debug/kdb/kdb_private.h
+++ b/kernel/debug/kdb/kdb_private.h
@@ -13,6 +13,7 @@
  */
 
 #include <linux/kgdb.h>
+#include <linux/list.h>
 #include "../debug_core.h"
 
 /* Kernel Debugger Command codes.  Must not overlap with error codes. */
@@ -164,6 +165,11 @@ typedef struct _kdb_bp {
 #ifdef CONFIG_KGDB_KDB
 extern kdb_bp_t kdb_breakpoints[/* KDB_MAXBPT */];
 
+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;
 
 extern void kdb_register_table(kdbtab_t *kp, size_t len);
-- 
2.25.1

Re: [PATCH] kdb: Refactor kdb_defcmd implementation

[Daniel Thompson]

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.

Re: [PATCH] kdb: Refactor kdb_defcmd implementation

[Sumit Garg]

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