Replace two magic tristates with #define'd names

- process_handle_t.status
- return value of tor_get_exit_code()

3 files changed, 53 insertions, 42 deletions

diff --git a/src/common/util.c b/src/common/util.c
index 91b83db01b..371e09b8db 100644
--- a/src/common/util.c
+++ b/src/common/util.c
@@ -3082,7 +3082,7 @@ tor_spawn_background(const char *const filename, const char **argv,
 
   /* Assume failure to start process */
   memset(process_handle, 0, sizeof(process_handle_t));
-  process_handle->status = -1;
+  process_handle->status = PROCESS_STATUS_ERROR;
 
   /* Set up pipe for stdout */
   if (!CreatePipe(&stdout_pipe_read, &stdout_pipe_write, &saAttr, 0)) {
@@ -3157,7 +3157,7 @@ tor_spawn_background(const char *const filename, const char **argv,
     /* TODO: Close hProcess and hThread in process_handle->pid? */
     process_handle->stdout_pipe = stdout_pipe_read;
     process_handle->stderr_pipe = stderr_pipe_read;
-    process_handle->status = 1;
+    process_handle->status = PROCESS_STATUS_RUNNING;
   }
 
   /* TODO: Close pipes on exit */
@@ -3183,7 +3183,7 @@ tor_spawn_background(const char *const filename, const char **argv,
 
   /* Assume failure to start */
   memset(process_handle, 0, sizeof(process_handle_t));
-  process_handle->status = -1;
+  process_handle->status = PROCESS_STATUS_ERROR;
 
   /* We do the strlen here because strlen() is not signal handler safe,
      and we are not allowed to use unsafe functions between fork and exec */
@@ -3329,7 +3329,7 @@ tor_spawn_background(const char *const filename, const char **argv,
             strerror(errno));
   }
 
-  process_handle->status = 1;
+  process_handle->status = PROCESS_STATUS_RUNNING;
   /* Set stdout/stderr pipes to be non-blocking */
   fcntl(process_handle->stdout_pipe, F_SETFL, O_NONBLOCK);
   fcntl(process_handle->stderr_pipe, F_SETFL, O_NONBLOCK);
@@ -3341,16 +3341,16 @@ tor_spawn_background(const char *const filename, const char **argv,
 #endif // MS_WINDOWS
 }
 
-/* Get the exit code of a process specified by <b>process_handle</b> and
- * store it in <b>exit_code</b>, if set to a non-NULL value.  If
- * <b>block</b> is set to true, the call will block until the process has
- * exited.  Otherwise if the process is still running, the function will
- * return -2, and exit_code will be left unchanged. Returns 0 if the
- * process did exit. If there is a failure, -1 will be returned and the
- * contents of exit_code (if non-NULL) will be undefined. N.B. Under *nix
- * operating systems, this will probably not work in Tor, because
- * waitpid() is called in main.c to reap any terminated child
- * processes.*/
+/* Get the exit code of a process specified by <b>process_handle</b> and store
+ * it in <b>exit_code</b>, if set to a non-NULL value.  If <b>block</b> is set
+ * to true, the call will block until the process has exited.  Otherwise if
+ * the process is still running, the function will return
+ * PROCESS_EXIT_RUNNING, and exit_code will be left unchanged. Returns
+ * PROCESS_EXIT_EXITED if the process did exit. If there is a failure,
+ * PROCESS_EXIT_ERROR will be returned and the contents of exit_code (if
+ * non-NULL) will be undefined. N.B. Under *nix operating systems, this will
+ * probably not work in Tor, because waitpid() is called in main.c to reap any
+ * terminated child processes.*/
 int
 tor_get_exit_code(const process_handle_t process_handle,
                   int block, int *exit_code)
@@ -3365,17 +3365,17 @@ tor_get_exit_code(const process_handle_t process_handle,
     if (retval != WAIT_OBJECT_0) {
       log_warn(LD_GENERAL, "WaitForSingleObject() failed (%d): %s",
               (int)retval, format_win32_error(GetLastError()));
-      return -1;
+      return PROCESS_EXIT_ERROR;
     }
   } else {
     retval = WaitForSingleObject(process_handle.pid.hProcess, 0);
     if (WAIT_TIMEOUT == retval) {
       /* Process has not exited */
-      return -2;
+      return PROCESS_EXIT_RUNNING;
     } else if (retval != WAIT_OBJECT_0) {
       log_warn(LD_GENERAL, "WaitForSingleObject() failed (%d): %s",
                (int)retval, format_win32_error(GetLastError()));
-      return -1;
+      return PROCESS_EXIT_ERROR;
     }
   }
 
@@ -3385,7 +3385,7 @@ tor_get_exit_code(const process_handle_t process_handle,
     if (!success) {
       log_warn(LD_GENERAL, "GetExitCodeProcess() failed: %s",
                format_win32_error(GetLastError()));
-      return -1;
+      return PROCESS_EXIT_ERROR;
     }
   }
 #else
@@ -3395,24 +3395,24 @@ tor_get_exit_code(const process_handle_t process_handle,
   retval = waitpid(process_handle.pid, &stat_loc, block?0:WNOHANG);
   if (!block && 0 == retval) {
     /* Process has not exited */
-    return -2;
+    return PROCESS_EXIT_RUNNING;
   } else if (retval != process_handle.pid) {
     log_warn(LD_GENERAL, "waitpid() failed for PID %d: %s", process_handle.pid,
              strerror(errno));
-    return -1;
+    return PROCESS_EXIT_ERROR;
   }
 
   if (!WIFEXITED(stat_loc)) {
     log_warn(LD_GENERAL, "Process %d did not exit normally",
              process_handle.pid);
-    return -1;
+    return PROCESS_EXIT_ERROR;
   }
 
   if (exit_code != NULL)
     *exit_code = WEXITSTATUS(stat_loc);
 #endif // MS_WINDOWS
 
-  return 0;
+  return PROCESS_EXIT_EXITED;
 }
 
 #ifdef MS_WINDOWS
@@ -3648,11 +3648,7 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port,
 
   /* Static variables are initialized to zero, so child_handle.status=0
    * which corresponds to it not running on startup */
-#ifdef MS_WINDOWS
   static process_handle_t child_handle;
-#else
-  static process_handle_t child_handle;
-#endif
 
   static time_t time_to_run_helper = 0;
   int stdout_status, stderr_status, retval;
@@ -3678,7 +3674,8 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port,
   argv[9] = NULL;
 
   /* Start the child, if it is not already running */
-  if (child_handle.status <= 0 && time_to_run_helper < now) {
+  if (child_handle.status != PROCESS_STATUS_RUNNING &&
+      time_to_run_helper < now) {
     /* Assume tor-fw-helper will succeed, start it later*/
     time_to_run_helper = now + TIME_TO_EXEC_FWHELPER_SUCCESS;
 
@@ -3688,7 +3685,7 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port,
 #else
     tor_spawn_background(filename, argv, &child_handle);
 #endif
-    if (child_handle.status < 0) {
+    if (PROCESS_STATUS_ERROR == child_handle.status) {
       log_warn(LD_GENERAL, "Failed to start port forwarding helper %s",
               filename);
       time_to_run_helper = now + TIME_TO_EXEC_FWHELPER_FAIL;
@@ -3705,7 +3702,7 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port,
   }
 
   /* If child is running, read from its stdout and stderr) */
-  if (child_handle.status > 0) {
+  if (PROCESS_STATUS_RUNNING == child_handle.status) {
     /* Read from stdout/stderr and log result */
     retval = 0;
 #ifdef MS_WINDOWS
@@ -3729,8 +3726,9 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port,
       /* There was a failure */
       retval = -1;
 #ifdef MS_WINDOWS
-    else if (tor_get_exit_code(child_handle, 0, NULL) >= 0) {
-      /* process has exited */
+    else if (tor_get_exit_code(child_handle, 0, NULL) !=
+             PROCESS_EXIT_RUNNING) {
+      /* process has exited or there was an error */
       /* TODO: Do something with the process return value */
       /* TODO: What if the process output something since
        * between log_from_handle and tor_get_exit_code? */
@@ -3751,10 +3749,10 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port,
     if (0 != retval) {
       if (1 == retval) {
         log_info(LD_GENERAL, "Port forwarding helper terminated");
-        child_handle.status = 0;
+        child_handle.status = PROCESS_STATUS_NOTRUNNING;
       } else {
         log_warn(LD_GENERAL, "Failed to read from port forwarding helper");
-        child_handle.status = -1;
+        child_handle.status = PROCESS_STATUS_ERROR;
       }
 
       /* TODO: The child might not actually be finished (maybe it failed or
diff --git a/src/common/util.h b/src/common/util.h
index e0252658ad..d8c73703e2 100644
--- a/src/common/util.h
+++ b/src/common/util.h
@@ -355,8 +355,14 @@ HANDLE load_windows_system_library(const TCHAR *library_name);
 #ifdef UTIL_PRIVATE
 /* Prototypes for private functions only used by util.c (and unit tests) */
 
+/* Values of process_handle_t.status. PROCESS_STATUS_NOTRUNNING must be
+ * 0 because tor_check_port_forwarding depends on this being the initial
+ * statue of the static instance of process_handle_t */
+#define PROCESS_STATUS_NOTRUNNING 0
+#define PROCESS_STATUS_RUNNING 1
+#define PROCESS_STATUS_ERROR -1
 typedef struct process_handle_s {
-  int status; // 0: not running; 1: running; -1: error
+  int status;
 #ifdef MS_WINDOWS
   HANDLE stdout_pipe;
   HANDLE stderr_pipe;
@@ -372,6 +378,11 @@ typedef struct process_handle_s {
 
 int tor_spawn_background(const char *const filename, const char **argv,
                          process_handle_t *process_handle);
+
+/* Return values of tor_get_exit_code() */
+#define PROCESS_EXIT_RUNNING 1
+#define PROCESS_EXIT_EXITED 0
+#define PROCESS_EXIT_ERROR -1
 int tor_get_exit_code(const process_handle_t process_handle,
                       int block, int *exit_code);
 #ifdef MS_WINDOWS
diff --git a/src/test/test_util.c b/src/test/test_util.c
index 40ab09813b..4568fdec1c 100644
--- a/src/test/test_util.c
+++ b/src/test/test_util.c
@@ -1397,7 +1397,7 @@ run_util_spawn_background(const char *argv[], const char *expected_out,
   tt_int_op(process_handle.status, ==, expected_status);
 
   /* If the process failed to start, don't bother continuing */
-  if (process_handle.status == -1)
+  if (process_handle.status == PROCESS_STATUS_ERROR)
     return;
 
   tt_int_op(process_handle.stdout_pipe, >, 0);
@@ -1413,7 +1413,7 @@ run_util_spawn_background(const char *argv[], const char *expected_out,
 
   /* Check it terminated correctly */
   retval = tor_get_exit_code(process_handle, 1, &exit_code);
-  tt_int_op(retval, ==, 0);
+  tt_int_op(retval, ==, PROCESS_EXIT_EXITED);
   tt_int_op(exit_code, ==, expected_exit);
   // TODO: Make test-child exit with something other than 0
 
@@ -1445,7 +1445,8 @@ test_util_spawn_background_ok(void *ptr)
 
   (void)ptr;
 
-  run_util_spawn_background(argv, expected_out, expected_err, 0, 1);
+  run_util_spawn_background(argv, expected_out, expected_err, 0,
+                            PROCESS_STATUS_RUNNING);
 }
 
 /** Check that failing to find the executable works as expected */
@@ -1457,14 +1458,15 @@ test_util_spawn_background_fail(void *ptr)
   const char *expected_out = "ERR: Failed to spawn background process "
                              "- code          9/2\n";
   const char *expected_err = "";
-  const int expected_status = -1;
+  const int expected_status = PROCESS_STATUS_ERROR;
 #else
   const char *argv[] = {BUILDDIR "/src/test/no-such-file", "--test", NULL};
   const char *expected_out = "ERR: Failed to spawn background process "
                              "- code          9/2\n";
   const char *expected_err = "";
-  // TODO: Once we can signal failure to exec, set this to be -1;
-  const int expected_status = 1;
+  /* TODO: Once we can signal failure to exec, set this to be
+   * PROCESS_STATUS_ERROR */
+  const int expected_status = PROCESS_STATUS_RUNNING;
 #endif
 
   (void)ptr;
@@ -1479,7 +1481,7 @@ static void
 test_util_spawn_background_partial_read(void *ptr)
 {
   const int expected_exit = 0;
-  const int expected_status = 1;
+  const int expected_status = PROCESS_STATUS_RUNNING;
 
   int retval, exit_code;
   ssize_t pos;
@@ -1536,7 +1538,7 @@ test_util_spawn_background_partial_read(void *ptr)
 
   /* Check it terminated correctly */
   retval = tor_get_exit_code(process_handle, 1, &exit_code);
-  tt_int_op(retval, ==, 0);
+  tt_int_op(retval, ==, PROCESS_EXIT_EXITED);
   tt_int_op(exit_code, ==, expected_exit);
   // TODO: Make test-child exit with something other than 0