| From: D. Hugh Redelmeier <hugh@xxxxxxxxxx>
| 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 ================
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility
