Re: [PATCH v3 1/2] kdb: Get rid of redundant kdb_register_flags()
From: Sumit Garg
Date: Thu May 13 2021 - 07:32:02 EST
On Thu, 13 May 2021 at 16:59, Sumit Garg <sumit.garg@xxxxxxxxxx> wrote:
>
> Commit e4f291b3f7bb ("kdb: Simplify kdb commands registration")
> allowed registration of pre-allocated kdb commands with pointer to
> struct kdbtab_t. Lets switch other users as well to register pre-
> allocated kdb commands via:
> - Changing prototype for kdb_register() to pass a pointer to struct
> kdbtab_t instead.
> - Embed kdbtab_t structure in defcmd_set rather than individual params.
> So while at it rename struct defcmd_set to struct kdb_macro_t as that
> sounds more appropriate given its purpose.
>
> With these changes kdb_register_flags() becomes redundant and hence
> removed. Also, since we have switched all users to register
> pre-allocated commands, "is_dynamic" flag in struct kdbtab_t becomes
> redundant and hence removed as well.
>
Missed Daniel's suggested by tag, so here it is:
Suggested-by: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
> Signed-off-by: Sumit Garg <sumit.garg@xxxxxxxxxx>
> ---
> include/linux/kdb.h | 27 +++--
> kernel/debug/kdb/kdb_main.c | 206 +++++++++++----------------------
> kernel/debug/kdb/kdb_private.h | 13 ---
> kernel/trace/trace_kdb.c | 12 +-
> samples/kdb/kdb_hello.c | 20 ++--
> 5 files changed, 104 insertions(+), 174 deletions(-)
>
> diff --git a/include/linux/kdb.h b/include/linux/kdb.h
> index 0125a677b67f..9e893279b2ea 100644
> --- a/include/linux/kdb.h
> +++ b/include/linux/kdb.h
> @@ -13,6 +13,8 @@
> * Copyright (C) 2009 Jason Wessel <jason.wessel@xxxxxxxxxxxxx>
> */
>
> +#include <linux/list.h>
> +
> /* Shifted versions of the command enable bits are be used if the command
> * has no arguments (see kdb_check_flags). This allows commands, such as
> * go, to have different permissions depending upon whether it is called
> @@ -64,6 +66,17 @@ typedef enum {
>
> typedef int (*kdb_func_t)(int, const char **);
>
> +/* The KDB shell command table */
> +typedef struct _kdbtab {
> + char *cmd_name; /* Command name */
> + kdb_func_t cmd_func; /* Function to execute command */
> + char *cmd_usage; /* Usage String for this command */
> + char *cmd_help; /* Help message for this command */
> + short cmd_minlen; /* Minimum legal # cmd chars required */
> + kdb_cmdflags_t cmd_flags; /* Command behaviour flags */
> + struct list_head list_node; /* Command list */
> +} kdbtab_t;
> +
> #ifdef CONFIG_KGDB_KDB
> #include <linux/init.h>
> #include <linux/sched.h>
> @@ -193,19 +206,13 @@ static inline const char *kdb_walk_kallsyms(loff_t *pos)
> #endif /* ! CONFIG_KALLSYMS */
>
> /* Dynamic kdb shell command registration */
> -extern int kdb_register(char *, kdb_func_t, char *, char *, short);
> -extern int kdb_register_flags(char *, kdb_func_t, char *, char *,
> - short, kdb_cmdflags_t);
> -extern int kdb_unregister(char *);
> +extern int kdb_register(kdbtab_t *cmd);
> +extern int kdb_unregister(kdbtab_t *cmd);
> #else /* ! CONFIG_KGDB_KDB */
> static inline __printf(1, 2) int kdb_printf(const char *fmt, ...) { return 0; }
> static inline void kdb_init(int level) {}
> -static inline int kdb_register(char *cmd, kdb_func_t func, char *usage,
> - char *help, short minlen) { return 0; }
> -static inline int kdb_register_flags(char *cmd, kdb_func_t func, char *usage,
> - char *help, short minlen,
> - kdb_cmdflags_t flags) { return 0; }
> -static inline int kdb_unregister(char *cmd) { return 0; }
> +static inline int kdb_register(kdbtab_t *cmd) { return 0; }
> +static inline int kdb_unregister(kdbtab_t *cmd) { return 0; }
> #endif /* CONFIG_KGDB_KDB */
> enum {
> KDB_NOT_INITIALIZED,
> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> index 1baa96a2ecb8..de685b2a8ce7 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>
> @@ -654,16 +653,14 @@ static void kdb_cmderror(int diag)
> * Returns:
> * zero for success, a kdb diagnostic if error
> */
> -struct defcmd_set {
> +struct kdb_macro_t {
> int count;
> bool usable;
> - char *name;
> - char *usage;
> - char *help;
> + kdbtab_t cmd;
> char **command;
> };
> -static struct defcmd_set *defcmd_set;
> -static int defcmd_set_count;
> +static struct kdb_macro_t *kdb_macro;
> +static int kdb_macro_count;
> static bool defcmd_in_progress;
>
> /* Forward references */
> @@ -671,20 +668,14 @@ 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;
> + struct kdb_macro_t *s = kdb_macro + kdb_macro_count - 1;
> char **save_command = s->command;
> 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);
> + kdb_register(&s->cmd);
> return 0;
> }
> if (!s->usable)
> @@ -704,7 +695,9 @@ static int kdb_defcmd2(const char *cmdstr, const char *argv0)
>
> static int kdb_defcmd(int argc, const char **argv)
> {
> - struct defcmd_set *save_defcmd_set = defcmd_set, *s;
> + struct kdb_macro_t *save_kdb_macro = kdb_macro, *s;
> + kdbtab_t *mp;
> +
> if (defcmd_in_progress) {
> kdb_printf("kdb: nested defcmd detected, assuming missing "
> "endefcmd\n");
> @@ -712,9 +705,9 @@ static int kdb_defcmd(int argc, const char **argv)
> }
> 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 (s = kdb_macro; s < kdb_macro + kdb_macro_count; ++s) {
> + kdb_printf("defcmd %s \"%s\" \"%s\"\n", s->cmd.cmd_name,
> + s->cmd.cmd_usage, s->cmd.cmd_help);
> for (i = 0; i < s->count; ++i)
> kdb_printf("%s", s->command[i]);
> kdb_printf("endefcmd\n");
> @@ -727,45 +720,50 @@ 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),
> + kdb_macro = kmalloc_array(kdb_macro_count + 1, sizeof(*kdb_macro),
> GFP_KDB);
> - if (!defcmd_set)
> + if (!kdb_macro)
> goto fail_defcmd;
> - memcpy(defcmd_set, save_defcmd_set,
> - defcmd_set_count * sizeof(*defcmd_set));
> - s = defcmd_set + defcmd_set_count;
> + memcpy(kdb_macro, save_kdb_macro,
> + kdb_macro_count * sizeof(*kdb_macro));
> + s = kdb_macro + kdb_macro_count;
> memset(s, 0, sizeof(*s));
> s->usable = true;
> - s->name = kdb_strdup(argv[1], GFP_KDB);
> - if (!s->name)
> +
> + mp = &s->cmd;
> + mp->cmd_func = kdb_exec_defcmd;
> + mp->cmd_minlen = 0;
> + mp->cmd_flags = KDB_ENABLE_ALWAYS_SAFE;
> + mp->cmd_name = kdb_strdup(argv[1], GFP_KDB);
> + if (!mp->cmd_name)
> goto fail_name;
> - s->usage = kdb_strdup(argv[2], GFP_KDB);
> - if (!s->usage)
> + mp->cmd_usage = kdb_strdup(argv[2], GFP_KDB);
> + if (!mp->cmd_usage)
> goto fail_usage;
> - s->help = kdb_strdup(argv[3], GFP_KDB);
> - if (!s->help)
> + mp->cmd_help = kdb_strdup(argv[3], GFP_KDB);
> + if (!mp->cmd_help)
> goto fail_help;
> - if (s->usage[0] == '"') {
> - strcpy(s->usage, argv[2]+1);
> - s->usage[strlen(s->usage)-1] = '\0';
> + if (mp->cmd_usage[0] == '"') {
> + strcpy(mp->cmd_usage, argv[2]+1);
> + mp->cmd_usage[strlen(mp->cmd_usage)-1] = '\0';
> }
> - if (s->help[0] == '"') {
> - strcpy(s->help, argv[3]+1);
> - s->help[strlen(s->help)-1] = '\0';
> + if (mp->cmd_help[0] == '"') {
> + strcpy(mp->cmd_help, argv[3]+1);
> + mp->cmd_help[strlen(mp->cmd_help)-1] = '\0';
> }
> - ++defcmd_set_count;
> + ++kdb_macro_count;
> defcmd_in_progress = true;
> - kfree(save_defcmd_set);
> + kfree(save_kdb_macro);
> return 0;
> fail_help:
> - kfree(s->usage);
> + kfree(mp->cmd_usage);
> fail_usage:
> - kfree(s->name);
> + kfree(mp->cmd_name);
> fail_name:
> - kfree(defcmd_set);
> + kfree(kdb_macro);
> fail_defcmd:
> - kdb_printf("Could not allocate new defcmd_set entry for %s\n", argv[1]);
> - defcmd_set = save_defcmd_set;
> + kdb_printf("Could not allocate new kdb_macro entry for %s\n", argv[1]);
> + kdb_macro = save_kdb_macro;
> return KDB_NOTIMP;
> }
>
> @@ -781,14 +779,14 @@ 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;
> + struct kdb_macro_t *s;
> 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)
> + for (s = kdb_macro, i = 0; i < kdb_macro_count; ++i, ++s) {
> + if (strcmp(s->cmd.cmd_name, argv[0]) == 0)
> break;
> }
> - if (i == defcmd_set_count) {
> + if (i == kdb_macro_count) {
> kdb_printf("kdb_exec_defcmd: could not find commands for %s\n",
> argv[0]);
> return KDB_NOTIMP;
> @@ -797,7 +795,7 @@ static int kdb_exec_defcmd(int argc, const char **argv)
> /* 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]);
> + kdb_printf("[%s]kdb> %s\n", s->cmd.cmd_name, s->command[i]);
> ret = kdb_parse(s->command[i]);
> if (ret)
> return ret;
> @@ -2620,55 +2618,6 @@ static int kdb_grep_help(int argc, const char **argv)
> return 0;
> }
>
> -/*
> - * kdb_register_flags - This function is used to register a kernel
> - * debugger command.
> - * Inputs:
> - * cmd Command name
> - * func Function to execute the command
> - * usage A simple usage string showing arguments
> - * help A simple help string describing command
> - * repeat Does the command auto repeat on enter?
> - * Returns:
> - * zero for success, one if a duplicate command.
> - */
> -int kdb_register_flags(char *cmd,
> - kdb_func_t func,
> - char *usage,
> - char *help,
> - short minlen,
> - kdb_cmdflags_t flags)
> -{
> - kdbtab_t *kp;
> -
> - list_for_each_entry(kp, &kdb_cmds_head, list_node) {
> - if (strcmp(kp->cmd_name, cmd) == 0) {
> - kdb_printf("Duplicate kdb command registered: "
> - "%s, func %px help %s\n", cmd, func, help);
> - return 1;
> - }
> - }
> -
> - kp = kmalloc(sizeof(*kp), GFP_KDB);
> - if (!kp) {
> - kdb_printf("Could not allocate new kdb_command table\n");
> - return 1;
> - }
> -
> - kp->cmd_name = cmd;
> - kp->cmd_func = func;
> - kp->cmd_usage = usage;
> - kp->cmd_help = help;
> - kp->cmd_minlen = minlen;
> - kp->cmd_flags = flags;
> - kp->is_dynamic = true;
> -
> - list_add_tail(&kp->list_node, &kdb_cmds_head);
> -
> - return 0;
> -}
> -EXPORT_SYMBOL_GPL(kdb_register_flags);
> -
> /*
> * kdb_register_table() - This function is used to register a kdb command
> * table.
> @@ -2684,54 +2633,37 @@ void kdb_register_table(kdbtab_t *kp, size_t len)
> }
>
> /*
> - * kdb_register - Compatibility register function for commands that do
> - * not need to specify a repeat state. Equivalent to
> - * kdb_register_flags with flags set to 0.
> - * Inputs:
> - * cmd Command name
> - * func Function to execute the command
> - * usage A simple usage string showing arguments
> - * help A simple help string describing command
> - * Returns:
> - * zero for success, one if a duplicate command.
> + * kdb_register() - This function is used to register a kernel debugger
> + * command.
> + * @cmd: pointer to kdb command
> */
> -int kdb_register(char *cmd,
> - kdb_func_t func,
> - char *usage,
> - char *help,
> - short minlen)
> -{
> - return kdb_register_flags(cmd, func, usage, help, minlen, 0);
> -}
> -EXPORT_SYMBOL_GPL(kdb_register);
> -
> -/*
> - * kdb_unregister - This function is used to unregister a kernel
> - * debugger command. It is generally called when a module which
> - * implements kdb commands is unloaded.
> - * Inputs:
> - * cmd Command name
> - * Returns:
> - * zero for success, one command not registered.
> - */
> -int kdb_unregister(char *cmd)
> +int kdb_register(kdbtab_t *cmd)
> {
> kdbtab_t *kp;
>
> - /*
> - * find the command.
> - */
> list_for_each_entry(kp, &kdb_cmds_head, list_node) {
> - if (strcmp(kp->cmd_name, cmd) == 0) {
> - list_del(&kp->list_node);
> - if (kp->is_dynamic)
> - kfree(kp);
> - return 0;
> + if (strcmp(kp->cmd_name, cmd->cmd_name) == 0) {
> + kdb_printf("Duplicate kdb cmd: %s, func %p help %s\n",
> + cmd->cmd_name, cmd->cmd_func, cmd->cmd_help);
> + return 1;
> }
> }
>
> - /* Couldn't find it. */
> - return 1;
> + list_add_tail(&cmd->list_node, &kdb_cmds_head);
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(kdb_register);
> +
> +/*
> + * kdb_unregister() - This function is used to unregister a kernel debugger
> + * command. It is generally called when a module which
> + * implements kdb command is unloaded.
> + * @cmd: pointer to kdb command
> + */
> +int kdb_unregister(kdbtab_t *cmd)
> +{
> + list_del(&cmd->list_node);
> + return 0;
> }
> EXPORT_SYMBOL_GPL(kdb_unregister);
>
> diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h
> index 95d2b730af4d..608d57f3c419 100644
> --- a/kernel/debug/kdb/kdb_private.h
> +++ b/kernel/debug/kdb/kdb_private.h
> @@ -164,19 +164,6 @@ typedef struct _kdb_bp {
> #ifdef CONFIG_KGDB_KDB
> extern kdb_bp_t kdb_breakpoints[/* KDB_MAXBPT */];
>
> -/* The KDB shell command table */
> -typedef struct _kdbtab {
> - char *cmd_name; /* Command name */
> - kdb_func_t cmd_func; /* Function to execute command */
> - char *cmd_usage; /* Usage String for this command */
> - char *cmd_help; /* Help message for this command */
> - short cmd_minlen; /* Minimum legal # command
> - * chars required */
> - kdb_cmdflags_t cmd_flags; /* Command behaviour flags */
> - struct list_head list_node; /* Command list */
> - bool is_dynamic; /* Command table allocation type */
> -} kdbtab_t;
> -
> extern void kdb_register_table(kdbtab_t *kp, size_t len);
> extern int kdb_bt(int, const char **); /* KDB display back trace */
>
> diff --git a/kernel/trace/trace_kdb.c b/kernel/trace/trace_kdb.c
> index 9da76104f7a2..6c4f92c79e43 100644
> --- a/kernel/trace/trace_kdb.c
> +++ b/kernel/trace/trace_kdb.c
> @@ -147,11 +147,17 @@ static int kdb_ftdump(int argc, const char **argv)
> return 0;
> }
>
> +static kdbtab_t ftdump_cmd = {
> + .cmd_name = "ftdump",
> + .cmd_func = kdb_ftdump,
> + .cmd_usage = "[skip_#entries] [cpu]",
> + .cmd_help = "Dump ftrace log; -skip dumps last #entries",
> + .cmd_flags = KDB_ENABLE_ALWAYS_SAFE,
> +};
> +
> static __init int kdb_ftrace_register(void)
> {
> - kdb_register_flags("ftdump", kdb_ftdump, "[skip_#entries] [cpu]",
> - "Dump ftrace log; -skip dumps last #entries", 0,
> - KDB_ENABLE_ALWAYS_SAFE);
> + kdb_register(&ftdump_cmd);
> return 0;
> }
>
> diff --git a/samples/kdb/kdb_hello.c b/samples/kdb/kdb_hello.c
> index c1c2fa0f62c2..9ad514a6648b 100644
> --- a/samples/kdb/kdb_hello.c
> +++ b/samples/kdb/kdb_hello.c
> @@ -28,28 +28,26 @@ static int kdb_hello_cmd(int argc, const char **argv)
> return 0;
> }
>
> +static kdbtab_t hello_cmd = {
> + .cmd_name = "hello",
> + .cmd_func = kdb_hello_cmd,
> + .cmd_usage = "[string]",
> + .cmd_help = "Say Hello World or Hello [string]",
> +};
>
> static int __init kdb_hello_cmd_init(void)
> {
> /*
> * Registration of a dynamically added kdb command is done with
> - * kdb_register() with the arguments being:
> - * 1: The name of the shell command
> - * 2: The function that processes the command
> - * 3: Description of the usage of any arguments
> - * 4: Descriptive text when you run help
> - * 5: Number of characters to complete the command
> - * 0 == type the whole command
> - * 1 == match both "g" and "go" for example
> + * kdb_register().
> */
> - kdb_register("hello", kdb_hello_cmd, "[string]",
> - "Say Hello World or Hello [string]", 0);
> + kdb_register(&hello_cmd);
> return 0;
> }
>
> static void __exit kdb_hello_cmd_exit(void)
> {
> - kdb_unregister("hello");
> + kdb_unregister(&hello_cmd);
> }
>
> module_init(kdb_hello_cmd_init);
> --
> 2.25.1
>