Mailing-List: contact stdcxx-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: stdcxx-dev@incubator.apache.org
Received-SPF: neutral (asf.osuosl.org: local policy)
Message-ID: <44CA2739.7030904@roguewave.com>
Date: Fri, 28 Jul 2006 09:03:21 -0600
From: Andrew Black <ablack@roguewave.com>
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US;
 rv:1.8.0.4) Gecko/20060516 SeaMonkey/1.0.2
MIME-Version: 1.0
To: stdcxx-dev@incubator.apache.org
Subject: Exec util bugfix [patch]
Content-Type: multipart/mixed;
 boundary="------------020800020401080800090400"

--------------020800020401080800090400
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Greetings all.

Attached is a patch that cleans up some of the internal logic of the 
exec utility.  The main purpose is to remove the dependency on the diff 
utility and make the error handling more consistent.

--Andrew Black

Log:
2006-07-28  Andrew Black <ablack@roguewave.com>
	* cmdopt.h (target_name): Declared global variable indicating current 
target
	* cmdopt.cpp (eval_options): Route unknown option message to stderr 
rather than stdout
	* cmdopt.cpp (split_child_opts): Cache output of a strlen() call
	* util.h (warn): Declared
	* util.cpp (warn): Add utility function for generating a (non-fatal) 
error message.
	* util.cpp (terminate): Add executable and target name to output.
	* output.h (parse_output): Alter prototype
	* exec.h (exec_file): Ditto
	* output.cpp (FILE_TEST, check_example): Alter check_example to remove 
reliance on the diff utility, remove FILE_TEST as unneeded.
	* output.cpp (check_test, check_compat_test, parse_output): Move 
opening of output file to parse_output.
	* exec.cpp (wait_for_child, exec_file): use warn() utility
	* runall.cpp (check_target_ok): Ditto
	* runall.cpp (process_results): Alter signature, alter call to parse_output
	* runall.cpp (run_target): set/use target_name global, alter calls to 
exec_file and process_results

--------------020800020401080800090400
Content-Type: text/x-patch;
 name="cleanup3.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="cleanup3.diff"

Index: output.h
===================================================================
--- output.h	(revision 426227)
+++ output.h	(working copy)
@@ -28,6 +28,6 @@
 #define OUTPUT_H
 
 void
-parse_output (const char*, const char*);
+parse_output (const char* target);
 
 #endif   // OUTPUT_H
Index: util.h
===================================================================
--- util.h	(revision 426227)
+++ util.h	(working copy)
@@ -27,6 +27,13 @@
 #ifndef RW_UTIL_H
 #define RW_UTIL_H
 
+/**
+   Generates a non-terminal error message on stderr.
+
+   @param format printf () format string to display on stderr
+*/
+void warn (const char* const format, ...);
+
 void terminate ( const int state, const char* const format, ... );
 
 /* Note: RW_MALLOC should be used rather than malloc within the runall 
Index: exec.cpp
===================================================================
--- exec.cpp	(revision 426227)
+++ exec.cpp	(working copy)
@@ -386,13 +386,13 @@
             }
             else if (ECHILD == errno) {
                 /* should not happen */
-                fprintf (stderr, "waitpid (%d) error: %s\n",
-                         (int)child_pid, strerror (errno));
+                warn ("waitpid (%d) error: %s\n", (int)child_pid, 
+                      strerror (errno));
             }
             else {
                 /* waitpid () error */
-                fprintf (stderr, "waitpid (%d) error: %s\n",
-                         (int)child_pid, strerror (errno));
+                warn ("waitpid (%d) error: %s\n", (int)child_pid, 
+                      strerror (errno));
             }
         }
         else if ((pid_t)0 == wait_pid) {
@@ -522,7 +522,7 @@
 }
 
 /**
-   Entry point to the watchdog subsystem.
+   Entry point to the child process (watchdog) subsystem.
 
    This method fork ()s, creating a child process.  This child process becomes
    a process group leader, redirects input and output files, then exec ()s
@@ -537,7 +537,7 @@
    @see wait_for_child ()
 */
 struct exec_attrs 
