462178 - Need better error handling when running NSPR tests (runtests.pl)

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P2)

Description

[Christophe Ravel]

As commented in bug 461502, the current error handling in runtests.pl is confusing and sometimes wrong.
- Confusing:
    * Use "exit status" instead of "errno" which has a special meaning
    * Need to report kill signal if any
- Wrong:
    * use "$? >> 8" instead of "$? % 256" to parse exit code

Comment 1

[Christophe Ravel]

Another potential issue I found: the current script does not handle correctly a failure to "exec" a NSPR test program. If the exec fails, the child process goes on and starts a new child with the rest of the list of programs to tests.

Comment 2

[Christophe Ravel]

Attachment: Address the issues listed in description and comment #1

Comment 3

[Wan-Teh Chang]

Comment on attachment 345320 [details] [diff] [review]
Address the issues listed in description and comment #1

r=wtc.

>+    # We should no be here unless exec failed.

"no" => "not"

>-    print_end($prog,$status);
>+    # There is no signal, no core on Windows
>+    print_end($prog,$status, 0, 0);

Nit: add a space after the first comma (,).

Comment 4

[Wan-Teh Chang]

Attachment: Fix typos ("errno" or "error code" should be "exit status")

This patch was reviewed in bug 461502 as attachment 345113 [details] [diff] [review].
I checked it in on the NSPR trunk (NSPR 4.7.3).

Checking in runtests.pl;
/cvsroot/mozilla/nsprpub/pr/tests/runtests.pl,v  <--  runtests.pl
new revision: 1.2; previous revision: 1.1
done

Comment 5

[Christophe Ravel]

The following code:

------
    if ($ltimeout == 0) {
        # we ran all the timeout: it's time to kill the child
        print "Timeout ! Kill child process $lpid\n";
        # Kill the child process and group
        kill(-9,$lpid);
        $lstatus = 1;
    }
------

produces the following output:
------
BEGIN TEST: nameshm1 (2008-09-29 21:27:09)

Timeout ! Kill child process 16335

END TEST: nameshm1 (2008-09-29 21:33:49)
TEST STATUS: nameshm1 = FAILED (exit status 0 - signal 1)
------

"signal 1" should be "signal 9". I will change "$lstatus = 1" to "$lstatus = 9".

In the same way, the following code:
------
    exec("./$lprog");
    # We should not be here unless exec failed.
    print "Faild to exec $lprog";
    exit 1;
------

will produce a "exit status 0 - signal 1". It should be "exit status 1" which is the exit status from the shell when one tries to run a program that doesn't exist.
I will change "exit 1" into "exit 256" (which gives 1 when >>8).

Comment 6

[Christophe Ravel]

Attachment: Addresses comments #3 and #5.

Comment 7

[Wan-Teh Chang]

Comment on attachment 345524 [details] [diff] [review]
Addresses comments #3 and #5.

r=wtc.

>     # Start test program    
>     exec("./$lprog");
>+    # We should not be here unless exec failed.
>+    print "Faild to exec $lprog";
>+    exit 256;

It seems strange that "exit 1" in Perl doesn't cause the Perl
process to exit with status 1.  I'm not familiar with Perl, so
I'll trust you on this.

I suggest that you write "exit 256" as
    exit (1 << 8);
to make it clear that when we do "$status >> 8" later, we'll get
1.

>         print "Timeout ! Kill child process $lpid\n";
>         # Kill the child process and group
>         kill(-9,$lpid);
>-        $lstatus = 1;
>+        $lstatus = 9;

Ideally, we should call $ret = waitpid($lpid,0) again after
kill(-9,$lpid) to reap the test program and get the real exit
status.  For now, your code is fine.

Comment 8

[Christophe Ravel]

Attachment: Patch as checked in

Addresses comment #7. r+=wtc

Checking in runtests.pl;
/cvsroot/mozilla/nsprpub/pr/tests/runtests.pl,v  <--  runtests.pl
new revision: 1.3; previous revision: 1.2