commit 1ad986335a5d2fc0c9952412d25037fb70226472 Author: Steven Murdoch Steven.Murdoch@cl.cam.ac.uk Date: Mon Aug 22 19:43:38 2011 +0100
Tidy up subprocess code
- Better error handling - Write description of functions - Don't assume non-negative process return values --- src/common/util.c | 105 +++++++++++++++++++++++++++++++++---------------- src/common/util.h | 3 +- src/test/test_util.c | 14 ++++--- 3 files changed, 81 insertions(+), 41 deletions(-)
diff --git a/src/common/util.c b/src/common/util.c index 48e48ce..36b5966 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -3070,6 +3070,7 @@ tor_spawn_background(const char *const filename, const char **argv) saAttr.bInheritHandle = TRUE; saAttr.lpSecurityDescriptor = NULL;
+ /* Assume failure to start process */ process_handle.status = -1;
/* Set up pipe for stdout */ @@ -3083,6 +3084,7 @@ tor_spawn_background(const char *const filename, const char **argv) log_warn(LD_GENERAL, "Failed to configure pipe for stdout communication with child process: %s", format_win32_error(GetLastError())); + return process_handle; }
/* Set up pipe for stderr */ @@ -3096,6 +3098,7 @@ tor_spawn_background(const char *const filename, const char **argv) log_warn(LD_GENERAL, "Failed to configure pipe for stderr communication with child process: %s", format_win32_error(GetLastError())); + return process_handle; }
/* Create the child process */ @@ -3163,10 +3166,8 @@ tor_spawn_background(const char *const filename, const char **argv)
static int max_fd = -1;
- // XXX - process_handle.pid = 0; - process_handle.stderr_pipe = 0; - process_handle.stdout_pipe = 0; + /* Assume failure to start */ + process_handle.status = -1;
/* 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 */ @@ -3180,7 +3181,6 @@ tor_spawn_background(const char *const filename, const char **argv) log_warn(LD_GENERAL, "Failed to set up pipe for stdout communication with child process: %s", strerror(errno)); - process_handle.status = -1; return process_handle; }
@@ -3189,7 +3189,6 @@ tor_spawn_background(const char *const filename, const char **argv) log_warn(LD_GENERAL, "Failed to set up pipe for stderr communication with child process: %s", strerror(errno)); - process_handle.status = -1; return process_handle; }
@@ -3276,7 +3275,6 @@ tor_spawn_background(const char *const filename, const char **argv) (void) nbytes;
_exit(255); - process_handle.status = -1; return process_handle; /* Never reached, but avoids compiler warning */ }
@@ -3288,7 +3286,6 @@ tor_spawn_background(const char *const filename, const char **argv) close(stdout_pipe[1]); close(stderr_pipe[0]); close(stderr_pipe[1]); - process_handle.status = -1; return process_handle; }
@@ -3330,37 +3327,64 @@ 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.*/ int -tor_get_exit_code(const process_handle_t process_handle, int block) +tor_get_exit_code(const process_handle_t process_handle, + int block, int *exit_code) { #ifdef MS_WINDOWS - DWORD exit_code; - BOOL retval; + DWORD retval; + BOOL success; + if (block) { - exit_code = WaitForSingleObject(process_handle.pid.hProcess, INFINITE); - retval = GetExitCodeProcess(process_handle.pid.hProcess, &exit_code); + /* Wait for the process to exit */ + retval = WaitForSingleObject(process_handle.pid.hProcess, INFINITE); + if (retval != WAIT_OBJECT_0) { + log_warn(LD_GENERAL, "WaitForSingleObject() failed (%d): %s", + (int)retval, format_win32_error(GetLastError())); + return -1; + } } else { - exit_code = WaitForSingleObject(process_handle.pid.hProcess, 0); - if (WAIT_TIMEOUT == exit_code) { - // Process has not exited + retval = WaitForSingleObject(process_handle.pid.hProcess, 0); + if (WAIT_TIMEOUT == retval) { + /* Process has not exited */ return -2; + } else if (retval != WAIT_OBJECT_0) { + log_warn(LD_GENERAL, "WaitForSingleObject() failed (%d): %s", + (int)retval, format_win32_error(GetLastError())); + return -1; } - retval = GetExitCodeProcess(process_handle.pid.hProcess, &exit_code); } - - if (!retval) { - log_warn(LD_GENERAL, "GetExitCodeProcess() failed: %s", - format_win32_error(GetLastError())); - return -1; - } else { - return exit_code; + + if (exit_code != NULL) { + success = GetExitCodeProcess(process_handle.pid.hProcess, + (PDWORD)exit_code); + if (!success) { + log_warn(LD_GENERAL, "GetExitCodeProcess() failed: %s", + format_win32_error(GetLastError())); + return -1; + } } + + return 0; #else int stat_loc; int retval;
- retval = waitpid(process_handle.pid, &stat_loc, 0); - if (retval != process_handle.pid) { + retval = waitpid(process_handle.pid, &stat_loc, block?0:WNOHANG); + if (!block && 0 == retval) { + /* Process has not exited */ + return -2; + } else if (retval != process_handle.pid) { log_warn(LD_GENERAL, "waitpid() failed for PID %d: %s", process_handle.pid, strerror(errno)); return -1; @@ -3371,7 +3395,8 @@ tor_get_exit_code(const process_handle_t process_handle, int block) return -1; }
- return WEXITSTATUS(stat_loc); + if (exit_code != NULL) + *exit_code = WEXITSTATUS(stat_loc); #endif // MS_WINDOWS }
@@ -3457,6 +3482,9 @@ tor_read_all_from_process_stderr(const process_handle_t process_handle, }
#ifdef MS_WINDOWS +/** Read from stream, and send lines to log at the specified log level. + * Returns -1 if there is a error reading, and 0 otherwise. + */ static int log_from_handle(HANDLE *pipe, int severity) { @@ -3501,7 +3529,7 @@ log_from_handle(HANDLE *pipe, int severity) } log_fn(severity, LD_GENERAL, "Port forwarding helper says: %s", buf+start); } - return pos; + return 0; }
#else @@ -3588,11 +3616,13 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port, { /* When fw-helper succeeds, how long do we wait until running it again */ #define TIME_TO_EXEC_FWHELPER_SUCCESS 300 -/* When fw-helper fails, how long do we wait until running it again */ +/* When fw-helper failed to start, how long do we wait until running it again */ #define TIME_TO_EXEC_FWHELPER_FAIL 60
+ /* 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 = {0, NULL, NULL, {NULL, NULL, 0, 0}}; + static process_handle_t child_handle; #else static process_handle_t child_handle; #endif @@ -3629,6 +3659,7 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port, if (child_handle.status < 0) { log_warn(LD_GENERAL, "Failed to start port forwarding helper %s", filename); + time_to_run_helper = now + TIME_TO_EXEC_FWHELPER_FAIL; return; } #ifdef MS_WINDOWS @@ -3647,7 +3678,7 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port, #ifdef MS_WINDOWS stdout_status = log_from_handle(child_handle.stdout_pipe, LOG_INFO); stderr_status = log_from_handle(child_handle.stderr_pipe, LOG_WARN); - // If we got this far (on Windows), the process started + /* If we got this far (on Windows), the process started */ retval = 0; #else stdout_status = log_from_pipe(child_handle.stdout_handle, @@ -3665,13 +3696,18 @@ 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) >= 0) { + else if (tor_get_exit_code(child_handle, 0, NULL) >= 0) { /* process has exited */ + /* 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? */ retval = 1; } #else else if (1 == stdout_status || 1 == stderr_status) - /* stdout or stderr was closed */ + /* stdout or stderr was closed, the process probably + * exited. It will be reaped by waitpid() in main.c */ + /* TODO: Do something with the process return value */ retval = 1; #endif else @@ -3682,13 +3718,14 @@ 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; } else { log_warn(LD_GENERAL, "Failed to read from port forwarding helper"); + child_handle.status = -1; }
/* TODO: The child might not actually be finished (maybe it failed or closed stdout/stderr), so maybe we shouldn't start another? */ - child_handle.status = -1; } } } diff --git a/src/common/util.h b/src/common/util.h index d0ad8eb..9022334 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -372,7 +372,8 @@ typedef struct process_handle_s {
process_handle_t tor_spawn_background(const char *const filename, const char **argv); -int tor_get_exit_code(const process_handle_t pid, int block); +int tor_get_exit_code(const process_handle_t process_handle, + int block, int *exit_code); ssize_t tor_read_all_handle(HANDLE h, char *buf, size_t count, HANDLE hProcess); ssize_t tor_read_all_from_process_stdout(const process_handle_t process_handle, char *buf, size_t count); diff --git a/src/test/test_util.c b/src/test/test_util.c index 40de76a..3916b4c 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -1382,7 +1382,7 @@ run_util_spawn_background(const char *argv[], const char *expected_out, const char *expected_err, int expected_exit, int expected_status) { - int retval; + int retval, exit_code; ssize_t pos; process_handle_t process_handle; char stdout_buf[100], stderr_buf[100]; @@ -1408,8 +1408,9 @@ run_util_spawn_background(const char *argv[], const char *expected_out, tt_int_op(pos, ==, strlen(expected_out));
/* Check it terminated correctly */ - retval = tor_get_exit_code(process_handle, 1); - tt_int_op(retval, ==, expected_exit); + retval = tor_get_exit_code(process_handle, 1, &exit_code); + tt_int_op(retval, ==, 0); + tt_int_op(exit_code, ==, expected_exit); // TODO: Make test-child exit with something other than 0
/* Check stderr */ @@ -1488,7 +1489,7 @@ test_util_spawn_background_partial_read(void *ptr) const int expected_exit = 0; const int expected_status = 0;
- int retval; + int retval, exit_code; ssize_t pos; process_handle_t process_handle; char stdout_buf[100], stderr_buf[100]; @@ -1531,8 +1532,9 @@ test_util_spawn_background_partial_read(void *ptr) #endif
/* Check it terminated correctly */ - retval = tor_get_exit_code(process_handle, 1); - tt_int_op(retval, ==, expected_exit); + retval = tor_get_exit_code(process_handle, 1, &exit_code); + tt_int_op(retval, ==, 0); + tt_int_op(exit_code, ==, expected_exit); // TODO: Make test-child exit with something other than 0
/* Check stderr */