-exec_file (const char* exec_name, char** argv)
+exec_file (char** argv)
 {
     const pid_t child_pid = fork ();
 
@@ -546,7 +546,7 @@
 
         assert (0 != argv);
         assert (0 != argv [0]);
-        assert (0 != exec_name);
+        assert (0 != target_name);
 
         /* Set process group ID (so entire group can be killed)*/
         {
@@ -571,7 +571,7 @@
 
         /* Redirect stdin */
         {
-            const int intermit = open_input (exec_name);
+            const int intermit = open_input (target_name);
             replace_file (intermit, 0, "stdin");
         }
 
@@ -604,8 +604,8 @@
 
         execv (argv [0], argv);
 
-        fprintf (error_file, "execv (\"%s\", ...) error: %s\n",
-                 argv [0], strerror (errno));
+        fprintf (error_file, "%s (%s): execv (\"%s\", ...) error: %s\n",
+                 exe_name, target_name, argv [0], strerror (errno));
 
         exit (1);
     }
@@ -613,8 +613,8 @@
     if (-1 == child_pid) {
         /* Fake a failue to execute status return structure */
         struct exec_attrs state = {127, -1};
-        fprintf (stderr, "Unable to create child process for %s: %s\n",
-                 argv [0], strerror (errno));
+        warn ("Unable to create child process for %s: %s\n", argv [0],
+              strerror (errno));
         return state;
     }
 
Index: cmdopt.cpp
===================================================================
--- cmdopt.cpp	(revision 426227)
+++ cmdopt.cpp	(working copy)
@@ -34,11 +34,13 @@
 
 #include "cmdopt.h"
 
-int timeout = 10; /**< Child process timeout.  Default 10 */
-int compat = 0; /**< Test compatability mode switch.  Defaults to 0 (off) */
-const char* exe_opts = ""; /**< Command line switches for child processes */
-const char* in_root = ""; /**< Root directory for input/reference files */
+int timeout = 10; /**< Child process timeout.  Default 10. */
+int compat = 0; /**< Test compatability mode switch.  Defaults to 0 (off). */
+const char* exe_opts = ""; /**< Global command line switches for child 
+                                processes. */
+const char* in_root = ""; /**< Root directory for input/reference files. */
 const char* exe_name; /**< Alias for process argv [0]. */
+const char* target_name;
 
 /**
    Display command line switches for program and terminate.
@@ -73,6 +75,7 @@
    @param argv command line arguments
    @return number of command line arguments parsed.
    @see timeout
+   @see compat
    @see in_root
    @see exe_opts
 */
@@ -140,7 +143,7 @@
         }
             /* Intentionally falling through */
         default:
-            printf ("Unknown option: %s\n", argv [i]);
+            fprintf (stderr, "Unknown option: %s\n", argv [i]);
             show_usage (1);
         }
     }
@@ -151,11 +154,11 @@
 /**
    Translates exe_opts into an array that can be passed to exec ().
 
-   This method malloc ()s two blocks of memory.  The first block is the argv
-   index array.  This is the return value of this method.  The second block
-   is the parsed and split string contents the index referenced.  This block
-   is stored in subscript 1 of the return array.  It is the responsibility of
-   the calling method to free () both blocks.
+   This method malloc ()s two blocks of memory.  The first block is the 
+   generated argv index array.  This is the return value of this method.  The 
+   second block is the parsed and split string contents the index referenced.  
+   This block is stored in element 1 of the return array.  It is the 
+   responsibility of the calling method to free () both blocks.
 
    @warning this logic is UTF-8 unsafe
    @warning I/O redirection command piping isn't supported in the parse logic
@@ -171,10 +174,11 @@
     const char *pos;
     char *target, *last;
     char **table_pos, **argv;
+    const size_t optlen = strlen (exe_opts);
 
     assert (0 != exe_opts);
 
-    if (0 == strlen (exe_opts)) {
+    if (0 == optlen) {
         /* Alloc a an index array to hold the program name  */
         argv = (char**)RW_MALLOC (2 * sizeof (char*));
 
@@ -183,15 +187,14 @@
         return argv;
     }
 
