Responses to each observation can be found following the observations.
Martin Sebor wrote:
Andrew Black wrote:
[...]
Some comments on the program code:
static struct exec_attrs
wait_for_child (const pid_t child_pid)
{
[...]
else if ((pid_t)-1 == wait_pid && EINTR != errno) {
/* waitpid() error */
fprintf (stderr, "waitpid(%d) error: %s\n",
(int)child_pid, strerror (errno));
}
else if (alarm_timeout) {
[...]
fputs ("No more kill options, Bailing", stderr);
We should probably have dedicated codes for these types of errors
(such as WAITPID for the waitpid error and something like TIMEOUT
or NKILL for when we run out of signals to kill the process). We
should not be writing these kinds of messages to stderr or stdout.
I agree that the message produced in the alarm_timeout segment is
redundant (it sets the state that results in the NKILL status message).
This question raises another question. What logic should we use within
this loop? There are a few situations that aren't adequately covered
within the loop as it stands.
One situation is where child.pid == wait.pid, but state.status isn't
WIFEXITED or WIFSIGNALED. We currently do nothing in this situation.
The second situation is when -1 == wait.pid, and EINTR != errno. In
this case, we currently send a message to stderr, and continue.
The third situation is where -1 == wait.pid, EINTR == errno, and
alarm_timeout is 0/false. We currently do nothing in this situation.
The final situation is where wait.pid is neither -1 nor child_pid.
Again, we currently do nothing in this situation.
Of these situations, I believe only the third situation is handled
correctly. I believe this situation would be triggered where the runall
executable is sent a signal that isn't fatal, and it doesn't know how to
handle. In this situation, the correct behavior probably should be to
continue the loop, and wait for the next return from waitpid, as is done
currently.
[...]
static void
replace_file(const int source, const int dest, const char* name)
{
[...]
result = close (source);
if (-1 == result)
terminate ( 1, "closing source for %s redirection failed:
%s\n", name, strerror (errno));
Is this necessary? I.e., does the failure to close the file affect
anything downstream? (If not, we can just issue a warning to stderr
and continue.)
replace_file() is only called from the child process as part of the
setup for the exec() call. I suppose a failure to close an input/output
file wouldn't be fatal, but it would consume a file descriptor which
might be needed by the child process.
/* extern */ struct exec_attrs exec_file (const char* exec_name, char
*const argv[])
{
struct sigaction act;
const pid_t child_pid = fork ();
if (0 == child_pid) { /* child */
[...]
/* Cache stdout for use if execv() fails */
{
const int error_cache = dup (2);
if (-1 == error_cache)
terminate ( 1, "Error duplicating stderr: %s\n",
strerror (errno));
The parent process needs to be able to distinguish between an error
before the call to exec below and one that occurs after (i.e., in the
executed program). It's probably not a good idea for the child to be
writing anything to the standard streams (it could indicate the type
of an error to the parent by writing to a pipe opened by the parent).
Btw., it occurs to me that it might be simpler to construct the argv
array in the child process instead of doing it in the parent so that
we don't have to worry about cleaning up after executing every child
once we implement the per-process options we discussed.
It probably would make sense to revisit how error handling is done in
the child setup, but what we have works for the moment and I suspect it
would be more productive at this time to focus my efforts on other
improvements.
[...]
static void
check_test(const char* exec_name, const char* out_name)
{
[...]
for (tok = fgetc (data); fsm < 6 && !feof (data); tok = fgetc
(data)) {
Wouldn't it be more efficient to read the file one line at a time?
It probably would be. Part of the reason I chose to go the direction I
did was my level of experience with the C library. A concern that could
come up is that line length is theoretically unlimited, and the use of a
fixed length buffer could lead to buffer overflows.
#define FILE_TEST(op, x) \
if (-1==(x)) \
terminate ( 1, op " failed: %s\n", strerror (errno) )
You're still planning to remove/replace this, right? :)
static void
check_example(const char* exec_name, const char* out_name)
{
[...]
/* Todo: diff with --strip-trailing-cr on windows */
execlp("diff", "diff", "-q", ref_name, out_name, (char *)0);
FYI: this could be much more easily done using the system function
but I assume you're planning to implement our own minimal diff
function.
The call to execlp (and the use of the FILE_TEST macro) will need to go
away as part of the windows port. This rework will be necessitated by
the lack of a diff utility on a 'stock' windows install (without any
unix compatibility layer).
[...]
/* extern */ int
main (int argc, /* const */ char *argv[])
{
I would prefer us to keep the logic and size of main to a minimum.
The body of the loop probably deserves its own function, and the
output part another.
This seems logical.
Martin
