stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Black <abl...@roguewave.com>
Subject Re: runall.sh replacement
Date Wed, 19 Jul 2006 21:59:58 GMT
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

Mime
View raw message