> If I recall it was painful to find entries in the help listing w/o sorting.

So it's a human reading problem, where ddb spat out the command names in
order that they were in the in-memory struct, and if I wanted to look
over the listing I had to visually scan every one since they weren't in
order?

> Feel free to propose an alternative implementation; noone's wed to 
> what's in the tree.

Here's a patch for 6.x.  The display order for the commands is an
interesting issue; it would also make sense to group commands (perhaps
alphabetically) by the module that defined them, so that when I added a
module all my ddb commands stayed together.  The patch I have here will
leave commands grouped by module, so it would be relatively trivial to
have linker_file_register_ddb add a new kind of dummy command that
specifies the module name for the following set of commands.

N.B. It appears from the FAQ that attachments are acceptable so I'm
sending the patch as an attachment.  Apologies if this is incorrect.

Thanks,
matthew

Index: kern/kern_linker.c
===================================================================
--- kern/kern_linker.c	(revision 108659)
+++ kern/kern_linker.c	(working copy)
@@ -40,43 +40,53 @@ __FBSDID("$FreeBSD: src/sys/kern/kern_li
 #include <sys/proc.h>
 #include <sys/lock.h>
 #include <sys/mutex.h>
 #include <sys/sx.h>
 #include <sys/mac.h>
 #include <sys/module.h>
 #include <sys/linker.h>
 #include <sys/fcntl.h>
 #include <sys/libkern.h>
 #include <sys/namei.h>
 #include <sys/vnode.h>
 #include <sys/sysctl.h>
 #ifdef KDB
 #include <sys/kdb.h>
 #endif
+#ifdef DDB
+#include <ddb/ddb.h>
+#endif /* DDB */
 
 #include <vm/uma.h>
 #include "linker_if.h"
 
 #ifdef KLD_DEBUG
 int kld_debug = 0;
 #endif
 
 /*
  * static char *linker_search_path(const char *name, struct mod_depend
  * *verinfo);
  */
 static const char 	*linker_basename(const char *path);
+#ifdef DDB
+static void linker_file_register_ddb(linker_file_t file);
+static void linker_file_unregister_ddb(linker_file_t file);
+#else /* !... DDB */
+#define linker_file_register_ddb(file)		do { } while (0)
+#define linker_file_unregister_ddb(file)	do { } while (0)
+#endif /* DDB */
 
 /* Metadata from the static kernel */
 SET_DECLARE(modmetadata_set, struct mod_metadata);
 
 MALLOC_DEFINE(M_LINKER, "linker", "kernel linker");
 
 linker_file_t linker_kernel_file;
 
 static struct mtx kld_mtx;	/* kernel linker mutex */
 
 static linker_class_list_t classes;
 linker_file_list_t linker_files;
 static int next_file_id = 1;
 static int linker_no_more_classes = 0;
 
@@ -361,30 +371,36 @@ linker_load_file(const char *filename, l
 		KLD_DPF(FILE, ("linker_load_file: trying to load %s\n",
 		    filename));
 		error = LINKER_LOAD_FILE(lc, filename, &lf);
 		/*
 		 * If we got something other than ENOENT, then it exists but
 		 * we cannot load it for some other reason.
 		 */
 		if (error != ENOENT)
 			foundfile = 1;
 		if (lf) {
 			error = linker_file_register_modules(lf);
 			if (error == EEXIST) {
 				linker_file_unload(lf, LINKER_UNLOAD_FORCE);
 				goto out;
 			}
+			/*
+			 * Register ddb commands before anything, so
+			 * they're available in case sysinit bug
+			 * causes a crash.
+			 */
+			linker_file_register_ddb(lf);
 			linker_file_register_sysctls(lf);
 			linker_file_sysinit(lf);
 			lf->flags |= LINKER_FILE_LINKED;
 			*result = lf;
 			error = 0;
 			goto out;
 		}
 	}
 	/*
 	 * Less than ideal, but tells the user whether it failed to load or
 	 * the module was not found.
 	 */
 	if (foundfile) {
 		/*
 		 * Format not recognized or otherwise unloadable.
@@ -540,30 +556,31 @@ linker_file_unload(linker_file_t file, i
 	for (ml = TAILQ_FIRST(&found_modules); ml; ml = nextml) {
 		nextml = TAILQ_NEXT(ml, link);
 		if (ml->container == file) {
 			TAILQ_REMOVE(&found_modules, ml, link);
 			free(ml, M_LINKER);
 		}
 	}
 
 	/* 
 	 * Don't try to run SYSUNINITs if we are unloaded due to a 
 	 * link error.
 	 */
 	if (file->flags & LINKER_FILE_LINKED) {
 		linker_file_sysuninit(file);
 		linker_file_unregister_sysctls(file);
+		linker_file_unregister_ddb(file);
 	}
 	mtx_lock(&kld_mtx);
 	TAILQ_REMOVE(&linker_files, file, link);
 	mtx_unlock(&kld_mtx);
 
 	if (file->deps) {
 		for (i = 0; i < file->ndeps; i++)
 			linker_file_unload(file->deps[i], flags);
 		free(file->deps, M_LINKER);
 		file->deps = NULL;
 	}
 	for (cp = STAILQ_FIRST(&file->common); cp;
 	    cp = STAILQ_FIRST(&file->common)) {
 		STAILQ_REMOVE(&file->common, cp, common_symbol, link);
 		free(cp, M_LINKER);
@@ -742,31 +759,74 @@ linker_ddb_search_symbol(caddr_t value, 
 		return (ENOENT);
 	}
 }
 
 int
 linker_ddb_symbol_values(c_linker_sym_t sym, linker_symval_t *symval)
 {
 	linker_file_t lf;
 
 	TAILQ_FOREACH(lf, &linker_files, link) {
 		if (LINKER_SYMBOL_VALUES(lf, sym, symval) == 0)
 			return (0);
 	}
 	return (ENOENT);
 }
-#endif
+
+/*
+ * Add and remove db_cmd and db_show_cmd that are defined in a module.
+ */
+static void
+linker_file_register_ddb(linker_file_t file)
+{
+	struct command **c_start, **c_stop;
+	char *set;
+
+	set = "db_cmd_set";
+	if (linker_file_lookup_set(file, set, &c_start, &c_stop, NULL) == 0) {
+		if (db_cmd_add(c_start, c_stop, false) != 0) {
+			printf("Could not allocate memory to add to %s; "
+			    "module %s's db commands are not merged\n",
+			    set, file->filename);
+		}
+	}
+
+	set = "db_show_cmd_set";
+	if (linker_file_lookup_set(file, set, &c_start, &c_stop, NULL) == 0) {
+		if (db_cmd_add(c_start, c_stop, true) != 0) {
+			printf("Could not allocate memory to add to %s; "
+			    "module %s's db commands are not merged\n",
+			    set, file->filename);
+		}
+	}
+}
+
+static void
+linker_file_unregister_ddb(linker_file_t file)
+{
+	struct command **c_start, **c_stop;
+
+	if (linker_file_lookup_set(file, "db_cmd_set", &c_start,
+	    &c_stop, NULL) == 0)
+		db_cmd_rem(c_start, c_stop, false);
+
+	if (linker_file_lookup_set(file, "db_show_cmd_set", &c_start,
+	    &c_stop, NULL) == 0)
+		db_cmd_rem(c_start, c_stop, true);
+}
+
+#endif /* DDB */
 
 /*
  * Syscalls.
  */
 /*
  * MPSAFE
  */
 int
 kldload(struct thread *td, struct kldload_args *uap)
 {
 	char *kldname, *modname;
 	char *pathname = NULL;
 	linker_file_t lf;
 	int error = 0;
 	size_t size;
@@ -1552,30 +1612,31 @@ restart:
 			}
 		}
 		/*
 		 * Now do relocation etc using the symbol search paths
 		 * established by the dependencies
 		 */
 		error = LINKER_LINK_PRELOAD_FINISH(lf);
 		if (error) {
 			printf("KLD file %s - could not finalize loading\n",
 			    lf->filename);
 			TAILQ_REMOVE(&depended_files, lf, loaded);
 			linker_file_unload(lf, LINKER_UNLOAD_FORCE);
 			continue;
 		}
 		linker_file_register_modules(lf);
+		linker_file_register_ddb(lf);
 		if (linker_file_lookup_set(lf, "sysinit_set", &si_start,
 		    &si_stop, NULL) == 0)
 			sysinit_add(si_start, si_stop);
 		linker_file_register_sysctls(lf);
 		lf->flags |= LINKER_FILE_LINKED;
 	}
 	/* woohoo! we made it! */
 }
 
 SYSINIT(preload, SI_SUB_KLD, SI_ORDER_MIDDLE, linker_preload, 0)
 
 /*
  * Search for a not-loaded module by name.
  * 
  * Modules may be found in the following locations:
Index: ddb/ddb.h
===================================================================
--- ddb/ddb.h	(revision 108659)
+++ ddb/ddb.h	(working copy)
@@ -77,60 +77,63 @@ func_name(addr, have_addr, count, modif)
 	db_expr_t count;					\
 	char *modif;
 
 extern db_expr_t db_maxoff;
 extern int db_indent;
 extern int db_inst_count;
 extern int db_load_count;
 extern int db_store_count;
 extern db_expr_t db_radix;
 extern db_expr_t db_max_width;
 extern db_expr_t db_tab_stop_width;
 extern db_expr_t db_lines_per_page;
 
 struct thread;
 struct vm_map;
+struct command;
 
 void		db_check_interrupt(void);
 void		db_clear_watchpoints(void);
 db_addr_t	db_disasm(db_addr_t loc, boolean_t altfmt);
 				/* instruction disassembler */
 void		db_error(const char *s);
 int		db_expression(db_expr_t *valuep);
 int		db_get_variable(db_expr_t *valuep);
 void		db_iprintf(const char *,...) __kprintflike(1, 2);
 struct vm_map	*db_map_addr(vm_offset_t);
 boolean_t	db_map_current(struct vm_map *);
 boolean_t	db_map_equal(struct vm_map *, struct vm_map *);
 void		db_print_loc_and_inst(db_addr_t loc);
 void		db_print_thread(void);
 void		db_printf(const char *fmt, ...) __kprintflike(1, 2);
 int		db_read_bytes(vm_offset_t addr, size_t size, char *data);
 				/* machine-dependent */
 int		db_readline(char *lstart, int lsize);
 void		db_restart_at_pc(boolean_t watchpt);
 int		db_set_variable(db_expr_t value);
 void		db_set_watchpoints(void);
 void		db_setup_paging(db_page_calloutfcn_t *callout, void *arg,
 				int maxlines);
 void		db_skip_to_eol(void);
 boolean_t	db_stop_at_pc(boolean_t *is_breakpoint);
 #define		db_strcpy	strcpy
 void		db_trace_self(void);
 int		db_trace_thread(struct thread *, int);
 int		db_value_of_name(const char *name, db_expr_t *valuep);
 int		db_write_bytes(vm_offset_t addr, size_t size, char *data);
