Re: [PATCH] global cleanup for exec utility

[Martin Sebor]

Andrew Black wrote:

[...]

Cool, lots of good changes. A few comments...

--- exec.cpp    (revision 450090)
+++ exec.cpp    (working copy)

[...]

@@ -448,14 +448,27 @@
         alarm (timeout);

while (1) {

-        const pid_t wait_pid = waitpid (child_pid, &state.status, waitopts);
+        const pid_t wait_pid = waitpid (child_pid, &status, waitopts);
         if (child_pid == wait_pid) {
-            if (WIFEXITED (state.status) || WIFSIGNALED (state.status))
+            if (WIFEXITED (status)) {
+                result->exit = WEXITSTATUS (status);
+                switch (result->exit) {
+                case 126:
+                    result->status = ST_EXIST;
+                    break;
+                case 127:
+                    result->status = ST_EXECUTE;
+                    break;
+                }
                 break; /*we've got an exit state, so let's bail*/
-            else if (WIFSTOPPED (state.status))
-                stopped = state.status;
+            } else if (WIFSIGNALED (status)) {

The else should go on the next line. Nothing but the semicolon
or a comment should follow a closing bracket.

+                result->exit = WTERMSIG (status);
+                result->signaled = 1;
+                break; /*we've got an exit state, so let's bail*/
+            } else if (WIFSTOPPED (status))

Same here.

[...]

@@ -502,7 +516,7 @@
                 }

/* Record the signal used*/

-                state.killed = signals [siginx];
+                /* result->killed = signals [siginx];*/

Is this correct? (If it is, the comment is wrong. It should say
why the next line is commented out.)

[...]

--- cmdopt.h    (revision 450090)
+++ cmdopt.h    (working copy)
@@ -27,76 +27,33 @@

[...]

+const char* const get_target ();

The second const is redundant and causes problems, remember?

--- runall.cpp  (revision 450090)
+++ runall.cpp  (working copy)

[...]

@@ -439,10 +389,17 @@
 int
 main (int argc, char *argv [])
 {
+    struct target_opts target_template;
+    char* exe_opts = "";

This is an illegal assignment (const char* to non-const char*).

+
     exe_name = argv [0];
+    memset (&target_template, 0, sizeof target_template);

+ target_template.timeout = 10;

+    target_template.data_dir = "";

Same here.

@@ -457,22 +414,30 @@

[...]

+    if (target_template.core) free (target_template.core);
+    if (target_template.cpu) free (target_template.cpu);
+    if (target_template.data) free (target_template.data);
+    if (target_template.fsize) free (target_template.fsize);
+    if (target_template.nofile) free (target_template.nofile);
+    if (target_template.stack) free (target_template.stack);
+    if (target_template.as) free (target_template.as);

There's no need to check the pointers before calling free,
the function can handle null pointers just fine (also the
style is bad).

[...]

/************************************************************************
 *
 * cmdopt.h - Interface declaration for the option parsing subsystem

This doesn't look right for target.h...

[...]

#ifndef RW_TARGET_H
#define RW_TARGET_H

[...]

struct target_opts {
    char** argv; /**< Target argv array. */
    unsigned timeout; /**< Allowed time for process execution. */
    char* data_dir; /**< Root directory for reference input/output files. */

This member should be const.

I corrected the const problems and got rid of the unnecessary
if's before the calls to free() and committed the whole thing
here: http://svn.apache.org/viewvc?view=rev&rev=451079

Please take a look at the commented out code on line 520 of
exec.cpp and either fix it or correct the comment. Also, please
submit a patch fixing the formatting and other problems I pointed
out here or any other transgressions against the style.

Thanks
Martin