-    table_pos = argv = (char**)RW_MALLOC (
-        (strlen (exe_opts) + 5) * sizeof (char*) / 2);
+    table_pos = argv = (char**)RW_MALLOC ((optlen + 5) * sizeof (char*) / 2);
     /* (strlen (exe_opts)+5)/2 is overkill for the most situations, but it is 
        just large enough to handle the worst case scenario.  The worst case 
        is a string similar to 'x y' or 'x y z', requiring array lengths of 4 
        and 5 respectively.
     */
 
-    last = target = argv [1] = (char*)RW_MALLOC (strlen (exe_opts) + 1);
+    last = target = argv [1] = (char*)RW_MALLOC (optlen + 1);
 
     /* Transcribe the contents, handling escaping and splitting */
     for (pos = exe_opts; *pos; ++pos) {
Index: output.cpp
===================================================================
--- output.cpp	(revision 426227)
+++ output.cpp	(working copy)
@@ -50,7 +50,7 @@
 
 
 /**
-   Parses output file out_name for test exec_name.
+   Parses output file out_name for test target_name.
    
    This method tries to open out_name.  If this succedes, it then searches
    the file for a result summary as produced by the rwtest driver.
@@ -68,31 +68,20 @@
      - 0 [assertion count] [failed assertion count] [pass rate]\n
        Parsed results
 
-   @param exec_name the name of the executable that generated the output file
-   @param out_name the name of the output file being parsed
+   @param data pointer to file structure for output file being parsed
 */
 static void
-check_test (const char* exec_name, const char* out_name)
+check_test (FILE* data)
 {
-    FILE* data = fopen (out_name, "r");
     unsigned r_lvl = 0, r_active = 0, r_total = 0;
     unsigned asserts = 0, failed = 0;
     int fmt_ok = 0;
     unsigned fsm = 0;
     char tok;
 
-    assert (0 != exec_name);
-    assert (0 != out_name);
+    assert (0 != target_name);
+    assert (0 != data);
 
-    if (0 == data) {
-        if (ENOENT != errno) {
-            printf ("Error opening %s: %s\n", out_name, strerror (errno));
-            return;
-        }
-        puts ("OUTPUT");
-        return;
-    }
-
     for (tok = fgetc (data); fsm < 6 && !feof (data); tok = fgetc (data)) {
         switch (tok) {
         case '\n':
@@ -124,7 +113,7 @@
             if (6 < r_lvl) {
                 /* The 0.new tests produces errors, and are all 
                    expected to be active, so invert the total */
-                if (8 == r_lvl && 0 == strcmp (exec_name,"0.new"))
+                if (8 == r_lvl && 0 == strcmp (target_name, "0.new"))
                     r_active = r_total-r_active;
                 failed += r_active;
                 asserts += r_total;
@@ -148,41 +137,28 @@
     else {
         puts ("FORMAT");
     }
-
-    fclose (data);
 }
 
 /**
-   Parses output file out_name for test exec_name.
+   Parses output file out_name for test target_name.
    
    This method is a reimplementation of check_test ().  The difference between
    this method and check_test () is how it parses the results.  This version
    parses compatability layer output, rather than the test driver output.
 
-   @param exec_name the name of the executable that generated the output file
-   @param out_name the name of the output file being parsed
+   @param data pointer to file structure for output file being parsed
    @see check_test ()
 */
 static void
-check_compat_test (const char* out_name)
+check_compat_test (FILE* data)
 {
-    FILE* data = fopen (out_name, "r");
     unsigned asserts = 0, failed = 0;
     int read = 0;
     unsigned fsm = 0;
     char tok;
 
-    assert (0 != out_name);
+    assert (0 != data);
 
-    if (0 == data) {
-        if (ENOENT != errno) {
-            printf ("Error opening %s: %s\n", out_name, strerror (errno));
-            return;
-        }
-        puts ("OUTPUT");
-        return;
-    }
-
     fseek (data, -64, SEEK_END); /* Seek near the end of the file */
 
     for (tok = fgetc (data); fsm < 4 && !feof (data); tok = fgetc (data)) {
@@ -221,36 +197,27 @@
     else {
         puts ("FORMAT");
     }
-
-    fclose (data);
 }
 
 /**
-   Sanity test macro for file descriptor operations.
-   
-   @killme this should be removed after removing the dependancy on the
-   posix diff utility
-   
-   @param op human understandable name for operation
-   @param x variable to test the result for
-   @see check_example
+   Arbitrary constant controling static read buffer size.
+
+   @see check_example ()
 */
-#define FILE_TEST(op, x)                                                \
-    if (-1==(x))                                                        \
-        terminate (2, op " failed: %s\n", strerror (errno))
+#define DELTA_BUF_LEN 64
 
 /**
-   Parses output file out_name for the example exec_name.
+   Parses output file out_name for the example target_name.
    
    This method tries to compare out_name against a reference output file and
-   displays a result code.  The reference file is determined by exec_name and
+   displays a result code.  The reference file is determined by target_name and
    the in_root global variable.
    This method relies on the POSIX diff utility at this time.  This dependancy
    needs to be removed, though doing so will require a rewrite of the method.
 
    Reference file locations:
-     - [in_root]/manual/out/[exec_name].out
-     - [in_root]/tutorial/out/[exec_name].out
+     - [in_root]/manual/out/[target_name].out
+     - [in_root]/tutorial/out/[target_name].out
 
    Output messages produced:
      - NOREF\n
@@ -260,49 +227,54 @@
      - 0\n
        Output file matches the reference file (check passes)
 
-   @param exec_name the name of the executable that generated the output file
-   @param out_name the name of the output file being parsed
+   @todo add logic to handle differing line endings (CR vs LF vs CRLF)
+
+   @param output_name the name of the output file
+   @param data pointer to file structure for output file being parsed
    @see in_root
-   @see FILE_TEST ()
-   @todo remove dependancy on POSIX diff utility
 */
 static void
-check_example (const char* const exec_name, const char* const out_name)
+check_example (char* const out_name, FILE* output)
 {
     struct stat file_info;
     const size_t root_len = strlen (in_root);
     char* const ref_name = (char*)RW_MALLOC (root_len 
-                                            + strlen (exec_name) + 19);
-    int state = -1;
+                                             + strlen (target_name) + 19);
+    FILE* reference;
 
     assert (0 != in_root);
     assert (0 < root_len);
-    assert (0 != exec_name);
-    assert (0 != out_name);
+    assert (0 != target_name);
+    assert (0 != output);
 
-    /* Try in_root/manual/out/exec_name.out */
+    /* Try in_root/manual/out/target_name.out */
     memcpy (ref_name, in_root, root_len+1);
     strcat (ref_name, "/manual/out/");
-    strcat (ref_name, exec_name);
+    strcat (ref_name, target_name);
     strcat (ref_name, ".out");
 
     if (0 > stat (ref_name, &file_info)) {
         if (ENOENT != errno) {
-            printf ("stat (%s) error: %s\n", ref_name, strerror (errno));
+            warn ("stat (%s) error: %s\n", exe_name, ref_name, 
+                  strerror (errno));
+            puts ("BADREF");
             free (ref_name);
             return;
         }
                         
         /* If that doesn't exist, try 
-           in_root/tutorial/out/exec_name.out */
+           in_root/tutorial/out/target_name.out */
         memcpy (ref_name, in_root, root_len+1);
         strcat (ref_name, "/tutorial/out/");
-        strcat (ref_name, exec_name);
+        strcat (ref_name, target_name);
         strcat (ref_name, ".out");
 
         if (0 > stat (ref_name, &file_info)) {
-            if (ENOENT != errno)
-                printf ("stat (%s) error: %s\n", ref_name, strerror (errno));
+            if (ENOENT != errno) {
+                warn ("stat (%s) error: %s\n", exe_name, ref_name, 
+                      strerror (errno));
+                puts ("BADREF");
+            }
             else
                 puts (" NOREF");
 
@@ -311,93 +283,98 @@
         }
     }
 
-    const pid_t child_pid = fork ();
+    reference = fopen (ref_name, "r");
 
-    if (0 == child_pid) {   /* child */
-        /* Cache stdout (hopefully) for use if execv () fails */
-        int error_cache = dup (2);
-        FILE* error_file;
-        FILE_TEST ("dup (stderr)", error_cache);
-
-        FILE_TEST ("close (stdin)",close (0));
-        FILE_TEST ("close (stdin)",close (1));
-        FILE_TEST ("close (stderr)",close (2));
-
-        /* Todo: diff with --strip-trailing-cr on windows */
-        execlp ("diff", "diff", ref_name, out_name, (char *)0);
-
-        if ((error_file = fdopen (error_cache, "a")))
-            fprintf (error_file, "execlp (\"diff\", ...) error: %s\n",
-                     strerror (errno));
-
-        exit (2);
+    if (0 == reference) {
+        int cache = errno; /* caching errno, as puts could alter it */
+        if (ENOENT != cache)
+            warn ("Error opening %s: %s\n", ref_name, strerror (cache));
+        puts ("BADREF");
     }
+    else {
+        char out_buf [DELTA_BUF_LEN], ref_buf [DELTA_BUF_LEN];
+        size_t out_read, ref_read;
+        int match = 1;
 
-    while (1) {
-        const pid_t wait_pid = waitpid (child_pid, &state, 0);
-
-        if (child_pid == wait_pid) {
-
-            if (WIFEXITED (state)) {
-                const int retcode = WEXITSTATUS (state);                
-                switch (retcode) {
-                case 0:
-                    puts ("     0");
-                    break;
-                case 1:
-                    puts ("OUTPUT");
-                    break;
-                default:
-                    printf ("diff returned %d\n", retcode);
-                }
+        while (!feof (reference) && !feof (output)) {
+            /* First, read a block from the files into the buffer */
+            out_read = fread (out_buf, DELTA_BUF_LEN, 1, output);
+            if (ferror (output)) {
+                warn ("Error reading %s: %s\n", out_name, strerror (errno));
+                match = 0;
                 break;
             }
-            else if (WIFSIGNALED (state)) {
-                printf ("diff exited with %s\n", 
-                        get_signame (WTERMSIG (state)));
+            ref_read = fread (ref_buf, DELTA_BUF_LEN, 1, reference);
+            if (ferror (reference)) {
+                warn ("Error reading %s: %s\n", ref_name, strerror (errno));
+                match = 0;
                 break;
             }
-/*
-            else if (WIFSTOPPED (state)) {
-                printf ("process %d stopped\n", (int)child_pid);
+
+            /* Then, check if the amount of data read or the state of the 
+               files differs
+            */
+            if (ref_read != out_read || feof (reference) != feof (output)) {
+                match = 0;
+                break;
             }
-            else if (WIFCONTINUED (state)) {
-                printf ("process %d continued\n", (int)child_pid);
+
+            /* Finally, check if the contents of the buffers differ */
+            if (0 != memcmp (out_buf, ref_buf, DELTA_BUF_LEN)) {
+                match = 0;
+                break;
             }
-*/
         }
-    }
 
+        if (match)
+            puts ("     0");
+        else
+            puts ("OUTPUT");
+
+        fclose (reference);
+    }
     free (ref_name);
 }
 
 /**
-   Dispatch method that invoks another method to parse the output file.
+   Umbrella (dispatch) function to analyse the (saved) output of target
 
-   @param target the path to the executable that generated the output file
-   @param exec_name the name of the executable that generated the output file
-   @see check_test ()
-   @see check_compat_test ()
-   @see check_example ()
+   @param target the path to the executable that generated the output file 
+   being parsed.
 */
 void
-parse_output (const char* target, const char* exec_name)
+parse_output (const char* target)
 {
     const size_t path_len = strlen (target);
     char* const out_name = (char*)RW_MALLOC (path_len + 5);
+    FILE* data;
+
+    assert (0 != target);
+
     memcpy (out_name, target, path_len + 1);
     strcat (out_name,".out");
 
-    if (!strlen (in_root)) {
-        /* If there is not an input directory, look at the assertion tags */
-        if (!compat)
-            check_test (exec_name, out_name);
-        else
-            check_compat_test (out_name);
+    data = fopen (out_name, "r");
+
+    if (0 == data) {
+        if (ENOENT != errno)
+            warn ("Error opening %s: %s\n", out_name, strerror (errno));
+        puts ("OUTPUT");
     }
     else {
-        /* Otherwise, diff against the output file */
-        check_example (exec_name, out_name);
+        if (!strlen (in_root)) {
+            /* If there is not an input directory, look at the assertion tags */
+
+            if (!compat)
+                check_test (data);
+            else
+                check_compat_test (data);
+        }
+        else {
+            /* Otherwise, diff against the output file */
+            check_example (out_name, data);
+        }
+        fclose (data);
     }
     free (out_name);
 }
Index: util.cpp
===================================================================
--- util.cpp	(revision 426227)
+++ util.cpp	(working copy)
@@ -28,8 +28,24 @@
 #include <string.h> /* for strerror */
 #include <sys/types.h> /* for size_t */
 
+#include "cmdopt.h" /* for exe_name, target_name */
+
 #include "util.h"
 
+void
+warn (const char* const format, ...)
+{
+    va_list args;
+
+    assert (0 != format);
+
+    fprintf (stderr, "%s (%s): ", exe_name, target_name);
+
+    va_start (args, format);
+    vfprintf (stderr, format, args);
+    va_end (args);
+}
+
 /**
    Wrapper for exit (), providing a terminal error message on stderr.
 
@@ -44,6 +60,8 @@
     assert (0 != format);
     assert (0 != state);
 
+    fprintf (stderr, "%s (%s): ", exe_name, target_name);
+
     va_start (args, format);
     vfprintf (stderr, format, args);
     va_end (args);
Index: exec.h
===================================================================
--- exec.h	(revision 426227)
+++ exec.h	(working copy)
@@ -32,10 +32,8 @@
     int killed;
 };
 
-const char* 
-get_signame (int);
+const char* get_signame (int signo);
 
-struct exec_attrs 
-exec_file (const char*, char**);
+struct exec_attrs exec_file (char** argv);
 
 #endif   // RW_EXEC
Index: cmdopt.h
===================================================================
--- cmdopt.h	(revision 426227)
+++ cmdopt.h	(working copy)
@@ -32,12 +32,13 @@
 extern const char* exe_opts;
 extern const char* in_root;
 extern const char* exe_name;
+extern const char* target_name; /**< Alias for current target name. */
 
 void 
 show_usage(const int status);
 
 int 
-eval_options (const int argc, /* const */ char* const argv[]);
+eval_options (const int argc, char* const argv[]);
 
 char**
 split_child_opts();
Index: runall.cpp
===================================================================
--- runall.cpp	(revision 426227)
+++ runall.cpp	(working copy)
@@ -76,7 +76,8 @@
 
     if (0 > stat (target, &file_info)) {
         if (ENOENT != errno) {
-            printf ("Stat error: %s\n", strerror (errno));
+            warn ("Error stating %s: %s\n", target, strerror (errno));
+            puts ("ERROR");
             return 0;
         }
         file_info.st_mode = 0; /* force mode on non-existant file to 0 */
@@ -110,8 +111,10 @@
         strcat (tmp_name,".o");
 
         if (0 > stat (tmp_name, &file_info)) {
-            if (ENOENT != errno)
-                printf ("stat (%s) error: %s\n", tmp_name, strerror (errno));
+            if (ENOENT != errno) {
+                warn ("Error stating %s: %s\n", tmp_name, strerror (errno));
+                puts ("ERROR");
+            }
             else
                 puts ("  COMP");
         }
@@ -152,11 +155,10 @@
    @see check_example ()
 */
 static void
-process_results (const char* target, const char* exec_name, 
-                 const struct exec_attrs* result)
+process_results (const char* target, const struct exec_attrs* result)
 {
     if (0 == result->status) {
-        parse_output (target, exec_name);
+        parse_output (target);
     } 
     else if (WIFEXITED (result->status)) {
         const int retcode = WEXITSTATUS (result->status);
@@ -229,22 +231,22 @@
 run_target (char* target, char** childargv)
 {
     struct exec_attrs status;
-    const char* const exec_name = rw_basename (target);
 
     assert (0 != target);
     assert (0 != childargv);
 
     childargv [0] = target;
+    target_name = rw_basename (target);
 
-    printf ("%-18.18s ", exec_name);
+    printf ("%-18.18s ", target_name);
     fflush (stdout);
 
     if (!check_target_ok (target))
         return;
 
-    status = exec_file (exec_name, childargv);
+    status = exec_file (childargv);
 
-    process_results (target, exec_name, &status);
+    process_results (target, &status);
 }
 
 /**

--------------020800020401080800090400--