+int		db_cmd_add(struct command **, struct command **, boolean_t);
+void		db_cmd_rem(struct command **, struct command **, boolean_t);
 
 db_cmdfcn_t	db_breakpoint_cmd;
 db_cmdfcn_t	db_continue_cmd;
 db_cmdfcn_t	db_delete_cmd;
 db_cmdfcn_t	db_deletehwatch_cmd;
 db_cmdfcn_t	db_deletewatch_cmd;
 db_cmdfcn_t	db_examine_cmd;
 db_cmdfcn_t	db_hwatchpoint_cmd;
 db_cmdfcn_t	db_listbreak_cmd;
 db_cmdfcn_t	db_print_cmd;
 db_cmdfcn_t	db_ps;
 db_cmdfcn_t	db_search_cmd;
 db_cmdfcn_t	db_set_cmd;
 db_cmdfcn_t	db_set_thread;
 db_cmdfcn_t	db_show_regs;
Index: ddb/db_command.c
===================================================================
--- ddb/db_command.c	(revision 108659)
+++ ddb/db_command.c	(working copy)
@@ -33,51 +33,60 @@
 
 #include <sys/cdefs.h>
 __FBSDID("$FreeBSD: src/sys/ddb/db_command.c,v 1.60.2.2 2005/10/25 20:10:56 jhb Exp $");
 
 #include <sys/param.h>
 #include <sys/linker_set.h>
 #include <sys/lock.h>
 #include <sys/kdb.h>
 #include <sys/mutex.h>
 #include <sys/proc.h>
 #include <sys/reboot.h>
 #include <sys/signalvar.h>
 #include <sys/systm.h>
 #include <sys/cons.h>
 #include <sys/watchdog.h>
