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.
[...]
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.)
/* 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.
[...]
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?
#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.
[...]
/* 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.
Martin
