incubator-stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Martin Sebor <se...@roguewave.com>
Subject Re: runall.sh replacement
Date Wed, 19 Jul 2006 20:35:36 GMT
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

Mime
View raw message