You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@stdcxx.apache.org by Andrew Black <ab...@roguewave.com> on 2006/07/15 01:19:49 UTC
runall.sh replacement
Greetings all.
As part of a project to automate testing of the stdcxx library using our
internal build system, I have been working on a replacement for the
runall.sh script.
This script produces the output when 'gmake run_all' is executed from
within the example/test/bin directories.
This executable is written in c, and it should be possible to compile it
using roughly the same set of flags as the library. Note that there
will be a few warnings, but I believe them to be harmless. (I have been
using the following as my compile line)
> gcc -W -Wall -Wcast-qual -Winline -Wshadow -Wwrite-strings
-Wcast-align --pedantic runall.c -o runall
One major failing of this executable at this time is that it likely
won't work natively on windows. Another major failing is that it isn't
written to kill grandchild processes. These are flaws shared by the
runall.sh script.
A final failing is that it doesn't color the output, and doesn't show
the watchdog countdown sequence. I hope to correct all these failings
at some point in the future.
This code has been lightly tested, and as such should be used at your
own risk.
--Andrew Black
Re: runall.sh replacement
Posted by Andrew Black <ab...@roguewave.com>.
Greetings Martin
Attached is a second try, this time without the non-formatting changes
--Andrew Black
Martin Sebor wrote:
> Andrew Black wrote:
>> Greetings Martin
>>
>> Attached is a patch that tries to address the formatting issues
>> remaining. I'm still not certain I've caught everything, but it's a
>> starting point.
>
> Thanks! This is better but a) I think we should fix all the issues
> before moving on, and b) as a matter of policy, we should fix only
> formatting issues and nothing else in the patch. Could you please
> resubmit just the formatting changes alone? A few more comments
> below.
>
> [...]
>> @@ -235,14 +237,14 @@
>> size_t i;
>> static char def[16];
>>
>> - for (i = 0; i != sizeof names / sizeof *names; ++i) {
>> + for (i = 0; i < sizeof (names) / sizeof (*names); ++i) {
>
> FYI: the parentheses are unnecessary here. The sizeof operator
> requires parentheses around types but not around objects.
>
> [...]
>> @@ -548,29 +550,29 @@
>>
>> /* Set process group ID (so entire group can be killed)*/
>> {
>> - const pid_t pgroup = setsid();
>> - if ( getpid() != pgroup )
>> - terminate ( 1, "Error setting process group: %s\n",
>> - strerror (errno));
>> + const pid_t pgroup = setsid ();
>> + if ( getpid () != pgroup )
>
> This should be:
>
> if (getpid () != pgroup)
>
>
>> + terminate (1, "Error setting process group: %s\n",
>> + strerror (errno));
> [...]
>> @@ -587,23 +589,23 @@
>> S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
>>
>> if (-1 == intermit)
>> - terminate( 1, "Error opening %s for output
>> redirection: "
>> - "%s\n", tmp_name, strerror (errno));
>> + terminate(1, "Error opening %s for output
>> redirection: "
>> + "%s\n", tmp_name, strerror (errno));
>
> The above is missing a space before the paren and has an extra
> space after the comma. Btw., it's easy to write an Emacs regular
> expression to correct this type of fomatting:
>
> search for: "\([a-z_A-Z0-9]\)("
>
> and replace it with: "\1 ("
>
> You might want to consider doing something similar for the extra
> space after the comma and the missing space before an opening
> brace, since those two seem to be a recurring problem as well.
>
>>
>> - replace_file(intermit, 1, "stdout");
>> + replace_file (intermit, 1, "stdout");
>>
>> free (tmp_name);
>> }
>>
>> /* Redirect stderr */
>> - if (-1 == dup2(1, 2))
>> - terminate ( 1, "Redirection of stderr to stdout failed:
>> %s\n", - strerror (errno));
>> + if (-1 == dup2 (1, 2))
>> + terminate (1, "Redirection of stderr to stdout failed:
>> %s\n",
>
> Extra space after the comma above :)
>
> [...]
>> @@ -611,11 +613,11 @@
>> if (-1 == child_pid){
>
> Missing space before the closing brace. It's easy to fix, too:
>
> replace: "\([^ ]\){"
> with: "\1 {"
>
> [...]
>> --- output.cpp (revision 425396)
>> +++ output.cpp (working copy)
>> @@ -69,46 +69,36 @@
>> Parsed results
>>
> [...]
>> case '#':
>> - fsm = ( 1 == fsm ) ? 2 : 0;
>> + fsm = (1 == fsm) ? 2 : 0;
>
> FYI: There is no need to parenthesize the subexpressions in the
> conditional expression.
>
>> break;
>> case ' ':
>> - fsm = ( 2 == fsm ) ? 3 : ( ( 4 == fsm ) ? 5 : 0 );
>> + fsm = (2 == fsm) ? 3 : ((4 == fsm) ? 5 : 0);
>
> Same here. Personally, I find the whole expression more readable
> without the parentheses (YMMV):
>
> fsm = 2 == fsm ? 3 : 4 == fsm ? 5 : 0;
>
> [...]
>> /**
>> @@ -159,30 +147,19 @@
>> this method and check_test() is how it parses the results. This
>> version
>> parses compatability layer output, rather than the test driver
>> output.
>>
>> - @param exec_name the name of the executable that generated the
>> output file
>> - @param out_name the name of the output file being parsed
>> + @param data pointer to file structure for output file being parsed
>> @see check_test()
>> */
>> static void
>> -check_compat_test (const char* out_name)
>> +check_compat_test (FILE* data)
>
> FYI: it's a good policy to avoid making any other type of changes
> when adjusting formatting. That way you reduce the likelihood of
> introducing regressions and having to wade through hundreds of
> lines of irrelevant and innocuous changes just to find it (or
> worse yet, making someone else do it).
>
> [...]
>> - assert ( 0 != out_name );
>> + assert ( 0 != data );
>
> Extra spaces after the opening and before the closing parentheses.
>
>>
> [...]
>> @@ -272,7 +232,7 @@
>> struct stat file_info;
>> const size_t root_len = strlen(in_root);
>> char* const ref_name = (char*)RW_MALLOC(root_len
>> - + strlen(exec_name) + 19);
>> + + strlen(exec_name) + 19);
>
> Missing spaces before the opening parentheses above.
>
> [...]
>> @@ -210,7 +207,7 @@
>> assert (0 != path);
>>
>> for (mark = pos = path; '\0' != *(pos); ++pos)
>> - mark = ( '/' == *(pos) || '\\' == *(pos) ) ? pos+1 : mark;
>> + mark = ('/' == *(pos) || '\\' == *(pos)) ? pos + 1 : mark;
>
> FYI: there is no need to parenthesize the name of a variable before
> dereferencing it. I.e., the above is the same as the (IMO) more
> readable equivalent:
>
> mark = '/' == *pos || '\\' == *pos ? pos + 1 : mark;
>
> Martin
Re: runall.sh replacement
Posted by Martin Sebor <se...@roguewave.com>.
Andrew Black wrote:
> Greetings Martin
>
> Attached is a patch that tries to address the formatting issues
> remaining. I'm still not certain I've caught everything, but it's a
> starting point.
Thanks! This is better but a) I think we should fix all the issues
before moving on, and b) as a matter of policy, we should fix only
formatting issues and nothing else in the patch. Could you please
resubmit just the formatting changes alone? A few more comments
below.
[...]
> @@ -235,14 +237,14 @@
> size_t i;
> static char def[16];
>
> - for (i = 0; i != sizeof names / sizeof *names; ++i) {
> + for (i = 0; i < sizeof (names) / sizeof (*names); ++i) {
FYI: the parentheses are unnecessary here. The sizeof operator
requires parentheses around types but not around objects.
[...]
> @@ -548,29 +550,29 @@
>
> /* Set process group ID (so entire group can be killed)*/
> {
> - const pid_t pgroup = setsid();
> - if ( getpid() != pgroup )
> - terminate ( 1, "Error setting process group: %s\n",
> - strerror (errno));
> + const pid_t pgroup = setsid ();
> + if ( getpid () != pgroup )
This should be:
if (getpid () != pgroup)
> + terminate (1, "Error setting process group: %s\n",
> + strerror (errno));
[...]
> @@ -587,23 +589,23 @@
> S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
>
> if (-1 == intermit)
> - terminate( 1, "Error opening %s for output redirection: "
> - "%s\n", tmp_name, strerror (errno));
> + terminate(1, "Error opening %s for output redirection: "
> + "%s\n", tmp_name, strerror (errno));
The above is missing a space before the paren and has an extra
space after the comma. Btw., it's easy to write an Emacs regular
expression to correct this type of fomatting:
search for: "\([a-z_A-Z0-9]\)("
and replace it with: "\1 ("
You might want to consider doing something similar for the extra
space after the comma and the missing space before an opening
brace, since those two seem to be a recurring problem as well.
>
> - replace_file(intermit, 1, "stdout");
> + replace_file (intermit, 1, "stdout");
>
> free (tmp_name);
> }
>
> /* Redirect stderr */
> - if (-1 == dup2(1, 2))
> - terminate ( 1, "Redirection of stderr to stdout failed: %s\n",
> - strerror (errno));
> + if (-1 == dup2 (1, 2))
> + terminate (1, "Redirection of stderr to stdout failed: %s\n",
Extra space after the comma above :)
[...]
> @@ -611,11 +613,11 @@
> if (-1 == child_pid){
Missing space before the closing brace. It's easy to fix, too:
replace: "\([^ ]\){"
with: "\1 {"
[...]
> --- output.cpp (revision 425396)
> +++ output.cpp (working copy)
> @@ -69,46 +69,36 @@
> Parsed results
>
[...]
> case '#':
> - fsm = ( 1 == fsm ) ? 2 : 0;
> + fsm = (1 == fsm) ? 2 : 0;
FYI: There is no need to parenthesize the subexpressions in the
conditional expression.
> break;
> case ' ':
> - fsm = ( 2 == fsm ) ? 3 : ( ( 4 == fsm ) ? 5 : 0 );
> + fsm = (2 == fsm) ? 3 : ((4 == fsm) ? 5 : 0);
Same here. Personally, I find the whole expression more readable
without the parentheses (YMMV):
fsm = 2 == fsm ? 3 : 4 == fsm ? 5 : 0;
[...]
> /**
> @@ -159,30 +147,19 @@
> this method and check_test() is how it parses the results. This version
> parses compatability layer output, rather than the test driver output.
>
> - @param exec_name the name of the executable that generated the output file
> - @param out_name the name of the output file being parsed
> + @param data pointer to file structure for output file being parsed
> @see check_test()
> */
> static void
> -check_compat_test (const char* out_name)
> +check_compat_test (FILE* data)
FYI: it's a good policy to avoid making any other type of changes
when adjusting formatting. That way you reduce the likelihood of
introducing regressions and having to wade through hundreds of
lines of irrelevant and innocuous changes just to find it (or
worse yet, making someone else do it).
[...]
> - assert ( 0 != out_name );
> + assert ( 0 != data );
Extra spaces after the opening and before the closing parentheses.
>
[...]
> @@ -272,7 +232,7 @@
> struct stat file_info;
> const size_t root_len = strlen(in_root);
> char* const ref_name = (char*)RW_MALLOC(root_len
> - + strlen(exec_name) + 19);
> + + strlen(exec_name) + 19);
Missing spaces before the opening parentheses above.
[...]
> @@ -210,7 +207,7 @@
> assert (0 != path);
>
> for (mark = pos = path; '\0' != *(pos); ++pos)
> - mark = ( '/' == *(pos) || '\\' == *(pos) ) ? pos+1 : mark;
> + mark = ('/' == *(pos) || '\\' == *(pos)) ? pos + 1 : mark;
FYI: there is no need to parenthesize the name of a variable before
dereferencing it. I.e., the above is the same as the (IMO) more
readable equivalent:
mark = '/' == *pos || '\\' == *pos ? pos + 1 : mark;
Martin
Re: runall.sh replacement
Posted by Andrew Black <ab...@roguewave.com>.
Greetings Martin
Attached is a patch that tries to address the formatting issues
remaining. I'm still not certain I've caught everything, but it's a
starting point.
--Andrew Black
Log:
2006-07-25 Andrew Black <ab...@roguewave.com>
* cmdopt.cpp: Formatting cleanup.
* exec.cpp: Same.
* output.cpp: Same.
* runall.cpp: Same.
* util.cpp: Same.
Martin Sebor wrote:
> Andrew Black wrote:
>> Revised patch attached.
>>
>> One change that I made while working on the self-test logic was to
>> split the output parsing logic into parse_output.cpp/h. I suspect
>> you'll have a better name for the file. That was bundled into this
>> patch as I didn't take the time to back those changes out.
>
> Okay, I made a number of changes and committed everything here:
> http://svn.apache.org/viewvc?rev=425242&view=rev
>
> One important change was removing the non-portable "-q" option
> from the invocation of diff (it was causing problems on Solaris).
>
> I would still like to see a patch addressing the formatting issues
> I pointed out initially (http://tinyurl.com/mmqgv), preferably
> before any other changes to the utility.
>
> Thanks!
> Martin
Re: runall.sh replacement
Posted by Martin Sebor <se...@roguewave.com>.
Martin Sebor wrote:
> Andrew Black wrote:
>
>> Revised patch attached.
>>
>> One change that I made while working on the self-test logic was to
>> split the output parsing logic into parse_output.cpp/h. I suspect
>> you'll have a better name for the file. That was bundled into this
>> patch as I didn't take the time to back those changes out.
>
>
> Okay, I made a number of changes and committed everything here:
> http://svn.apache.org/viewvc?rev=425242&view=rev
>
> One important change was removing the non-portable "-q" option
> from the invocation of diff (it was causing problems on Solaris).
Another change I forgot to mention was removing the (mostly)
redundant const qualifiers from function arguments (e.g.,
const int). The cv-qualifiers in function declarations are
ignored (i.e., they are not part of the function's type).
That said, they are respected in the definition of function
so that might be something to think about. For now, though,
the convention has been not to use const on function arguments
so let's stick to it until it's changed.
Martin
Re: runall.sh replacement
Posted by Martin Sebor <se...@roguewave.com>.
Andrew Black wrote:
> Revised patch attached.
>
> One change that I made while working on the self-test logic was to split
> the output parsing logic into parse_output.cpp/h. I suspect you'll have
> a better name for the file. That was bundled into this patch as I
> didn't take the time to back those changes out.
Okay, I made a number of changes and committed everything here:
http://svn.apache.org/viewvc?rev=425242&view=rev
One important change was removing the non-portable "-q" option
from the invocation of diff (it was causing problems on Solaris).
I would still like to see a patch addressing the formatting issues
I pointed out initially (http://tinyurl.com/mmqgv), preferably
before any other changes to the utility.
Thanks!
Martin
Re: runall.sh replacement
Posted by Andrew Black <ab...@roguewave.com>.
Revised patch attached.
One change that I made while working on the self-test logic was to split
the output parsing logic into parse_output.cpp/h. I suspect you'll have
a better name for the file. That was bundled into this patch as I
didn't take the time to back those changes out.
--Andrew Black
Martin Sebor wrote:
> Andrew Black wrote:
>> Greetings all
>>
>> Attached is a slightly modified version of the patch from yesterday.
>> Changes from the previous version:
>> - Compatibility layer support (via --compat/--nocompat switches)
>> - Handle failure to fork()
>>
>> I suspect that some changes may be required before submission, but I
>> also consider this to be in a mostly usable state.
> [...]
>
> Very good! We do need to make a few changes. One is making sure it's
> (also) valid C++ (since it's now being compiled with C++ compilers).
> I suspect gcc might let it slide but sigaction() is a C interface
> that takes a C (i.e., extern "C") function, so handle_alrm() needs
> to be decorated with extern "C" in C++ (i.e., when __cplusplus is
> #defined).
>
> The other change I'd like to make is to the names of some of the
> files :) Here's what I'm thinking:
>
> watchdog.{cpp,h} --> exec.{cpp,h}
> parse_opts.{cpp,h} --> cmdopt.{cpp,h}
> util.{cpp,h} --> ???
>
> I'm not too fond of files named util etc. because they end up being
> collections of all kinds of odds and ends that don't seem to fit
> anywhere else, and make a convenient excuse not thinking about the
> logical organization of a program.
>
> We have two functions in util.cpp: terminate() (which should be
> renamed to something that better expresses the purpose of the
> function -- i.e., to issue a diagnostic *and* then [maybe] exit),
> and rw_basename(), the replacement for the UNIX basename().
>
> rw_basename() is only called from runall.c, so it might as well
> be defined (static) there.
>
> terminate() could go in its own file, say diag.{cpp,h}, after
> being renamed to something like issue_diag(). The whole program
> should use issue_diag() for all types of diagnostic messages,
> i.e, for both errors and warnings (with the distinction being
> that the latter don't cause the program to exit with a non-zero
> status but allow it to continue).
>
> There are other tweaks that I'd like to make to the code but those
> can be made after the commit. The file name changes should be made
> prior to it.
>
> Could you make the naming changes and resubmit the patch? Also,
> in your ChangeLog, please avoid using wildcards instead of file
> names and instead spell out the name of each file in full.
>
> Thanks!
> Martin
Tuscany/stdcxx at OSCON (was: Re: runall.sh replacement)
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Edward Slattery wrote:
> Hi, is anyone going to OSCON this week? There are some Tuscany folks there
> (cc Geoff) - it would be good to make contact and talk about using stdcxx
> as the STL for Tuscany.
This sounds great! Just as a reminder, be sure to fill in stdcxx-dev@
(or stdcxx-private@ but only if truly confidential) with a summary.
It's important that project related ideas and their disposition land
back on the list to keep all of the participants/ppmc members in the
loop. Wish I was there - too many conferences this summer.
Bill
Tuscany/stdcxx at OSCON (was: Re: runall.sh replacement)
Posted by Martin Sebor <se...@roguewave.com>.
Edward Slattery wrote:
> Hi, is anyone going to OSCON this week? There are some Tuscany folks there
> (cc Geoff) - it would be good to make contact and talk about using stdcxx
> as the STL for Tuscany.
Unfortunately, I'm not going to be there but I believe Patrick
is planning on attending. I agree that it would be good to get
together and discuss.
Martin
>
> cheers,
> Ed.
>
> Ed Slattery
> e-BIT( e-Business Integration Technologies) MP146
> IBM Hursley Park, Winchester, SO21 2JN, England
> Telephone: +44 196 2816483 (external) 246483 (internal)
> Email: Edward Slattery/UK/IBM @ IBMGB, slattery@uk.ibm.com
>
Re: runall.sh replacement
Posted by Edward Slattery <SL...@uk.ibm.com>.
Hi, is anyone going to OSCON this week? There are some Tuscany folks there
(cc Geoff) - it would be good to make contact and talk about using stdcxx
as the STL for Tuscany.
cheers,
Ed.
Ed Slattery
e-BIT( e-Business Integration Technologies) MP146
IBM Hursley Park, Winchester, SO21 2JN, England
Telephone: +44 196 2816483 (external) 246483 (internal)
Email: Edward Slattery/UK/IBM @ IBMGB, slattery@uk.ibm.com
Re: runall.sh replacement
Posted by Martin Sebor <se...@roguewave.com>.
Andrew Black wrote:
> Greetings all
>
> Attached is a slightly modified version of the patch from yesterday.
> Changes from the previous version:
> - Compatibility layer support (via --compat/--nocompat switches)
> - Handle failure to fork()
>
> I suspect that some changes may be required before submission, but I
> also consider this to be in a mostly usable state.
[...]
Very good! We do need to make a few changes. One is making sure it's
(also) valid C++ (since it's now being compiled with C++ compilers).
I suspect gcc might let it slide but sigaction() is a C interface
that takes a C (i.e., extern "C") function, so handle_alrm() needs
to be decorated with extern "C" in C++ (i.e., when __cplusplus is
#defined).
The other change I'd like to make is to the names of some of the
files :) Here's what I'm thinking:
watchdog.{cpp,h} --> exec.{cpp,h}
parse_opts.{cpp,h} --> cmdopt.{cpp,h}
util.{cpp,h} --> ???
I'm not too fond of files named util etc. because they end up being
collections of all kinds of odds and ends that don't seem to fit
anywhere else, and make a convenient excuse not thinking about the
logical organization of a program.
We have two functions in util.cpp: terminate() (which should be
renamed to something that better expresses the purpose of the
function -- i.e., to issue a diagnostic *and* then [maybe] exit),
and rw_basename(), the replacement for the UNIX basename().
rw_basename() is only called from runall.c, so it might as well
be defined (static) there.
terminate() could go in its own file, say diag.{cpp,h}, after
being renamed to something like issue_diag(). The whole program
should use issue_diag() for all types of diagnostic messages,
i.e, for both errors and warnings (with the distinction being
that the latter don't cause the program to exit with a non-zero
status but allow it to continue).
There are other tweaks that I'd like to make to the code but those
can be made after the commit. The file name changes should be made
prior to it.
Could you make the naming changes and resubmit the patch? Also,
in your ChangeLog, please avoid using wildcards instead of file
names and instead spell out the name of each file in full.
Thanks!
Martin
Re: runall.sh replacement
Posted by Andrew Black <ab...@roguewave.com>.
Greetings all
Attached is a slightly modified version of the patch from yesterday.
Changes from the previous version:
- Compatibility layer support (via --compat/--nocompat switches)
- Handle failure to fork()
I suspect that some changes may be required before submission, but I
also consider this to be in a mostly usable state. A pair of useful
features that the older runall.sh script has that are currently lacking
in this replacement are the coloring of the output and an incremental
progress display while a test/example is running. I hope to add both
these features at some point in the future.
--Andrew Black
Log:
2006-07-21 Andrew Black <ab...@roguewave.com>
* GNUmakefile (BINDIR): Add and use convenience variable referencing
the bin subdirectory of the buildspace
* GNUmakefile.exm (RUNFLAGS): Change command line switch used to
specify input/output file directory to match switch expected by runutil
utility
* GNUmakefile.bin (runutil): Add rule to build the new runutil utility
* GNUmakefile.tst (RUNFLAGS): remove additional switches as unneeded
* makefile.rules (run runall run_all): Add dependency on runutil utility
* GNUmakefile.exm, GNUmakefile.tst: Add rule to build the runutil utility
* run_locale_utils.sh: Change default output file to /dev/stdout, add
test driver style assertion count summary
* *.cpp, *.h: Add source files for C reimplementation of the runall.sh
shell script.
Andrew Black wrote:
> Greetings all
>
> Attached is a diff against subversion r424097, integrating the runall.sh
> replacement script into the makefile infrastructure.
>
> I don't think this version is ready for submission into subversion, as
> there are multiple issues with it. One issue is that the executable
> builds and links as a C++ utility (like the locale and localedef
> utilities), though it is written in what I believe to be pure C. Another
> issue is that there are several (corner) cases in the watchdog subsystem
> (watchdog.cpp) that need to be sorted out.
>
> I have tried to document all the .cpp files with JavaDoc style Doxygen
> comments.
>
> --Andrew Black
Re: runall.sh replacement
Posted by Andrew Black <ab...@roguewave.com>.
Greetings all
Attached is a diff against subversion r424097, integrating the runall.sh
replacement script into the makefile infrastructure.
I don't think this version is ready for submission into subversion, as
there are multiple issues with it. One issue is that the executable
builds and links as a C++ utility (like the locale and localedef
utilities), though it is written in what I believe to be pure C.
Another issue is that there are several (corner) cases in the watchdog
subsystem (watchdog.cpp) that need to be sorted out.
I have tried to document all the .cpp files with JavaDoc style Doxygen
comments.
--Andrew Black
Re: runall.sh replacement
Posted by Andrew Black <ab...@roguewave.com>.
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
Re: runall.sh replacement
Posted by Martin Sebor <se...@roguewave.com>.
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
Re: runall.sh replacement
Posted by Andrew Black <ab...@roguewave.com>.
Greetings Martin
The cause of the problems being experienced with the examples is that
the name of the flag used to specify the input/output directory is -d in
this version of the script, rather than -x (as used by runall.sh).
Similarly, the name of the flag used to specify command line options in
this version is -x, rather than -X (as used by runall.sh).
If you want to get a list of known command line options, you can pass
the script -h or -? (No support for --help yet)
--Andrew Black
Martin Sebor wrote:
> Andrew Black wrote:
>> Greetings
>>
>> Attached is the second version of the replacement runall script. In
>> this version, I have tried to address the concerns raised by Martin in
>> the previous review of the source.
>>
>> One important bug fix made was to resolve the SEGV issue that was
>> encountered on Solaris. The cause was an incorrect termination of the
>> argv array used in the call to execv().
>
> Running tests seems to work fine but most examples are reported
> with the FORMAT code and those that read from stdin are killed
> with SIGHUP. Are the examples supposed to be handled correctly
> in this version?
>
> To make it easier to debug these kinds of issues I think it might
> be handy to add a verbose option and print out the command line
> (i.e., the argv vector) used to invoke each program including any
> redirection (using shell notation). I.e., invoking
> ./runall -v -x"--foo --bar" ./a.out ./b.out ./c.out
> should produce something like
>
> ./a.out --foo --bar </dev/null >a.output 2>&1
> ./b.out --foo --bar </dev/null >b.output 2>&1
> status=1
> ./c.out --foo --bar </dev/null >c.output 2>&1
> status=134 (SIGABRT, core dumped)
>
> after a successful invocation of ./a.out, ./b.out having exited
> with a status of 1, and ./c.out having exited with SIGABRT and
> producing a core dump. (Additional output could be added for
> failed assertions, diffs, etc.)
>
> Martin
Re: runall.sh replacement
Posted by Martin Sebor <se...@roguewave.com>.
Andrew Black wrote:
> Greetings
>
> Attached is the second version of the replacement runall script. In
> this version, I have tried to address the concerns raised by Martin in
> the previous review of the source.
>
> One important bug fix made was to resolve the SEGV issue that was
> encountered on Solaris. The cause was an incorrect termination of the
> argv array used in the call to execv().
Running tests seems to work fine but most examples are reported
with the FORMAT code and those that read from stdin are killed
with SIGHUP. Are the examples supposed to be handled correctly
in this version?
To make it easier to debug these kinds of issues I think it might
be handy to add a verbose option and print out the command line
(i.e., the argv vector) used to invoke each program including any
redirection (using shell notation). I.e., invoking
./runall -v -x"--foo --bar" ./a.out ./b.out ./c.out
should produce something like
./a.out --foo --bar </dev/null >a.output 2>&1
./b.out --foo --bar </dev/null >b.output 2>&1
status=1
./c.out --foo --bar </dev/null >c.output 2>&1
status=134 (SIGABRT, core dumped)
after a successful invocation of ./a.out, ./b.out having exited
with a status of 1, and ./c.out having exited with SIGABRT and
producing a core dump. (Additional output could be added for
failed assertions, diffs, etc.)
Martin
Re: runall.sh replacement
Posted by Andrew Black <ab...@roguewave.com>.
Greetings
Attached is the second version of the replacement runall script. In
this version, I have tried to address the concerns raised by Martin in
the previous review of the source.
One important bug fix made was to resolve the SEGV issue that was
encountered on Solaris. The cause was an incorrect termination of the
argv array used in the call to execv().
There are a fair number of directions this project could go from here.
A few of them are as follows
* Add support for killing grandchild processes
* Add logic to makefiles to compile this script and use it rather than
the runall.sh script
* Split into multiple source files to make code more manageable (current
length is ~1000 lines)
* Add self-test mode to verify operations
* Add support for compilation/execution on windows
- Add support for windows process management
- remove dependency on the diff utility
* Refine output analysis logic (see STDCXX-261)
* Test on additional platforms (including AIX, HPUX, IRIX, Tru64)
--Andrew Black
Re: runall.sh replacement
Posted by Martin Sebor <se...@roguewave.com>.
Andrew Black wrote:
[...]
> I have been working on a replacement for the
> runall.sh script.
I get the following result when I run it on Solaris 9 (note that
I don't have '.' in my PATH -- I believe the program assume that
and doesn't work otherwise; I think I fixed it in my copy but I
still get the same result):
$ ./runall ./runall
NAME STATUS ASSRTS FAILED PERCNT
runall SIGSEGV
A few high-level comments:
1. Please make sure the code is const-correct. It's much easier
to follow that way. It's good to also declare const any and all
local variables (other than function arguments) that aren't
modified by the function.
2. As far as formatting goes, please try to follow the informal
(and still unwritten) stdcxx guidelines, in particular
-- opening brace is in column 1 for file-scope functions and
structs, but it follows the declared name in local scope
-- closing brace is always alone on a line
-- separate opening brace from whatever precedes it by one
space
-- use a single space between operators and operands
-- separate function names from the opening parenthesis by
a single space (ditto for for, if, switch, and while
statements)
-- each statement should be on a line of its own, including
the body of the if statement:
if (condition)
return -1; // okay
if (condition) return -1; // NOT okay
3. Other suggestions:
-- prefer functions to macros (e.g., define a single helper
function instead of the ALLOC_TEST and FILE_TEST macros)
-- avoid complicated expressions or modifying variables when
passing arguments to function calls (e.g., avoid things
like
foo (a = b, bar ())
and instead write
a = b;
c = bar ();
foo (a, c);
to avoid being bitten by function-like macros, and for
better readability)
-- always check the return value of malloc() and handle null
pointers (by issuing an error message and exiting with a
non-zero status)
-- define a single function to report errors and (optionally)
exit the program (with the appropriate exit status) instead
of calling exit() from multiple places
-- in diagnostic messages use text understandable to users no
familiar with the implementation of the program (e.g., prefer
"failed to open file: %s" to "open(%s) failed")
-- use the name of the program (i.e., argv[0]) in diagnostic
messages (to make it possible to identify the source of
the diagnostic in pipelines such as foo | bar)
-- prefer the C standard library calls to POSIX calls (i.e.,
prefer fopen() to open())
[...]
> This executable is written in c, and it should be possible to compile it
> using roughly the same set of flags as the library. Note that there
> will be a few warnings, but I believe them to be harmless. (I have been
> using the following as my compile line)
>
> > gcc -W -Wall -Wcast-qual -Winline -Wshadow -Wwrite-strings
> -Wcast-align --pedantic runall.c -o runall
These are good, although -Winline won't do anything unless you
also enable optimization.
Martin