Greetings Martin
I've glanced over the patch and have attached a revised version that I
feel is a tad more consistent.
* Invalid/missing options error messages are now routed through the
(new) bad_value and missing_value functions. I prefer these to calling
terminate(), as they also display the usage instructions when the option
is incorrectly used.
* the -d and -x switches now check for a missing value for consistency
with the -t switch.
* use 'sizeof option - 1' rather than a magic constant for parameter 3
when calling get_long_val
* Removed superfluous assignment to optname for the compat and nocompat
switches (This one's debatable).
* Moved declaration of act struct in one conditional
--Andrew Black
Martin Sebor wrote:
I've discovered a number of problems with the eval_options() function
(see below). The attached patch implements fixes for them all. Andrew,
let me know if you see any problems with it. If not, I'll commit it
later today.
Btw., as an FYI, C99 strtol() is declared to take a couple of restrict
pointers:
long int strtol(const char * restrict nptr,
char ** restrict endptr,
int base);
which means that the behavior of the function is undefined when
(nptr == &nptr) is true. I fixed this as well.
Martin
1. Undefined behavior for "-t"
$ ./exec -t
Segmentation Fault (core dumped)
2. Negative argument to -t not diagnosed:
$ ./exec -t -1; echo $?
0
3. Poor diagnostic of invalid argument to -t:
$ ./exec -t 1x
./exec: Unknown value for -t: x
4. Abort on missing option argument:
$ ./exec --exit
Assertion failed: 0 != opt, file /build/sebor/stdcxx/util/cmdopt.cpp,
line 141
Abort (core dumped)
5. Missing text for invalid argument:
$ ./exec --exit=9999999999
./exec: Unknown value for --exit:
6. Invalid argument to --signal and --ignorte not diagnosed:
$ ./exec --signal 12345; echo $?
0
$ ./exec --ignore 12345; echo $?
0
With the patch applied:
$ ./exec -t
./exec: Missing argument for -t
$ ./exec -t -1
./exec: Bad argument for -t: -1
$ ./exec -t 1x
./exec: Bad argument for -t: 1x
$ ./exec --exit
./exec: Missing argument for --exit
$ ./exec --exit=9999999999
./exec: Bad argument for --exit: 9999999999
$ ./exec --signal 12345
./exec: kill(20842, SIG#12345) failed: Invalid argument
$ ./exec --ignore 12345 ./exec: sigaction(SIG#12345, ...) failed:
Invalid argument
Index: cmdopt.cpp
===================================================================
--- cmdopt.cpp (revision 428177)
+++ cmdopt.cpp (working copy)
@@ -129,6 +129,37 @@
}
/**
+ Helper function to produce 'Bad argument' error message.
+
+ Terminates via show_usage.
+
+ @param opt name of option encountered
+ @param val invalid value found
+*/
+static void
+bad_value (char* const opt, char* const val)
+{
+ assert (0 != opt);
+ warn ("Bad argument for %s: %s\n", opt, val);
+ show_usage (1);
+}
+
+/**
+ Helper function to produce 'Missing argument' error message.
+
+ Terminates via show_usage.
+
+ @param opt name of option missing argument
+*/
+static void
+missing_value (char* const opt)
+{
+ assert (0 != opt);
+ warn ("Missing argument for %s\n", opt);
+ show_usage (1);
+}
+
+/**
Helper function to produce 'Unknown option' error message.
Terminates via show_usage.
@@ -157,10 +188,18 @@
int
eval_options (const int argc, char* const argv [])
{
+ const char opt_timeout[] = "-t";
+ const char opt_data_dir[] = "-d";
+ const char opt_t_flags[] = "-x";
+ const char opt_compat[] = "--compat";
+ const char opt_exit[] = "--exit";
+ const char opt_ignore[] = "--ignore";
+ const char opt_nocompat[] = "--nocompat";
+ const char opt_signal[] = "--signal";
+ const char opt_sleep[] = "--sleep";
+
int i;
- char* val;
-
assert (0 != argv);
if (1 == argc || '-' != argv [1][0])
@@ -168,6 +207,14 @@
for (i = 1; i < argc && '-' == argv [i][0]; ++i) {
+ /* the name of the option being processed */
+ const char* optname = argv [i];
+
+ /* the option's argument, if any */
+ const char* optarg = 0;
+
+ char* end = 0;
+
switch (argv [i][1]) {
case '?':
case 'h':
@@ -176,16 +223,29 @@
++i; /* Ignore -r option (makefile compat) */
break;
case 't':
- val = get_short_val (argv, &i);
- timeout = strtol (val, &val, 10);
- if (*val || errno)
- terminate (1, "Unknown value for -t: %s\n", val);
+ optname = opt_timeout;
+ optarg = get_short_val (argv, &i);
+ if (optarg) {
+ timeout = strtol (optarg, &end, 10);
+ if (*end || timeout < 0 || errno)
+ bad_value (optname, optarg);
+ }
+ else
+ missing_value (optname);
+
break;
+
case 'd':
+ optname = opt_data_dir;
in_root = get_short_val (argv, &i);
+ if (!in_root)
+ missing_value (optname);
break;
case 'x':
+ optname = opt_t_flags;
exe_opts = get_short_val (argv, &i);
+ if (!exe_opts)
+ missing_value (optname);
break;
case 'v':
++verbose;
@@ -193,6 +253,7 @@
case 'q':
verbose = 0;
break;
+
case '-':
{
const size_t arglen = strlen (argv [i]);
@@ -201,80 +262,81 @@
if ('\0' == argv [i][2])
return i+1;
- if (8 == arglen && 0 == memcmp ("--compat\0", argv [i], 9)) {
- compat = 1;
+ if ( sizeof opt_compat - 1 == arglen
+ && !memcmp (opt_compat, argv [i], sizeof opt_compat)) {
+ compat = 1;
break;
}
- else if (10 == arglen && 0 == memcmp ("--nocompat\0", argv [i],
- 11)) {
- compat = 0;
+ else if ( sizeof opt_nocompat - 1 == arglen
+ && !memcmp (opt_nocompat, argv [i], sizeof opt_nocompat)) {
+ compat = 0;
break;
}
- else if (6 <= arglen && 0 == memcmp ("--exit", argv [i], 6)) {
- val = get_long_val (argv, &i, 6);
- if (val) {
- int code = strtol (val, &val, 10);
- if (*val || errno)
- terminate (1, "Unknown value for --exit: %s\n", val);
- exit (code);
+ else if ( sizeof opt_exit - 1 <= arglen
+ && !memcmp (opt_exit, argv [i], sizeof opt_exit - 1)) {
+ optname = opt_exit;
+ optarg = get_long_val (argv, &i, sizeof opt_exit - 1);
+ if (optarg && *optarg) {
+ const long code = strtol (optarg, &end, 10);
+ if ('\0' == *end && !errno)
+ exit (code);
}
- else
- bad_option (argv [i]);
}
- else if (7 <= arglen && 0 == memcmp ("--sleep", argv [i], 7)) {
- val = get_long_val (argv, &i, 7);
- if (val) {
- int duration = strtol (val, &val, 10);
- if (*val || errno)
- terminate (1, "Unknown value for --sleep: %s\n", val);
- sleep (duration);
- break;
+ else if ( sizeof opt_sleep - 1 <= arglen
+ && !memcmp (opt_sleep, argv [i], sizeof opt_sleep - 1)) {
+ optname = opt_sleep;
+ optarg = get_long_val (argv, &i, sizeof opt_sleep - 1);
+ if (optarg && *optarg) {
+ const long nsec = strtol (optarg, &end, 10);
+ if ('\0' == *end && 0 <= nsec && !errno) {
+ sleep (nsec);
+ break;
+ }
}
- else
- bad_option (argv [i]);
}
- else if (8 <= arglen && 0 == memcmp ("--signal", argv [i], 8)) {
- val = get_long_val (argv, &i, 8);
- if (val) {
- int sig = get_signo (val);
- if (0 > sig)
- terminate (1, "Unknown signal name for --signal: "
- "%s\n", val);
-
- /* Ignore kill errors (what should we do with them?) */
- (void)kill (getpid (), sig);
-
- /* Not certain what we should do if we don't terminate by
- signal */
- break;
+ else if ( sizeof opt_signal - 1 <= arglen
+ && !memcmp (opt_signal, argv [i], sizeof opt_signal - 1)) {
+ optname = opt_signal;
+ optarg = get_long_val (argv, &i, sizeof opt_signal - 1);
+ if (optarg && *optarg) {
+ const long signo = get_signo (optarg);
+ if (0 <= signo) {
+ if (0 > kill (getpid (), signo))
+ terminate (1, "kill(%d, %s) failed: %s\n",
+ getpid (), get_signame (signo),
+ strerror (errno));
+ break;
+ }
}
- else
- bad_option (argv [i]);
}
- else if (8 <= arglen && 0 == memcmp ("--ignore", argv [i], 8)) {
- val = get_long_val (argv, &i, 8);
- if (val) {
- struct sigaction act;
- int sig = get_signo (val);
- if (0 > sig)
- terminate (1, "Unknown signal name for --ignore: "
- "%s\n", val);
-
- memset (&act, 0, sizeof act);
- act.sa_handler = SIG_IGN;
- (void)sigaction (sig, &act, 0);
- /* Ignore sigaction errors (what should we do with them?)
- */
- break;
+ else if ( sizeof opt_ignore <= arglen
+ && !memcmp (opt_ignore, argv [i], sizeof opt_ignore - 1)) {
+ optname = opt_ignore;
+ optarg = get_long_val (argv, &i, sizeof opt_ignore);
+ if (optarg && *optarg) {
+ const long signo = get_signo (optarg);
+ if (0 <= signo) {
+ struct sigaction act;
+ memset (&act, 0, sizeof act);
+ act.sa_handler = SIG_IGN;
+ if (0 > sigaction (signo, &act, 0))
+ terminate (1, "sigaction(%s, ...) failed: %s\n",
+ get_signame (signo), strerror (errno));
+ break;
+ }
}
- else
- bad_option (argv [i]);
}
- else
- bad_option (argv [i]);
+
+ /* fall through */
}
default:
- bad_option (argv [i]);
+ if (optarg)
+ bad_value (optname, optarg);
+
+ if (argv [i])
+ bad_option (argv [i]);
+ else
+ missing_value (optname);
}
}
