[Crash-utility] implementation of -h and --help [was: documentation of crash flags]linux-2.6.22.1-20.fc7]

| From: D. Hugh Redelmeier <hugh(a)mimosa.com&gt; | It would be good if more of the flags were documented. | | Also: getopt makes it easy to add a long option name as a synonym for | a one-character option. I think that you can use this feature to help | make options clearer. For example, --force could be added as a | synonym for -f at the cost of one more line initializing long_options. While I was looking at this, I found some interesting issues with -h and --help. There is separate and different code to implement --help and -h and the undocumented -H flag. In fact, -h is implemented twice: once in main.c:main() case 'h' and once in the default case! --help uses program_usage(), -h case 'h' and -H use cmd_usage (slightly differently), and -h default uses program_usage. One difference is that the -H and one implementation of -h must have an argument, the other -h must not, and neither must the --help. Very confusing. -h and --help should optionally take an argument and -H should not exist. I would also claim that -h or --help should exit with status 0 but currently they exit with status 1. I am enclosing a patch that fixes these anomalies and does a small amount of tidying. The resulting code is simpler and easier to understand, at least in my eyes. In help.c: - Disentangle the code for program_usage(LONG_FORM) and program_usage(SHORT_FORM). The code for these two cases was pretty different but intermingled. This change makes them simpler. It makes sense that LONG_FORM exits with status 0 and that SHORT_FORM exits with status 1 since the first is requested by the user while the second is a response to a user error. Note that LONG_FORM unconditionally uses less(1). This is questionable. I didn't change this. These two cases should probably be separate routines. After my changes to main.c only one call with LONG_FORM remains. In help.c cmd_usage(): - make sure that cmd_usage does not call gdb if gdb isn't initialized (in trying to give -h help for an unrecognized command) - add a flag MUST_HELP to cmd_usage to specify that if help isn't found a diagnostic should be printed. This used to be the case if the command calling cmd_usage was help but not if -h called it. - this command still unconditionally uses less(1) for anything but a synopsis. This should be changed. In main.c, the long_options array: - use the name "required_argument" in place of the magic number 1. - make --help a synonym of 'h' - specify that --help has an optional_argument. In main.c main(): - in the call of getopt_long: add another ':' for -h. This means that the argument is optional. This, in turn, means that the switch default no longer handles -h at the end of an argument list. - I added code to detect and report an unhandled long option. It is easy to get the long_option table out of sync with the handlers. Mostly this involves adding "else" a lot of places in case 0. - ditch the code to implement --help since it is now handled by the code for -h - ditch the code to implement -h in the switch default since it is now handled in case 'h' - the case 'h' code now has to handle the fact that the -h / --help argument is optional. - removed -H flag which wasn't documented and is now redundant. =================================================================== RCS file: RCS/defs.h,v retrieving revision 1.1 diff -u -r1.1 defs.h --- defs.h 2007/07/07 17:56:15 1.1 +++ defs.h 2007/07/16 21:34:50 @@ -2630,6 +2630,7 @@ #define SYNOPSIS (0x1) #define COMPLETE_HELP (0x2) #define PIPE_TO_LESS (0x4) +#define MUST_HELP (0x8) #define LEFT_JUSTIFY (1) #define RIGHT_JUSTIFY (2) =================================================================== RCS file: RCS/help.c,v retrieving revision 1.1 diff -u -r1.1 help.c --- help.c 2007/07/16 17:54:15 1.1 +++ help.c 2007/07/16 21:57:40 @@ -105,34 +105,27 @@ void program_usage(int form) { - int i; - char **p; - FILE *less; - - if (form == LONG_FORM) - less = popen("/usr/bin/less", "w"); - else - less = NULL; - - p = program_usage_info; + if (form == SHORT_FORM) { + fprintf(fp, program_usage_info[0], pc->program_name); + fprintf(fp, "\nEnter \"%s -h\" for details.\n", + pc->program_name); + clean_exit(1); + } else { + char **p = program_usage_info; + FILE *less = popen("/usr/bin/less", "w"); - if (form == LONG_FORM) { if (less) fp = less; - for (i = 0; program_usage_info[i]; i++, p++) { - fprintf(fp, *p, pc->program_name); + for (; *p; p++) { + fprintf(fp, *p, pc->program_name); fprintf(fp, "\n"); } - } else { - fprintf(fp, *p, pc->program_name); - fprintf(fp, "\nEnter \"%s -h\" for details.\n", - pc->program_name); - } - fflush(fp); - if (less) - pclose(less); + fflush(fp); + if (less) + pclose(less); - clean_exit(1); + clean_exit(0); + } } @@ -397,7 +390,7 @@ if (oflag) dump_offset_table(args[optind], FALSE); else - cmd_usage(args[optind], COMPLETE_HELP); + cmd_usage(args[optind], COMPLETE_HELP|MUST_HELP); optind++; } while (args[optind]); } @@ -4902,12 +4895,9 @@ void cmd_usage(char *cmd, int helpflag) { - int i; - int found; char **p; struct command_table_entry *cp; char buf[BUFSIZE]; - struct alias_data *ad; FILE *less; if (helpflag & PIPE_TO_LESS) { @@ -4939,6 +4929,8 @@ } if (STREQ(cmd, "all")) { + int i; + display_input_info(); display_output_info(); help_init(); @@ -4959,46 +4951,50 @@ goto done_usage; } - found = FALSE; -retry: - if ((cp = get_command_table_entry(cmd))) { - if ((p = cp->help_data)) - found = TRUE; - } + /* look up command, possibly through an alias */ + for (;;) { + struct alias_data *ad; + + cp = get_command_table_entry(cmd); + if (cp != NULL) + break; /* found command */ + + /* try for an alias */ + ad = is_alias(cmd); + if (ad == NULL) + break; /* neither command nor alias */ - /* - * Check for alias names or gdb commands. - */ - if (!found) { - if ((ad = is_alias(cmd))) { - cmd = ad->args[0]; - goto retry; - } + cmd = ad->args[0]; + cp = get_command_table_entry(cmd); + } - if (helpflag == SYNOPSIS) { - fprintf(fp, - "No usage data for the \"%s\" command is available.\n", + if (cp == NULL || (p = cp->help_data) == NULL) { + if (helpflag & SYNOPSIS) { + fprintf(fp, + "No usage data for the \"%s\" command" + " is available.\n", cmd); RESTART(); } - if (STREQ(pc->curcmd, "help")) { - if (cp) - fprintf(fp, - "No help data for the \"%s\" command is available.\n", + if (helpflag & MUST_HELP) { + if (cp || !(pc->flags & GDB_INIT)) + fprintf(fp, + "No help data for the \"%s\" command" + " is available.\n", cmd); else if (!gdb_pass_through(concat_args(buf, 0, FALSE), NULL, GNU_RETURN_ON_ERROR)) fprintf(fp, - "No help data for \"%s\" is available.\n", - cmd); + "No help data for \"%s\" is available.\n", + cmd); } goto done_usage; } p++; - if (helpflag == SYNOPSIS) { + if (helpflag & SYNOPSIS) { p++; fprintf(fp, "Usage: %s ", cmd); fprintf(fp, *p, pc->program_name, pc->program_name); =================================================================== RCS file: RCS/main.c,v retrieving revision 1.1 diff -u -r1.1 main.c --- main.c 2007/07/16 17:52:28 1.1 +++ main.c 2007/07/16 21:41:01 @@ -27,12 +27,12 @@ static void check_xen_hyper(void); static struct option long_options[] = { - {"memory_module", 1, 0, 0}, - {"memory_device", 1, 0, 0}, + {"memory_module", required_argument, 0, 0}, + {"memory_device", required_argument, 0, 0}, {"no_kallsyms", 0, 0, 0}, {"no_modules", 0, 0, 0}, {"no_namelist_gzip", 0, 0, 0}, - {"help", 0, 0, 0}, + {"help", optional_argument, 0, 'h'}, {"data_debug", 0, 0, 0}, {"no_data_debug", 0, 0, 0}, {"no_crashrc", 0, 0, 0}, @@ -40,14 +40,14 @@ {"kmem_cache_delay", 0, 0, 0}, {"readnow", 0, 0, 0}, {"smp", 0, 0, 0}, - {"machdep", 1, 0, 0}, + {"machdep", required_argument, 0, 0}, {"version", 0, 0, 0}, {"buildinfo", 0, 0, 0}, {"shadow_page_tables", 0, 0, 0}, - {"cpus", 1, 0, 0}, + {"cpus", required_argument, 0, 0}, {"no_ikconfig", 0, 0, 0}, {"hyper", 0, 0, 0}, - {"p2m_mfn", 1, 0, 0}, + {"p2m_mfn", required_argument, 0, 0}, {"zero_excluded", 0, 0, 0}, {"no_panic", 0, 0, 0}, {0, 0, 0, 0} @@ -65,7 +65,7 @@ */ opterr = 0; optind = 0; - while((c = getopt_long(argc, argv, "LgH:h:e:i:sSvc:d:tfp:m:", + while((c = getopt_long(argc, argv, "Lgh::e:i:sSvc:d:tfp:m:", long_options, &option_index)) != -1) { switch (c) { @@ -74,60 +74,55 @@ "memory_module")) pc->memory_module = optarg; - if (STREQ(long_options[option_index].name, + else if (STREQ(long_options[option_index].name, "memory_device")) pc->memory_device = optarg; - if (STREQ(long_options[option_index].name, + else if (STREQ(long_options[option_index].name, "no_kallsyms")) kt->flags |= NO_KALLSYMS; - if (STREQ(long_options[option_index].name, + else if (STREQ(long_options[option_index].name, "no_modules")) kt->flags |= NO_MODULE_ACCESS; - if (STREQ(long_options[option_index].name, + else if (STREQ(long_options[option_index].name, "no_ikconfig")) kt->flags |= NO_IKCONFIG; - if (STREQ(long_options[option_index].name, + else if (STREQ(long_options[option_index].name, "no_namelist_gzip")) pc->flags |= NAMELIST_NO_GZIP; - if (STREQ(long_options[option_index].name, "help")) { - program_usage(LONG_FORM); - clean_exit(0); - } - - if (STREQ(long_options[option_index].name, + else if (STREQ(long_options[option_index].name, "data_debug")) pc->flags |= DATADEBUG; - if (STREQ(long_options[option_index].name, + else if (STREQ(long_options[option_index].name, "no_data_debug")) pc->flags &= ~DATADEBUG; - if (STREQ(long_options[option_index].name, + else if (STREQ(long_options[option_index].name, "no_kmem_cache")) vt->flags |= KMEM_CACHE_UNAVAIL; - if (STREQ(long_options[option_index].name, + else if (STREQ(long_options[option_index].name, "kmem_cache_delay")) vt->flags |= KMEM_CACHE_DELAY; - if (STREQ(long_options[option_index].name, + else if (STREQ(long_options[option_index].name, "readnow")) pc->flags |= READNOW; - if (STREQ(long_options[option_index].name, + else if (STREQ(long_options[option_index].name, "smp")) kt->flags |= SMP; - if (STREQ(long_options[option_index].name, + else if (STREQ(long_options[option_index].name, "machdep")) machdep->cmdline_arg = optarg; - if (STREQ(long_options[option_index].name, + else if (STREQ(long_options[option_index].name, "version")) { pc->flags |= VERSION_QUERY; display_version(); @@ -135,30 +130,36 @@ clean_exit(0); } - if (STREQ(long_options[option_index].name, + else if (STREQ(long_options[option_index].name, "buildinfo")) { dump_build_data(); clean_exit(0); } - if (STREQ(long_options[option_index].name, + else if (STREQ(long_options[option_index].name, "shadow_page_tables")) kt->xen_flags |= SHADOW_PAGE_TABLES; - if (STREQ(long_options[option_index].name, "cpus")) + else if (STREQ(long_options[option_index].name, "cpus")) kt->cpus_override = optarg; - if (STREQ(long_options[option_index].name, "hyper")) + else if (STREQ(long_options[option_index].name, "hyper")) pc->flags |= XEN_HYPER; - if (STREQ(long_options[option_index].name, "p2m_mfn")) + else if (STREQ(long_options[option_index].name, "p2m_mfn")) xen_kdump_p2m_mfn(optarg); - if (STREQ(long_options[option_index].name, "zero_excluded")) + else if (STREQ(long_options[option_index].name, "zero_excluded")) *diskdump_flags |= ZERO_EXCLUDED; - if (STREQ(long_options[option_index].name, "no_panic")) + else if (STREQ(long_options[option_index].name, "no_panic")) tt->flags |= PANIC_TASK_NOT_FOUND; + + else { + error(INFO, "internal error: option %s unhandled\n", + long_options[option_index].name); + program_usage(SHORT_FORM); + } break; case 'f': @@ -169,12 +170,19 @@ pc->flags |= KERNEL_DEBUG_QUERY; break; - case 'H': - cmd_usage(optarg, COMPLETE_HELP); - clean_exit(0); - case 'h': - cmd_usage(optarg, COMPLETE_HELP|PIPE_TO_LESS); + /* note: long_getopt's handling of optional arguments is weak. + * To it, an optional argument must be part of the same argument + * as the flag itself (eg. --help=commands or -hcommands). + * We want to accept "--help commands" or "-h commands". + * So we must do that part ourselves. + */ + if (optarg != NULL) + cmd_usage(optarg, COMPLETE_HELP|PIPE_TO_LESS|MUST_HELP); + else if (argv[optind] != NULL && argv[optind][0] != '-') + cmd_usage(argv[optind++], COMPLETE_HELP|PIPE_TO_LESS|MUST_HELP); + else + program_usage(LONG_FORM); clean_exit(0); case 'e': @@ -238,13 +246,9 @@ break; default: - if (STREQ(argv[optind-1], "-h")) - program_usage(LONG_FORM); - else { - error(INFO, "invalid option: %s\n", - argv[optind-1]); - program_usage(SHORT_FORM); - } + error(INFO, "invalid option: %s\n", + argv[optind-1]); + program_usage(SHORT_FORM); } } opterr = 1; ================ end ================