And, take 2 on the patch. I believe I've resolved the issue with soft
killing the child processes, but I am not completely happy with it.
Part of this reason for this unhappiness is that the MSDN documentation
on process creation flags seems to indicate that the Control-C signal
handling is disabled on the new process group. Unfortunately, I don't
think there's any way around this. Still, I don't see error messages
when a child process is killed, so I think it will work.
--Andrew Black
Andrew Black wrote:
Greetings
Now that the preparation patch has been applied, I have updated the
actual port patch to reflect the current state of subversion. This
patch is attached. Below is the revised changelog which should reflect
the changes made in this iteration of the patch.
A concern I have about this patch is in the behavior of the soft kill
logic. When I test the child process kill logic (by issuing a command
like 'exec.exe -t3 "exec.exe --sleep=10"'), I receive error output
similar to the following:
> Sending child process Control-C failed with error 87. Additionally,
FormatMessage failed with error 8.
> Sending child process Control-Break failed with error 87.
Additionally, FormatMessage failed with error 8.
From these error messages, I believe that the soft kills aren't being
sent to the child process, and I am uncertain as to the reason. I would
appreciate it if someone who has more experience with Windows could look
into the problem. I don't know if this is a big enough issue to stop
the patch from going in as it stands, as we are able to hard kill
processes, but it is something to consider first.
--Andrew Black
Log:
* exec.h [_WIN32 || _WIN64] (exec_attrs): Alter structure definition
for windows.
* exec.cpp [_WIN32 || _WIN64]: Use windows.h and process.h in place
of unistd.h and sys/wait.h on windows.
[!_WIN32 && !_WIN64] (handle_alrm, wait_for_child, open_input,
replace_file, exec_file): Reduce scope of (existing) functions to
non-windows platforms only.
[_WIN32 || _WIN64] (open_input, merge_argv, warn_last_error,
exec_file): Define implementation of named functions for windows
platforms only.
* cmdopt.cpp [_WIN32 || _WIN64]: Use windows.h in place of unistd.h
on windows.
(escape_code, default_path_sep, suffix_sep, exe_suffix_len): Alter
values for windows.
(rw_sleep, rw_signal): Define platform independent wrapper for
sleep/Sleep and sigaction/signal.
(eval_options): Use above.
* runall.cpp [!_WIN32 && !_WIN64]: Don't include sys/wait.h on windows.
(S_IXUSR, S_IXGRP, S_IXOTH): Define values if undefined (for
windows).
(check_target_ok) [_WIN32 || _WIN64]: Make object file comparison
operating system specific.
(process_results) [_WIN32 || _WIN64]: Make error state processing
code operating system specific.
(rw_basename) [_WIN32 || _WIN64]: Treat / as an additional path
separator on windows.
Martin Sebor wrote:
Andrew Black wrote:
I believe I posted my patch to the list prior to the referenced
change being submitted to subversion, but I am uncertain why those
particular hunks failed to apply (running svn up appears to have
merged them without conflicts).
Attached is a revised version of the patch that tries to address the
formatting issues in changed code (a formatting and comment cleanup
sweep is needed after the windows port is finished). I have also
tried to alter the log based on the changes to the patch and the
referenced link.
Great, thank you! Applied as follows:
http://svn.apache.org/viewvc?rev=431984&view=rev
Martin
Index: exec.cpp
===================================================================
--- exec.cpp (revision 432251)
+++ exec.cpp (working copy)
@@ -37,10 +37,15 @@
#include <stdlib.h>
#include <string.h> /* for str*, mem* */
-#include <unistd.h> /* for close, dup, exec, fork */
+#if !defined (_WIN32) && !defined (_WIN64)
+# include <unistd.h> /* for close, dup, exec, fork */
+# include <sys/wait.h>
+#else
+# include <windows.h> /* for PROCESS_INFORMATION, ... */
+# include <process.h> /* for CreateProcess, ... */
+#endif
#include <sys/stat.h> /* for S_* */
#include <sys/types.h>
-#include <sys/wait.h>
#include "cmdopt.h"
#include "util.h"
@@ -354,6 +359,7 @@
return def;
}
+#if !defined (_WIN32) && !defined (_WIN64)
/**
Callback used to set the alarm_timeout flag in response to recieving
the signal SIGALRM
@@ -714,3 +720,306 @@
/* parent */
return wait_for_child (child_pid);
}
+#else /* _WIN{32,64} */
+/**
+ Opens an input file, based on exec_name, using the child_sa security
+ setting.
+
+ Takes an executable name and security setting, and tries to open an input
+ file based on these variables and the value of the in_root global variable.
+ If a file is found in neither of two locations derived from these
+ variables, or if in_root is a null string, it returns null.
+ If a file system error occurs when opening a file, INVALID_HANDLE_VALUE
+ is returned (and should be checked for).
+
+ Source file locations:
+ - [in_root]/manual/in/[exec_name].in
+ - [in_root]/tutorial/in/[exec_name].in
+
+ @param exec_name the name of executable being run
+ @param child_sa pointer to a SECURITY_ATTRIBUTES structure
+ @returns the file descriptor of the opened file
+ @see in_root
+*/
+static HANDLE
+open_input (const char* exec_name, SECURITY_ATTRIBUTES* child_sa)
+{
+ const size_t root_len = strlen (in_root);
+ HANDLE intermit;
+ DWORD error;
+ char* tmp_name;
+
+ assert (0 != exec_name);
+ assert (0 != in_root);
+ assert (0 != child_sa);
+
+ if (!root_len)
+ return 0;
+
+ /* Try in_root\manual\in\exec_name.in */
+ tmp_name = reference_name ("manual", "in");
+
+ intermit = CreateFile (tmp_name, GENERIC_READ, FILE_SHARE_READ, child_sa,
+ OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
+
+ error = GetLastError ();
+ /* If we found the file, return the descriptor */
+ if (INVALID_HANDLE_VALUE != intermit || (2 != error && 3 != error)) {
+ free (tmp_name);
+ return intermit;
+ }
+
+ /* Try in_root\tutorial\in\exec_name.in */
+ free (tmp_name);
+ tmp_name = reference_name ("tutorial", "in");
+ intermit = CreateFile (tmp_name, GENERIC_READ, FILE_SHARE_READ, child_sa,
+ OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
+
+ /* If we didn't find the file, null out the handle to return */
+ error = GetLastError ();
+ if (INVALID_HANDLE_VALUE == intermit && (2 == error || 3 == error)) {
+ intermit = 0;
+ }
+
+ free (tmp_name);
+ return intermit;
+}
+
+/**
+ Convert an argv array into a string that can be passed to CreateProcess.
+
+ This method allocates memory which the caller is responsible for free ()ing.
+ The provided argv array is converted into a series of quoted strings.
+
+ @param argv argv array to convert
+ @return allocated array with converted contents
+*/
+static char*
+merge_argv (char** argv)
+{
+ size_t len = 0;
+ char** opts;
+ char* term;
+ char* merge;
+ char* pos;
+
+ assert (0 != argv);
+
+ for (opts = argv; *opts; ++opts) {
+ len += 3; /* for open ", close " and trailing space or null */
+ for (term = *opts; *term; ++term) {
+ if ('"' == *term || escape_code == *term)
+ ++len; /* Escape embedded "s and ^s*/
+ ++len;
+ }
+ }
+
+ pos = merge = (char*)RW_MALLOC (len);
+ for (opts = argv; *opts; ++opts) {
+ *(pos++) = '"';
+ for (term = *opts; *term; ++term) {
+ if ('"' == *term || escape_code == *term)
+ *(pos++) = escape_code; /* Escape embedded "s and ^s*/
+ *(pos++) = *term;
+ }
+ *(pos++) = '"';
+ *(pos++) = ' ';
+ }
+ *(pos-1) = '\0'; /* convert trailing space to null */
+ return merge;
+}
+
+/**
+ Wrapper function around warn for windows native API calls.
+
+ @param action Human understandable description of failed action.
+ @return Value of GetLastError.
+*/
+static DWORD
+warn_last_error (const char* action)
+{
+ DWORD error = GetLastError ();
+
+ if (error) {
+ LPTSTR error_text = 0;
+ if (FormatMessage (FORMAT_MESSAGE_ALLOCATE_BUFFER |
+ FORMAT_MESSAGE_FROM_SYSTEM, NULL, error,
+ MAKELANGID (LANG_NEUTRAL, SUBLANG_DEFAULT),
+ error_text, 0, NULL)) {
+ warn ("%s failed with error %d: %s\n", action, error, error_text);
+ LocalFree (error_text);
+ }
+ else {
+ warn ("%s failed with error %d. Additionally, FormatMessage "
+ "failed with error %d.\n", action, error, GetLastError ());
+ }
+ }
+ return error;
+}
+
+/**
+ Entry point to the child process (watchdog) subsystem.
+
+ This method creates a process using the windows CreateProcess API call.
+ The startup context for this process is based on the context of the exec
+ utility, but with the standard * file handles redirected. The execution of
+ the process is monitored with the WaitForSingleObject API call. If the
+ process doesn't complete within the allowed timeout, it is then killed. On
+ Windows NT systems, a soft kill of the process (via the
+ GenerateConsoleCtrlEvent API call) are first attempted attempted. The
+ process is then hard killed via the TerminateProcess API call.
+
+ @param exec_name name of the child executable
+ @param argv argv array for child process
+ @return structure describing how the child procees exited
+ @see wait_for_child ()
+*/
+struct exec_attrs
+exec_file (char** argv)
+{
+ char* merged;
+ PROCESS_INFORMATION child;
+ OSVERSIONINFO OSVer;
+ STARTUPINFO context;
+ SECURITY_ATTRIBUTES child_sa = /* SA for inheritable handle. */
+ {sizeof (SECURITY_ATTRIBUTES), NULL, TRUE};
+ struct exec_attrs status;
+ const DWORD real_timeout = (timeout > 0) ? timeout * 1000 : INFINITE;
+ DWORD wait_code;
+
+ assert (0 != argv);
+
+ memset (&status, 0, sizeof status);
+
+ OSVer.dwOSVersionInfoSize = sizeof (OSVERSIONINFO);
+ if (0 == GetVersionEx (&OSVer)) {
+ status.status = -1;
+ status.error = warn_last_error ("Retrieving host version");
+ return status;
+ }
+ /* Borrow our startup info */
+ GetStartupInfo (&context);
+ context.dwFlags = STARTF_USESTDHANDLES;
+
+ /* Create I/O handles */
+ {
+ /* Output redirection */
+ char* const tmp_name = output_name (argv [0]);
+
+ context.hStdOutput = CreateFile (tmp_name, GENERIC_WRITE,
+ FILE_SHARE_WRITE, &child_sa, CREATE_ALWAYS,
+ FILE_ATTRIBUTE_NORMAL, NULL);
+ if (INVALID_HANDLE_VALUE == context.hStdOutput) {
+ status.status = -1;
+ status.error = warn_last_error ("Opening child output stream");
+ return status;
+ }
+
+ context.hStdError = context.hStdOutput;
+ free (tmp_name);
+
+ /* Input redirection */
+ context.hStdInput = open_input (target_name, &child_sa);
+ if (INVALID_HANDLE_VALUE == context.hStdInput) {
+ CloseHandle (context.hStdOutput);
+ status.status = -1;
+ status.error = warn_last_error ("Opening child input stream");
+ return status;
+ }
+ }
+
+ merged = merge_argv (argv);
+
+ /* Create the child process */
+ if (0 == CreateProcess (argv [0], merged, 0, 0, 1,
+ CREATE_NEW_PROCESS_GROUP, 0, 0, &context, &child)) {
+ /* record the status if we failed to create the process */
+ status.status = -1;
+ status.error = warn_last_error ("Creating child process");;
+ }
+
+ /* Clean up handles */
+ if (context.hStdInput)
+ if (0 == CloseHandle (context.hStdInput))
+ warn_last_error ("Closing child input stream");
+ if (0 == CloseHandle (context.hStdOutput))
+ warn_last_error ("Closing child output stream");
+
+ /* Clean up argument string*/
+ free (merged);
+
+ /* Return if we failed to create the child process */
+ if (-1 == status.status)
+ return status;
+
+ /* Wait for the child process to terminate */
+ wait_code = WaitForSingleObject (child.hProcess, real_timeout);
+ if (WAIT_TIMEOUT != wait_code) {
+ if (WAIT_OBJECT_0 == wait_code) {
+ if (0 == GetExitCodeProcess (child.hProcess, &status.status)) {
+ warn_last_error ("Retrieving child process exit code");
+ status.status = -1;
+ }
+ status.error = 0;
+ }
+ else {
+ status.status = -1;
+ status.error = warn_last_error ("Waiting for child process");
+ }
+ return status;
+ }
+
+ /* Try to soft kill child process group if it didn't terminate, but only
+ on NT */
+ if (VER_PLATFORM_WIN32_NT == OSVer.dwPlatformId) {
+ if (0 == GenerateConsoleCtrlEvent (CTRL_C_EVENT, child.dwProcessId))
+ warn_last_error ("Sending child process Control-C");
+
+ wait_code = WaitForSingleObject (child.hProcess, 1000);
+ if (WAIT_TIMEOUT != wait_code) {
+ if (WAIT_OBJECT_0 == wait_code) {
+ if (0 == GetExitCodeProcess (child.hProcess,
+ &status.status)) {
+ warn_last_error ("Retrieving child process exit code");
+ status.status = -1;
+ }
+ status.error = 1;
+ }
+ else {
+ status.status = -1;
+ status.error = warn_last_error ("Waiting for child process");
+ }
+ return status;
+ }
+
+ if (0 == GenerateConsoleCtrlEvent (CTRL_BREAK_EVENT,
+ child.dwProcessId))
+ warn_last_error ("Sending child process Control-Break");
+
+ if (WAIT_TIMEOUT != wait_code) {
+ if (WAIT_OBJECT_0 == wait_code) {
+ if (0 == GetExitCodeProcess (child.hProcess,
+ &status.status)) {
+ warn_last_error ("Retrieving child process exit code");
+ status.status = -1;
+ }
+ status.error = 2;
+ }
+ else {
+ status.status = -1;
+ status.error = warn_last_error ("Waiting for child process");
+ }
+ return status;
+ }
+ }
+ /* Then hard kill the child process */
+ if (0 == TerminateProcess (child.hProcess, 3))
+ warn_last_error ("Terminating child process");
+ if (0 == GetExitCodeProcess (child.hProcess, &status.status)) {
+ warn_last_error ("Retrieving child process exit code");
+ status.status = -1;
+ }
+ status.error = 3;
+ return status;
+}
+#endif /* _WIN{32,64} */
Index: cmdopt.cpp
===================================================================
--- cmdopt.cpp (revision 432251)
+++ cmdopt.cpp (working copy)
@@ -35,7 +35,11 @@
#include <stdio.h> /* for *printf, fputs */
#include <stdlib.h> /* for exit */
#include <string.h> /* for str* */
-#include <unistd.h> /* for sleep */
+#if !defined (_WIN32) && !defined (_WIN64)
+# include <unistd.h> /* for sleep */
+#else
+# include <windows.h> /* for Sleep */
+#endif /* _WIN{32,64} */
#include "exec.h"
#include "util.h"
@@ -50,10 +54,17 @@
const char* in_root = ""; /**< Root directory for input/reference files. */
const char* exe_name; /**< Alias for process argv [0]. */
const char* target_name;
+#if !defined (_WIN32) && !defined (_WIN64)
const char escape_code = '\\';
const char default_path_sep = '/';
const char suffix_sep = '.';
const size_t exe_suffix_len = 0;
+#else
+const char escape_code = '^';
+const char default_path_sep = '\\';
+const char suffix_sep = '.';
+const size_t exe_suffix_len = 4; /* strlen(".exe") == 4 */
+#endif
static const char
usage_text[] = {
@@ -88,6 +99,35 @@
" '--option=value' or '--option value'.\n"
};
+#if !defined (_WIN32) && !defined (_WIN64)
+static void
+rw_sleep (int seconds)
+{
+ sleep (seconds);
+}
+
+static int
+rw_signal (int signo, void (*func)(int))
+{
+ struct sigaction act;
+ memset (&act, 0, sizeof act);
+ act.sa_handler = func;
+ return 0 > sigaction (signo, &act, 0);
+}
+#else
+static void
+rw_sleep (int seconds)
+{
+ Sleep (seconds * 1000);
+}
+
+static int
+rw_signal (int signo, void (*func)(int))
+{
+ return SIG_ERR == signal (signo, func);
+}
+#endif
+
/**
Display command line switches for program and terminate.
@@ -308,7 +348,7 @@
if (optarg && *optarg) {
const long nsec = strtol (optarg, &end, 10);
if ('\0' == *end && 0 <= nsec && !errno) {
- sleep (nsec);
+ rw_sleep (nsec);
break;
}
}
@@ -334,11 +374,8 @@
if (optarg && *optarg) {
const long signo = get_signo (optarg);
if (0 <= signo) {
- struct sigaction act;
- memset (&act, 0, sizeof act);
- act.sa_handler = SIG_IGN;
- if (0 > sigaction (signo, &act, 0))
- terminate (1, "sigaction(%s, ...) failed: %s\n",
+ if (rw_signal (signo, SIG_IGN))
+ terminate (1, "rw_signal(%s, ...) failed: %s\n",
get_signame (signo), strerror (errno));
break;
}
Index: exec.h
===================================================================
--- exec.h (revision 432251)
+++ exec.h (working copy)
@@ -28,8 +28,14 @@
#define RW_EXEC_H
struct exec_attrs {
+#if !defined (_WIN32) && !defined (_WIN64)
int status;
int killed;
+#else
+ /* AKA DWORD */
+ unsigned long status;
+ unsigned long error;
+#endif /* _WIN{32,64} */
};
int get_signo (const char* signame);
Index: runall.cpp
===================================================================
--- runall.cpp (revision 432251)
+++ runall.cpp (working copy)
@@ -33,7 +33,9 @@
#include <ctype.h> /* for isspace */
#include <sys/types.h>
#include <sys/stat.h>
-#include <sys/wait.h> /* for WIFEXITED(), ... */
+#if !defined (_WIN32) && !defined (_WIN64)
+# include <sys/wait.h> /* for WIFEXITED(), ... */
+#endif
#include "cmdopt.h"
#include "exec.h"
@@ -44,6 +46,18 @@
# define ENOENT 2
#endif // ENOENT
+#ifndef S_IXUSR
+# define S_IXUSR 0100
+#endif // S_IXUSR
+
+#ifndef S_IXGRP
+# define S_IXGRP 0010
+#endif // S_IXGRP
+
+#ifndef S_IXOTH
+# define S_IXOTH 0001
+#endif // S_IXOTH
+
/**
Utility function to rework the argv array
@@ -231,10 +245,29 @@
return 0;
}
- /* Otherwise, check for the .o file */
+#if !defined (_WIN32) && !defined (_WIN64)
+ /* Otherwise, check for the .o file on non-windows systems */
tmp_name = (char*)RW_MALLOC (path_len + 3);
memcpy (tmp_name, target, path_len + 1);
strcat (tmp_name,".o");
+#else
+ /* Or the target\target.obj file on windows systems*/
+ {
+ size_t target_len = strlen (target_name);
+ size_t tmp_len = path_len + target_len - 2;
+ /* - 2 comes from removing 4 characters (extra .exe) and
+ adding 2 characters (\ directory seperator and trailing
+ null) */
+ tmp_name = (char*)RW_MALLOC (tmp_len);
+ memcpy (tmp_name, target, path_len - 4);
+ tmp_name [path_len - 4] = default_path_sep;
+ memcpy (tmp_name + path_len - 3, target_name, target_len);
+ tmp_name [tmp_len - 4] = 'o';
+ tmp_name [tmp_len - 3] = 'b';
+ tmp_name [tmp_len - 2] = 'j';
+ tmp_name [tmp_len - 2] = '\0';
+ }
+#endif /* _WIN{32,64} */
if (0 > stat (tmp_name, &file_info)) {
if (ENOENT != errno) {
@@ -286,6 +319,7 @@
if (0 == result->status) {
parse_output (target);
}
+#if !defined (_WIN32) && !defined (_WIN64)
else if (WIFEXITED (result->status)) {
const int retcode = WEXITSTATUS (result->status);
switch (retcode) {
@@ -311,6 +345,14 @@
else {
printf ("(%d|%d)\n", result->status, result->killed);
}
+#else
+ else if (-1 == result->status)
+ puts (" I/O");
+ else if (result->error)
+ puts ("KILLED");
+ else
+ printf ("%6d\n", result->status);
+#endif /* _WIN{32,64} */
}
/**
@@ -335,7 +377,11 @@
assert (0 != path);
for (mark = pos = path; '\0' != *pos; ++pos)
+#if !defined (_WIN32) && !defined (_WIN64)
mark = (default_path_sep == *pos) ? pos + 1 : mark;
+#else
+ mark = (default_path_sep == *pos || '/' == *pos) ? pos + 1 : mark;
+#endif /* _WIN{32,64} */
return mark;
}
