Greetings
One more take on this patch, hopefully the final. I think I've
addressed all the concerns presented here.
--Andrew Black
Martin Sebor wrote:
Andrew Black wrote:
Ok...
I've tried to address the issues noted below in the attached (revised)
version of the test_switches.diff patch. In the process of testing
the modifications, I discovered a bug with how get_signo and
get_signame determine the end of the array. That bug has also been
resolved in this revised patch.
Goody! :)
+const int
+get_signo (const char* signame)
+{
size_t i;
+ int trans;
+ char *junk;
+
+ assert (0 != signame);
+
+ if ('s' == tolower (signame [0]) && 'i' == tolower (signame [1])
I'm afraid this is unsafe when char is a signed type. tolower() takes
an int but the behavior of the function is undefined when the argument
is neither EOF or representable in unsigned char.
The safe way to use these functions is to convert the character to
unsigned char first:
typedef unsigned char UChar;
if ( 's' == tolower ((UChar)signame [0])
&& 'i' == tolower ((UChar)signame [1])
+ && 'g' == tolower (signame [2]))
+ signame += 3;
+ + if ('#' == signame [0])
+ ++signame;
+ + trans = strtol (signame, &junk, 0);
Did you really intend to accept things like SIG#0xa and interpret
input such as SIG#011 as octal? I would expect this function (and
probably all the rest) to handle only decimal numbers and reject
anything else.
+
+ if (0 == *junk && 0 == errno)
+ return trans;
+
+ for (i = 0; signal_names [i].str; ++i) {
+ if (0 == strcasecmp (signal_names [i].str, signame)) {
strcasecmp() is non-standard, we can't use it. AFAIK, there's no
standard case-insensitive string comparison function in C or in
POSIX.
Could you please fix these and resubmit?
Thanks
Martin
Index: exec.cpp
===================================================================
--- exec.cpp (revision 428121)
+++ exec.cpp (working copy)
@@ -29,6 +29,7 @@
#undef __PURE_CNAME
#include <assert.h> /* for assert */
+#include <ctype.h> /* for tolower */
#include <errno.h> /* for errno */
#include <fcntl.h> /* for O_*, */
#include <signal.h>
@@ -58,192 +59,292 @@
static int alarm_timeout;
/**
- Translates a signal number into a human understandable name
-
- @param signo a signal number
- @returns the human understandable name for the signal (minus the SIG
- prefix), or "#n" if the the name for the number is unknown to the
- function, where n is signo
+ Utility macro to generate a signal number/name pair.
+
+ @parm val 'short' signal name (no leading SIG) to generate pair for.
+ @see signal_names []
*/
-const char*
-get_signame (int signo)
-{
- static const struct {
- int val;
- const char* str;
- } names [] = {
-
#undef SIGNAL
#define SIGNAL(val) { SIG ## val, #val }
+/**
+ Signal name/number translation table.
+
+ This table is populated using the SIGNAL helper macro to translate system SIG* macro values into name/value pairs.
+
+ @see SIGNAL ()
+*/
+static const struct {
+ int val; /**< Signal value for lookup pair */
+ const char* str; /**< Signal name for lookup pair */
+} signal_names [] = {
#ifdef SIGABRT
- SIGNAL (ABRT),
+ SIGNAL (ABRT),
#endif /* SIGABRT */
#ifdef SIGALRM
- SIGNAL (ALRM),
+ SIGNAL (ALRM),
#endif /* SIGALRM */
#ifdef SIGBUS
- SIGNAL (BUS),
+ SIGNAL (BUS),
#endif /* SIGBUS */
#ifdef SIGCANCEL
- SIGNAL (CANCEL),
+ SIGNAL (CANCEL),
#endif /* SIGCANCEL */
#ifdef SIGCHLD
- SIGNAL (CHLD),
+ SIGNAL (CHLD),
#endif /* SIGCHLD */
#ifdef SIGCKPT
- SIGNAL (CKPT),
+ SIGNAL (CKPT),
#endif /* SIGCKPT */
#ifdef SIGCLD
- SIGNAL (CLD),
+ SIGNAL (CLD),
#endif /* SIGCLD */
#ifdef SIGCONT
- SIGNAL (CONT),
+ SIGNAL (CONT),
#endif /* SIGCONT */
#ifdef SIGDIL
- SIGNAL (DIL),
+ SIGNAL (DIL),
#endif /* SIGDIL */
#ifdef SIGEMT
- SIGNAL (EMT),
+ SIGNAL (EMT),
#endif /* SIGEMT */
#ifdef SIGFPE
- SIGNAL (FPE),
+ SIGNAL (FPE),
#endif /* SIGFPE */
#ifdef SIGFREEZE
- SIGNAL (FREEZE),
+ SIGNAL (FREEZE),
#endif /* SIGFREEZE */
#ifdef SIGGFAULT
- SIGNAL (GFAULT),
+ SIGNAL (GFAULT),
#endif /* SIGGFAULT */
#ifdef SIGHUP
- SIGNAL (HUP),
+ SIGNAL (HUP),
#endif /* SIGHUP */
#ifdef SIGILL
- SIGNAL (ILL),
+ SIGNAL (ILL),
#endif /* SIGILL */
#ifdef SIGINFO
- SIGNAL (INFO),
+ SIGNAL (INFO),
#endif /* SIGINFO */
#ifdef SIGINT
- SIGNAL (INT),
+ SIGNAL (INT),
#endif /* SIGINT */
#ifdef SIGIO
- SIGNAL (IO),
+ SIGNAL (IO),
#endif /* SIGIO */
#ifdef SIGIOT
- SIGNAL (IOT),
+ SIGNAL (IOT),
#endif /* SIGIOT */
#ifdef SIGK32
- SIGNAL (K32),
+ SIGNAL (K32),
#endif /* SIGK32 */
#ifdef SIGKILL
- SIGNAL (KILL),
+ SIGNAL (KILL),
#endif /* SIGKILL */
#ifdef SIGLOST
- SIGNAL (LOST),
+ SIGNAL (LOST),
#endif /* SIGLOST */
#ifdef SIGLWP
- SIGNAL (LWP),
+ SIGNAL (LWP),
#endif /* SIGLWP */
#ifdef SIGPIPE
- SIGNAL (PIPE),
+ SIGNAL (PIPE),
#endif /* SIGPIPE */
#ifdef SIGPOLL
- SIGNAL (POLL),
+ SIGNAL (POLL),
#endif /* SIGPOLL */
#ifdef SIGPROF
- SIGNAL (PROF),
+ SIGNAL (PROF),
#endif /* SIGPROF */
#ifdef SIGPTINTR
- SIGNAL (PTINTR),
+ SIGNAL (PTINTR),
#endif /* SIGPTINTR */
#ifdef SIGPTRESCHED
- SIGNAL (PTRESCHED),
+ SIGNAL (PTRESCHED),
#endif /* SIGPTRESCHED */
#ifdef SIGPWR
- SIGNAL (PWR),
+ SIGNAL (PWR),
#endif /* SIGPWR */
#ifdef SIGQUIT
- SIGNAL (QUIT),
+ SIGNAL (QUIT),
#endif /* SIGQUIT */
#ifdef SIGRESTART
- SIGNAL (RESTART),
+ SIGNAL (RESTART),
#endif /* SIGRESTART */
#ifdef SIGRESV
- SIGNAL (RESV),
+ SIGNAL (RESV),
#endif /* SIGRESV */
#ifdef SIGSEGV
- SIGNAL (SEGV),
+ SIGNAL (SEGV),
#endif /* SIGSEGV */
#ifdef SIGSTKFLT
- SIGNAL (STKFLT),
+ SIGNAL (STKFLT),
#endif /* SIGSTKFLT */
#ifdef SIGSTOP
- SIGNAL (STOP),
+ SIGNAL (STOP),
#endif /* SIGSTOP */
#ifdef SIGSYS
- SIGNAL (SYS),
+ SIGNAL (SYS),
#endif /* SIGSYS */
#ifdef SIGTERM
- SIGNAL (TERM),
+ SIGNAL (TERM),
#endif /* SIGTERM */
#ifdef SIGTHAW
- SIGNAL (THAW),
+ SIGNAL (THAW),
#endif /* SIGTHAW */
#ifdef SIGTRAP
- SIGNAL (TRAP),
+ SIGNAL (TRAP),
#endif /* SIGTRAP */
#ifdef SIGTSTP
- SIGNAL (TSTP),
+ SIGNAL (TSTP),
#endif /* SIGTSTP */
#ifdef SIGTTIN
- SIGNAL (TTIN),
+ SIGNAL (TTIN),
#endif /* SIGTTIN */
#ifdef SIGTTOU
- SIGNAL (TTOU),
+ SIGNAL (TTOU),
#endif /* SIGTTOU */
#ifdef SIGUNUSED
- SIGNAL (UNUSED),
+ SIGNAL (UNUSED),
#endif /* SIGUNUSED */
#ifdef SIGURG
- SIGNAL (URG),
+ SIGNAL (URG),
#endif /* SIGURG */
#ifdef SIGUSR1
- SIGNAL (USR1),
+ SIGNAL (USR1),
#endif /* SIGUSR1 */
#ifdef SIGUSR2
- SIGNAL (USR2),
+ SIGNAL (USR2),
#endif /* SIGUSR2 */
#ifdef SIGVTALRM
- SIGNAL (VTALRM),
+ SIGNAL (VTALRM),
#endif /* SIGVTALRM */
#ifdef SIGWAITING
- SIGNAL (WAITING),
+ SIGNAL (WAITING),
#endif /* SIGWAITING */
#ifdef SIGWINCH
- SIGNAL (WINCH),
+ SIGNAL (WINCH),
#endif /* SIGWINCH */
#ifdef SIGWINDOW
- SIGNAL (WINDOW),
+ SIGNAL (WINDOW),
#endif /* SIGWINDOW */
#ifdef SIGXCPU
- SIGNAL (XCPU),
+ SIGNAL (XCPU),
#endif /* SIGXCPU */
#ifdef SIGXFSZ
- SIGNAL (XFSZ),
+ SIGNAL (XFSZ),
#endif /* SIGXFSZ */
#ifdef SIGXRES
- SIGNAL (XRES),
+ SIGNAL (XRES),
#endif /* SIGXRES */
- { -1, 0 }
- };
+ { -1, 0 }
+};
+/**
+ Compare two characters in a case-insensitive manner
+
+ @param c1 first character to compare
+ @param c2 second character to compare
+ @return an integer less than, equal to, or greater than 0, coresponding
+ to whether c1 is less than, equal to, or greater than c2 when compared
+ in a case insensitive manner.
+*/
+static int
+rw_charcasecmp (char c1, char c2)
+{
+ typedef unsigned char UChar;
+ return tolower ((UChar)c1) - tolower ((UChar)c2);
+}
+
+/**
+ Reimplementation of the POSIX strcasecmp function.
+
+ This is a simplistic (re)implementation of the strcasecmp function
+ specified in the XSI extension to the IEEE Std 1003.1 (POSIX) standard.
+
+ @param s1 pointer to first string to compare
+ @param s2 pointer to second string to compare
+ @return an integer less than, equal to, or greater than 0, coresponding
+ to whether s1 is less than, equal to, or greater than s2 when compared
+ in a case insensitive manner.
+*/
+static int
+rw_strcasecmp (const char* s1, const char* s2)
+{
+ int delta;
+
+ assert (0 != s1);
+ assert (0 != s2);
+
+ for (delta = rw_charcasecmp (*s1, *s2);
+ *s1 && *s2 && 0 == delta;
+ delta = rw_charcasecmp (*(++s1), *(++s2)));
+ return delta;
+}
+
+/**
+ Translates a human understandable signal name into a number usable by kill ().
+
+ This method understands several formats for signal names. They are as follows:
+ - n
+ - SIGFOO
+ - FOO
+ In this list, n denotes a number and FOO denotes a short signal name.
+
+ @param signame a signal name to decode
+ @returns the signal number or -1 if a number couldn't be determined
+ @see signal_names []
+*/
+const int
+get_signo (const char* signame)
+{
size_t i;
+ typedef unsigned char UChar;
+
+ assert (0 != signame);
+
+ if (isdigit (signame [0])){
+ char *junk;
+ int trans = strtol (signame, &junk, 10);
+
+ if (0 == *junk && 0 == errno)
+ return trans;
+ else
+ return -1;
+ }
+
+ if ( 's' == tolower ((UChar)signame [0])
+ && 'i' == tolower ((UChar)signame [1])
+ && 'g' == tolower ((UChar)signame [2]))
+ signame += 3;
+
+ for (i = 0; signal_names [i].str; ++i) {
+ if (0 == rw_strcasecmp (signal_names [i].str, signame)) {
+ return signal_names [i].val;
+ }
+ }
+
+ return -1;
+}
+
+/**
+ Translates a signal number into a human understandable name
+
+ @param signo a signal number
+ @returns the human understandable name for the signal (minus the SIG
+ prefix), or "#n" if the the name for the number is unknown to the
+ function, where n is signo
+ @see signal_names []
+*/
+const char*
+get_signame (int signo)
+{
+ size_t i;
static char def [16];
- for (i = 0; i < sizeof names / sizeof *names; ++i) {
- if (names [i].val == signo) {
- return names [i].str;
+ for (i = 0; signal_names [i].str; ++i) {
+ if (signal_names [i].val == signo) {
+ return signal_names [i].str;
}
}
@@ -621,7 +722,7 @@
strerror (errno));
return state;
}
-
+
/* parent */
return wait_for_child (child_pid);
}
Index: cmdopt.cpp
===================================================================
--- cmdopt.cpp (revision 428121)
+++ cmdopt.cpp (working copy)
@@ -26,16 +26,21 @@
#include <assert.h>
#include <ctype.h> /* for isspace */
+#include <errno.h> /* for errno */
+#include <signal.h> /* for kill, SIG_IGN */
#include <stdio.h> /* for *printf, fputs */
-#include <stdlib.h> /* for atoi, exit */
+#include <stdlib.h> /* for exit */
#include <string.h> /* for str* */
+#include <unistd.h> /* for getpid */
+#include "exec.h"
#include "util.h"
#include "cmdopt.h"
int timeout = 10; /**< Child process timeout. Default 10. */
int compat = 0; /**< Test compatability mode switch. Defaults to 0 (off). */
+unsigned verbose = 0;
const char* exe_opts = ""; /**< Global command line switches for child
processes. */
const char* in_root = ""; /**< Root directory for input/reference files. */
@@ -58,17 +63,87 @@
"processed after termination. If\n execution takes longer "
"than a certain (configurable) period of time, the\n process is "
"killed\n\n", stderr);
- fputs (" -d dir Root directory for output reference files\n"
- " -h, -? Display usage information and exit\n"
- " -t seconds Watchdog timeout before killing target (default is "
-"10 \n seconds)\n"
- " -x opts Command line options to pass to targets\n"
- " -- Terminate option processing and treat all "
- "following arguments\n as targets\n", stderr);
+ fputs (" -d dir Root directory for output reference files\n"
+ " -h, -? Display usage information and exit\n"
+ " -t seconds Watchdog timeout before killing target (default "
+ "is 10 \n seconds)\n"
+ " -q Set verbosity level to 0 (default)\n"
+ " -v Increase verbosity of output\n"
+ " -x opts Command line options to pass to targets\n",
+ stderr);
+ fputs (" -- Terminate option processing and treat all "
+ "following arguments\n as targets\n"
+ " --compat Use compatability mode test output parsing\n"
+ " --nocompat Use standard test output parsing (default)\n"
+ " --exit=val Exit (now) with a return code of val\n"
+ " --sleep=sec sleep() (now) for sec seconds\n"
+ " --signal=sig Send self signal sig (now)\n"
+ " --ignore=sig Ignore signal sig\n", stderr);
+ fputs ("\n"
+ " All short (single dash) options must be specified seperately.\n"
+ " If a short option takes a value, it may either be provided like"
+ "\n '-sval' or '-s val'\n"
+ " If a long option take a value, it may either be provided like\n"
+ " '--option=value' or '--option value'\n", stderr);
+
exit (status);
}
/**
+ Helper function to read the value for a short option
+
+ @param argv argument array
+ @param idx reference to index for option
+*/
+static char*
+get_short_val (char* const* argv, int* idx)
+{
+ assert (0 != argv);
+ assert (0 != idx);
+
+ if ('\0' == argv [*idx][2])
+ return argv [++(*idx)];
+ else
+ return argv [*idx] + 2;
+}
+
+/**
+ Helper function to read the value for a long option
+
+ @param argv argument array
+ @param idx reference to index for option
+ @param offset length of option name (including leading --)
+*/
+static char*
+get_long_val (char* const* argv, int* idx, unsigned offset)
+{
+ assert (0 != argv);
+ assert (0 != idx);
+
+ if ('\0' == argv [*idx][offset])
+ return argv [++(*idx)];
+ else if ('=' == argv [*idx][offset])
+ return argv [*idx] + offset + 1;
+ else
+ return (char*)0;
+}
+
+/**
+ Helper function to produce 'Unknown option' error message.
+
+ Terminates via show_usage.
+
+ @param opt name of option encountered
+*/
+static void
+bad_option (char* const opt)
+{
+ assert (0 != opt);
+ warn ("Unknown option: %s\n", opt);
+ show_usage (1);
+}
+
+/**
Parses command line arguments for switches and options.
@param argc number of command line arguments
@@ -101,29 +176,23 @@
++i; /* Ignore -r option (makefile compat) */
break;
case 't':
- if ('\0' == argv [i][2])
- val = argv [++i];
- else
- val = argv [i] + 2;
-
- timeout = atoi (val);
+ val = get_short_val (argv, &i);
+ timeout = strtol (val, &val, 10);
+ if (*val || errno)
+ terminate (1, "Unknown value for -t: %s\n", val);
break;
case 'd':
- if ('\0' == argv [i][2])
- val = argv [++i];
- else
- val = argv [i] + 2;
-
- in_root = val;
+ in_root = get_short_val (argv, &i);
break;
case 'x':
- if ('\0' == argv [i][2])
- val = argv [++i];
- else
- val = argv [i] + 2;
-
- exe_opts = val;
+ exe_opts = get_short_val (argv, &i);
break;
+ case 'v':
+ ++verbose;
+ break;
+ case 'q':
+ verbose = 0;
+ break;
case '-':
{
const size_t arglen = strlen (argv [i]);
@@ -132,19 +201,80 @@
if ('\0' == argv [i][2])
return i+1;
- if (8 == arglen && 0 == memcmp ("--compat", argv [i], 8)) {
+ if (8 == arglen && 0 == memcmp ("--compat\0", argv [i], 9)) {
compat = 1;
break;
}
- else if (10 == arglen && 0 == memcmp ("--nocompat", argv [i], 10)) {
+ else if (10 == arglen && 0 == memcmp ("--nocompat\0", argv [i],
+ 11)) {
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
+ 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
+ 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
+ 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
+ bad_option (argv [i]);
+ }
+ else
+ bad_option (argv [i]);
}
- /* Intentionally falling through */
default:
- fprintf (stderr, "Unknown option: %s\n", argv [i]);
- show_usage (1);
+ bad_option (argv [i]);
}
}
Index: exec.h
===================================================================
--- exec.h (revision 428121)
+++ exec.h (working copy)
@@ -32,6 +32,8 @@
int killed;
};
+const int get_signo (const char* signame);
+
const char* get_signame (int signo);
struct exec_attrs exec_file (char** argv);
Index: cmdopt.h
===================================================================
--- cmdopt.h (revision 428121)
+++ cmdopt.h (working copy)
@@ -29,6 +29,7 @@
extern int timeout;
extern int compat;
+extern unsigned verbose; /**< Verbose output mode switch. Defaults to 0 (off) */
extern const char* exe_opts;
extern const char* in_root;
extern const char* exe_name;