+#include <sys/malloc.h>
 
 #include <ddb/ddb.h>
 #include <ddb/db_command.h>
 #include <ddb/db_lex.h>
 #include <ddb/db_output.h>
 
 #include <machine/cpu.h>
 #include <machine/setjmp.h>
 
 /*
  * Exported global variables
  */
 boolean_t	db_cmd_loop_done;
 db_addr_t	db_dot;
 db_addr_t	db_last_addr;
 db_addr_t	db_prev;
 db_addr_t	db_next;
 
 SET_DECLARE(db_cmd_set, struct command);
 SET_DECLARE(db_show_cmd_set, struct command);
 
+
+struct command **db_cmd_start = SET_BEGIN(db_cmd_set);
+struct command **db_cmd_end = SET_LIMIT(db_cmd_set);
+struct command **db_show_cmd_start = SET_BEGIN(db_show_cmd_set);
+struct command **db_show_cmd_end = SET_LIMIT(db_show_cmd_set);
+int db_cmd_is_mallocd = 0;
+int db_show_cmd_is_mallocd = 0;
+
 static db_cmdfcn_t	db_fncall;
 static db_cmdfcn_t	db_gdb;
 static db_cmdfcn_t	db_kill;
 static db_cmdfcn_t	db_reset;
 static db_cmdfcn_t	db_stack_trace;
 static db_cmdfcn_t	db_stack_trace_all;
 static db_cmdfcn_t	db_watchdog;
 
 /* XXX this is actually forward-static. */
 extern struct command	db_show_cmds[];
 
 /*
  * if 'ed' style: 'dot' is set at start of last item printed,
  * and '+' points to next line.
  * Otherwise: 'dot' points to next item, '..' points to last.
@@ -154,30 +163,33 @@ db_cmd_search(name, table, aux_tablep, a
 		       this lets us match single letters */
 		}
 		else {
 		    *cmdp = cmd;
 		    result = CMD_FOUND;
 		}
 	    }
 	}
 	if (aux_tablep != 0)
 	    /* XXX repeat too much code. */
 	    for (aux_cmdp = aux_tablep; aux_cmdp < aux_tablep_end; aux_cmdp++) {
 		register char *lp;
 		register char *rp;
 		register int  c;
 
+		if (*aux_cmdp == NULL)
+		    continue;
+
 		lp = name;
 		rp = (*aux_cmdp)->name;
 		while ((c = *lp) == *rp) {
 		    if (c == 0) {
 			/* complete match */
 			*cmdp = *aux_cmdp;
 			return (CMD_UNIQUE);
 		    }
 		    lp++;
 		    rp++;
 		}
 		if (c == 0) {
 		    /* end of name, not end of command -
 		       partial match */
 		    if (result == CMD_FOUND) {
@@ -204,30 +216,32 @@ static void
 db_cmd_list(table, aux_tablep, aux_tablep_end)
 	struct command *table;
 	struct command **aux_tablep;
 	struct command **aux_tablep_end;
 {
 	register struct command *cmd;
 	register struct command **aux_cmdp;
 
 	for (cmd = table; cmd->name != 0; cmd++) {
 	    db_printf("%-12s", cmd->name);
 	    db_end_line();
 	}
 	if (aux_tablep == 0)
 	    return;
 	for (aux_cmdp = aux_tablep; aux_cmdp < aux_tablep_end; aux_cmdp++) {
+	    if (*aux_cmdp == NULL)
+		continue;
 	    db_printf("%-12s", (*aux_cmdp)->name);
 	    db_end_line();
 	}
 }
 
 static void
 db_command(last_cmdp, cmd_table, aux_cmd_tablep, aux_cmd_tablep_end)
 	struct command	**last_cmdp;	/* IN_OUT */
 	struct command	*cmd_table;
 	struct command	**aux_cmd_tablep;
 	struct command	**aux_cmd_tablep_end;
 {
 	struct command	*cmd;
 	int		t;
 	char		modif[TOK_STRING_SIZE];
@@ -271,32 +285,32 @@ db_command(last_cmdp, cmd_table, aux_cmd
 		    case CMD_AMBIGUOUS:
 			db_printf("Ambiguous\n");
 			db_flush_lex();
 			return;
 		    case CMD_HELP:
 			db_cmd_list(cmd_table, aux_cmd_tablep, aux_cmd_tablep_end);
 			db_flush_lex();
 			return;
 		    default:
 			break;
 		}
 		if ((cmd_table = cmd->more) != 0) {
 		    /* XXX usually no more aux's. */
 		    aux_cmd_tablep = 0;
 		    if (cmd_table == db_show_cmds) {
-			aux_cmd_tablep = SET_BEGIN(db_show_cmd_set);
-			aux_cmd_tablep_end = SET_LIMIT(db_show_cmd_set);
+			aux_cmd_tablep = db_show_cmd_start;
+			aux_cmd_tablep_end = db_show_cmd_end;
 		    }
 
 		    t = db_read_token();
 		    if (t != tIDENT) {
 			db_cmd_list(cmd_table, aux_cmd_tablep, aux_cmd_tablep_end);
 			db_flush_lex();
 			return;
 		    }
 		}
 	    }
 
 	    if ((cmd->flag & CS_OWN) == 0) {
 		/*
 		 * Standard syntax:
 		 * command [/modifier] [addr] [,count]
@@ -360,30 +374,138 @@ db_command(last_cmdp, cmd_table, aux_cmd
 		}
 		else {
 		    db_dot = db_next;
 		}
 	    }
 	    else {
 		/*
 		 * If command does not change dot,
 		 * set 'next' location to be the same.
 		 */
 		db_next = db_dot;
 	    }
 	}
 }
 
+
+int
+db_cmd_add(struct command **set, struct command **set_end, boolean_t is_show)
+{
+	struct command **newset, **newset_end;
+	struct command ***oldset, ***oldset_end;
+	struct command **cpp, **xpp;
+	int *is_mallocd;
+	size_t old_count, new_count, holes;
+
+	if (is_show) {
+		oldset = &db_show_cmd_start;
+		oldset_end = &db_show_cmd_end;
+		is_mallocd = &db_show_cmd_is_mallocd;
+	} else {
+		oldset = &db_cmd_start;
+		oldset_end = &db_cmd_end;
+		is_mallocd = &db_cmd_is_mallocd;
+	}
+
+	new_count = set_end - set;
+	old_count = *oldset_end - *oldset;
+	holes = 0;
+
+	for (cpp = *oldset; cpp < *oldset_end; cpp++) {
+		if (*cpp == NULL)
+			holes++;
+	}
+
+	if (new_count > holes) {
+		size_t count;
+
+		count = old_count - holes + new_count;
+		newset = malloc(count * sizeof(*cpp), M_TEMP, M_NOWAIT);
+		if (newset == NULL)
+			return ENOMEM;
+		newset_end = newset + count;
+	} else {
+		newset = *oldset;
+		newset_end = *oldset_end;
+	}
+
+	xpp = newset;
+	for (cpp = *oldset; cpp < *oldset_end; cpp++) {
+		if (*cpp != NULL)
+			*xpp++ = *cpp;
+	}
+	for (cpp = set; cpp < set_end; cpp++)
+		*xpp++ = *cpp;
+	while (xpp < newset_end)
+	       *xpp++ = NULL;
+
+	cpp = *oldset;
+
+	/*
+	 * Using the debugger over these two assignments would be a
+	 * bad idea.
+	 */
+	*oldset = newset;
+	*oldset_end = newset_end;
+
+	if (new_count > holes) {
+		if (*is_mallocd)
+			free(cpp, M_TEMP);
+		*is_mallocd = 1;
+	}
+
+	return 0;
+}
+
+void
+db_cmd_rem(struct command **set, struct command **set_end, boolean_t is_show)
+{
+	struct command **oldset, **oldset_end;
+	struct command **cpp, **xpp;
+	size_t new_count, holes;
+
+	new_count = set_end - set;
+	if (is_show) {
+		oldset = db_show_cmd_start;
+		oldset_end = db_show_cmd_end;
+	} else {
+		oldset = db_cmd_start;
+		oldset_end = db_cmd_end;
+	}
+
+	xpp = oldset;
+	holes = 0;
+	for (cpp = set; cpp < set_end; cpp++) {
+		/*
+		 * Find *cpp.  Items were copied into the global set
+		 * in order, so they should be found in order.
+		 */
+		for (; xpp < oldset_end; xpp++) {
+			if (*xpp == *cpp) {
+				*xpp = NULL;
+				holes++;
+				break;
+			}
+		}
+	}
+
+	if (holes != new_count)
+		printf("%s found %zd %scmd's; there should be %zd.\n",
+		    __func__, holes, is_show ? "show_" : "", new_count);
+}
+
+
 /*
  * 'show' commands
  */
 
 static struct command db_show_all_cmds[] = {
 	{ "procs",	db_ps,			0,	0 },
 	{ (char *)0 }
 };
 
 static struct command db_show_cmds[] = {
 	{ "all",	0,			0,	db_show_all_cmds },
 	{ "registers",	db_show_regs,		0,	0 },
 	{ "breaks",	db_listbreak_cmd, 	0,	0 },
 	{ "threads",	db_show_threads,	0,	0 },
 	{ (char *)0, }
@@ -444,31 +566,31 @@ db_command_loop()
 	/*
 	 * Initialize 'prev' and 'next' to dot.
 	 */
 	db_prev = db_dot;
 	db_next = db_dot;
 
 	db_cmd_loop_done = 0;
 	while (!db_cmd_loop_done) {
 	    if (db_print_position() != 0)
 		db_printf("\n");
 
 	    db_printf("db> ");
 	    (void) db_read_line();
 
 	    db_command(&db_last_command, db_command_table,
-		       SET_BEGIN(db_cmd_set), SET_LIMIT(db_cmd_set));
+		       db_cmd_start, db_cmd_end);
 	}
 }
 
 void
 db_error(s)
 	const char *s;
 {
 	if (s)
 	    db_printf("%s", s);
 	db_flush_lex();
 	kdb_reenter();
 }
 
 
 /*
_______________________________________________
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"

Reply via email to