stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Martin Sebor <se...@roguewave.com>
Subject Re: [PATCH] global cleanup for exec utility
Date Fri, 29 Sep 2006 01:16:54 GMT
Andrew Black wrote:

[...]

Cool, lots of good changes. A few comments...

> --- exec.cpp	(revision 450090)
> +++ exec.cpp	(working copy)
[...]
> @@ -448,14 +448,27 @@
>          alarm (timeout);
>  
>      while (1) {
> -        const pid_t wait_pid = waitpid (child_pid, &state.status, waitopts);
> +        const pid_t wait_pid = waitpid (child_pid, &status, waitopts);
>          if (child_pid == wait_pid) {
> -            if (WIFEXITED (state.status) || WIFSIGNALED (state.status))
> +            if (WIFEXITED (status)) {
> +                result->exit = WEXITSTATUS (status);
> +                switch (result->exit) {
> +                case 126:
> +                    result->status = ST_EXIST;
> +                    break;
> +                case 127:
> +                    result->status = ST_EXECUTE;
> +                    break;
> +                }
>                  break; /*we've got an exit state, so let's bail*/
> -            else if (WIFSTOPPED (state.status))
> -                stopped = state.status;
> +            } else if (WIFSIGNALED (status)) {

The else should go on the next line. Nothing but the semicolon
or a comment should follow a closing bracket.

> +                result->exit = WTERMSIG (status);
> +                result->signaled = 1;
> +                break; /*we've got an exit state, so let's bail*/
> +            } else if (WIFSTOPPED (status))

Same here.

[...]
> @@ -502,7 +516,7 @@
>                  }
>  
>                  /* Record the signal used*/
> -                state.killed = signals [siginx];
> +                /* result->killed = signals [siginx];*/

Is this correct? (If it is, the comment is wrong. It should say
why the next line is commented out.)

[...]
> --- cmdopt.h	(revision 450090)
> +++ cmdopt.h	(working copy)
> @@ -27,76 +27,33 @@
[...]
> +const char* const get_target ();

The second const is redundant and causes problems, remember?

> --- runall.cpp	(revision 450090)
> +++ runall.cpp	(working copy)
[...]
> @@ -439,10 +389,17 @@
>  int
>  main (int argc, char *argv [])
>  {
> +    struct target_opts target_template;
> +    char* exe_opts = "";

This is an illegal assignment (const char* to non-const char*).

> +
>      exe_name = argv [0];
> +    memset (&target_template, 0, sizeof target_template);
>  
> +    target_template.timeout = 10;
> +    target_template.data_dir = "";

Same here.

> @@ -457,22 +414,30 @@
[...]
> +    if (target_template.core) free (target_template.core);
> +    if (target_template.cpu) free (target_template.cpu);
> +    if (target_template.data) free (target_template.data);
> +    if (target_template.fsize) free (target_template.fsize);
> +    if (target_template.nofile) free (target_template.nofile);
> +    if (target_template.stack) free (target_template.stack);
> +    if (target_template.as) free (target_template.as);

There's no need to check the pointers before calling free,
the function can handle null pointers just fine (also the
style is bad).

[...]
> /************************************************************************
>  *
>  * cmdopt.h - Interface declaration for the option parsing subsystem

This doesn't look right for target.h...

[...]
> #ifndef RW_TARGET_H
> #define RW_TARGET_H
[...]
> struct target_opts {
>     char** argv; /**< Target argv array. */
>     unsigned timeout; /**< Allowed time for process execution. */
>     char* data_dir; /**< Root directory for reference input/output files. */

This member should be const.

I corrected the const problems and got rid of the unnecessary
if's before the calls to free() and committed the whole thing
here: http://svn.apache.org/viewvc?view=rev&rev=451079

Please take a look at the commented out code on line 520 of
exec.cpp and either fix it or correct the comment. Also, please
submit a patch fixing the formatting and other problems I pointed
out here or any other transgressions against the style.

Thanks
Martin

Mime
View raw message