Merge lp:~zorba-coders/zorba/bug1086323-processExitCode into lp:zorba/process-module

[Cezar Andrei]

Fix for bug 1086323, align Fedora and Ubuntu when process module executes a command that crashes.

Fix for bug 1086323.

[Zorba Build Bot (zorba-buildbot)]

Validation queue starting for merge proposal.
Log at: http://zorbatest.lambda.nu:8080/remotequeue/bug1086323-processExitCode-2012-12-04T19-47-40.632Z/log.html

[Paul J. Lucas (paul-lucas)]

1. Why return made-up numbers, e.g., -1000, etc? Why not simply return the actual exit status? Of course this implies that you'd have to implement XQuery functions that do the equivalent of the W* macros. Either that, or return XML data, e.g.:

  <process id="1234" raw-exit-code="0" status="normal"/>

or:

  <process id="1234" raw-exit-code="56" status="signal" signal="5" dumped-core="true"/>

2. When you throw the USER_EXCEPTION, why not include the actual OS-level error message? There is code in src/util/error_util.h. Yes, this implies that this code would have to be moved to the public API.

review: Needs Fixing

[Zorba Build Bot (zorba-buildbot)]

The attempt to merge lp:~zorba-coders/zorba/bug1086323-processExitCode into lp:zorba/process-module failed. Below is the output from the failed tests.

CMake Error at /home/ceej/zo/testing/zorbatest/tester/TarmacLander.cmake:275 (message):
  Validation queue job bug1086323-processExitCode-2012-12-04T19-47-40.632Z is
  finished. The final status was:

  4 tests did not succeed - changes not commited.

Error in read script: /home/ceej/zo/testing/zorbatest/tester/TarmacLander.cmake

[Chris Hillery (ceejatec)]

I assume using made-up exit codes was so the new information could fit into the existing result schema, for backwards compatibility. Still, I think it probably would be better to improve the schema and bump the module version number. If there is any way for the results to be the same on posix and Windows that'd be ideal.

Also, I would like there to be a test case for this, including invoking a program that segfaults and getting a result. I'm not sure if this could be done cross-platform but it'd be good to try.

[Cezar Andrei (cezar-andrei)]

Interesting Ubuntu and Fedora don't implement this the same way.

On Ubuntu running a bash script or the process xq script which calls a program that segFaults, the exit code is 139. The macro WIFEXITED(stat) returns true and WEXITSTATUS(stat) returns 139. Which btw means acording to the link below an 128+signal 11.
http://tldp.org/LDP/abs/html/exitcodes.html#EXITCODESREF

On Fedora running the same process implementation, the macro WIFEXITED(stat) returns false and WIFSIGNALED(stat) returns true which means we can call WTERMSIG(stat) which returns 11.

So in order to bring these two together I propose make them both return 139, just like shell because this process module executes a shell command not just an executable and I think we need to respect the same conventions. This means that for the Fedora case just add 128 and use it as exit code.

On Windows I couldn't find something similar, if possible we could follow the same convention.

Comments?

[Chris Hillery (ceejatec)]

Why does the process module execute a shell command? I would expect it to behave like execl(), not exec() (which is not part of POSIX as far as I can tell).

Oh, yeah, ugh - I see that process.cpp collects the (separate) command-line arguments from the XQuery functions and concatenates them together with spaces. That's broken, IMHO. It uses double-quotes, but that won't work for arguments that contain double-quotes.

I'm not sure how to do all this on Windows, although I'm confident there's a better way. But on Unix this implementation is simply not correct. That probably explains to some degree why the behaviour on Fedora is weird - it's not actually getting the return value of the executed command, but of some intermediate shell which is evaluating that command.

review: Needs Fixing

[Cezar Andrei (cezar-andrei)]

it's executing a shell with the commands, this way one can have access to shell commands.

[Chris Hillery (ceejatec)]

It's my opinion that that is not a valuable feature, and the downsides outweigh its usefulness. Also, users can do this themselves if they require it by executing a shell or cmd. We should not do it for them, or at the very least this should not be the default.

[Cezar Andrei (cezar-andrei)]

Using the usual exit codes convention, as per our todays' talk.

[Zorba Build Bot (zorba-buildbot)]

Validation queue starting for merge proposal.
Log at: http://zorbatest.lambda.nu:8080/remotequeue/bug1086323-processExitCode-2012-12-05T17-36-43.652Z/log.html

[Zorba Build Bot (zorba-buildbot)]

The attempt to merge lp:~zorba-coders/zorba/bug1086323-processExitCode into lp:zorba/process-module failed. Below is the output from the failed tests.

CMake Error at /home/ceej/zo/testing/zorbatest/tester/TarmacLander.cmake:275 (message):
  Validation queue job bug1086323-processExitCode-2012-12-05T17-36-43.652Z is
  finished. The final status was:

  4 tests did not succeed - changes not commited.

Error in read script: /home/ceej/zo/testing/zorbatest/tester/TarmacLander.cmake

[Zorba Build Bot (zorba-buildbot)]

Validation queue starting for merge proposal.
Log at: http://zorbatest.lambda.nu:8080/remotequeue/bug1086323-processExitCode-2012-12-05T18-10-35.842Z/log.html

[Chris Hillery (ceejatec)]

The module comments still list the -2000 / -1000 return value numbers.

review: Needs Fixing

[Chris Hillery (ceejatec)]

Paul (not sure if you were on the conf call today) : we decided that we should fix just this problem in the process module for now, to allow it to consistently report exit status across Linux distributions and hopefully MacOS. As for the other issues you and I raised, I will file new bugs which will be rolled into a v2 of the process module sometime after Zorba 2.8 is shippped.

It would be great if you could test this on MacOS, and vote Approve if there are no other issues.

[Paul J. Lucas (paul-lucas)]

> It would be great if you could test this on MacOS, and vote Approve if there
> are no other issues.

I could test it if there were a test I could run.

[Zorba Build Bot (zorba-buildbot)]

Validation queue job bug1086323-processExitCode-2012-12-05T18-10-35.842Z is finished. The final status was:

All tests succeeded!

[Zorba Build Bot (zorba-buildbot)]

Voting does not meet specified criteria. Required: Approve > 1, Disapprove < 1, Needs Fixing < 1, Pending < 1. Got: 1 Needs Fixing, 2 Pending.

[Sorin Marian Nasoi (sorin.marian.nasoi)]

I have checked on Fedora and these are the returned codes:

- if a test ends in a SEG FAULT => exit-code = 139
- if a test reaches an ASSERT => exit code = 134

So I approve this proposal but at least someone on Ubuntu (ideally also on MacOS) should confirm that they get the same exact codes.

review: Approve

[lp:~zorba-coders/zorba/bug1086323-processExitCode]

35. By Cezar Andrei <email address hidden> on 2012-12-05

Also fix xqdoc.

[Cezar Andrei (cezar-andrei)]

Fixed xqdoc.

Also I tested it on Ubuntu, segfault comes with exit code 139.

To test it one needs to call process:exec on a program that ends with a segmentation fault.

[Chris Hillery (ceejatec)]

Verified (with Sorin's repro script in email) that this works on Ubuntu.

review: Approve

[Paul J. Lucas (paul-lucas)]

On Mac OS X 10.8.2, it prints:

139 0

[Paul J. Lucas (paul-lucas)]

After discussion with Chris, I'm approving this despite it not working on Mac OS X since the short-term goal is to have this working on Linux where having it working on Mac OS X is a nice-to-have.

The test that was supposed to SEGFAULT did; the test that was supposed to have an assert() failure didn't.

review: Approve

[Paul J. Lucas (paul-lucas)]

Follow-up: after running a new query to call a C program:

#include <stdlib.h>
int main() {
  abort();
}

it prints 134. Hence, the process-module fix works but the HOF code behaves differently on Mac OS X.

[Zorba Build Bot (zorba-buildbot)]

Validation queue starting for merge proposal.
Log at: http://zorbatest.lambda.nu:8080/remotequeue/bug1086323-processExitCode-2012-12-06T01-48-38.697Z/log.html

[Zorba Build Bot (zorba-buildbot)]

Validation queue job bug1086323-processExitCode-2012-12-06T01-48-38.697Z is finished. The final status was:

All tests succeeded!

=== modified file 'src/com/zorba-xquery/www/modules/process.xq'
--- src/com/zorba-xquery/www/modules/process.xq	2012-09-27 16:53:40 +0000
+++ src/com/zorba-xquery/www/modules/process.xq	2012-12-05 19:16:20 +0000
@@ -56,6 +56,10 @@
  : @return the result of the execution as an element as
  :         shown in the documentation of this module. The exit-code
  :         element returns the exit code of the child process.
+ : For POSIX compliant platforms: returns the process exit code. If process is 
+ : terminated or stopped: 128 + termination signal code. 
+ : For Windows platforms: returns the return value of the process or the exit 
+ : or terminate process specified value.
  :
  : @error process:PROC01 if an error occurred while communicating 
  :   with the executed process.
@@ -75,6 +79,10 @@
  : @return the result of the execution as an element as
  :         shown in the documentation of this module. The exit-code
  :         element returns the exit code of the child process.
+ : For POSIX compliant platforms: returns the process exit code. If process is 
+ : terminated or stopped: 128 + termination signal code. 
+ : For Windows platforms: returns the return value of the process or the exit 
+ : or terminate process specified value.
  :
  : @error process:PROC01 if an error occurred while communicating 
  :   with the executed process.

=== modified file 'src/com/zorba-xquery/www/modules/process.xq.src/process.cpp'
--- src/com/zorba-xquery/www/modules/process.xq.src/process.cpp	2012-07-21 01:09:37 +0000
+++ src/com/zorba-xquery/www/modules/process.xq.src/process.cpp	2012-12-05 19:16:20 +0000
@@ -257,7 +257,7 @@
   };
   
   //start child process
-  BOOL ok=create_child_process(lStdOut,lStdErr,aCommand,lChildProcessInfo);
+  BOOL ok = create_child_process(lStdOut,lStdErr,aCommand,lChildProcessInfo);
   if(ok==TRUE)
   {
 
@@ -413,8 +413,8 @@
   std::ostringstream lStderr;
 
 #ifdef WIN32
-  std::string lCommandLineString=lTmp.str();
-  int code = run_process(lCommandLineString,lStdout,lStderr);
+  std::string lCommandLineString = lTmp.str();
+  int code = run_process(lCommandLineString, lStdout, lStderr);
   
   if (code != 0)
   {
@@ -424,6 +424,7 @@
       "http://www.zorba-xquery.com/modules/process", "PROC01");
     throw USER_EXCEPTION(lQName, lErrorMsg.str().c_str());
   }
+  exit_code = code;
 
 #else //not WIN32
 
@@ -470,36 +471,39 @@
 
     int  stat = 0;
     
-    waitpid(pid, &stat, 0);
-    /*pid_t w;
-    do 
-    {
-      w = waitpid(pid, &stat, WUNTRACED | WCONTINUED);
-      if (w == -1) 
-      { 
-          perror("waitpid"); 
-          exit(EXIT_FAILURE); 
-      }
+    pid_t w = waitpid(pid, &stat, 0);
+    
+    if (w == -1) 
+    { 
+        std::stringstream lErrorMsg;
+        lErrorMsg << "Failed to wait for child process ";
+        Item lQName = ProcessModule::getItemFactory()->createQName(
+          "http://www.zorba-xquery.com/modules/process", "PROC01");
+        throw USER_EXCEPTION(lQName, lErrorMsg.str().c_str());          
+    }
 
-      if (WIFEXITED(stat)) 
-      {
-          printf("exited, status=%d\n", WEXITSTATUS(stat));
-      } 
-      else if (WIFSIGNALED(stat)) 
-      {
-          printf("killed by signal %d\n", WTERMSIG(stat));
-      }
-      else if (WIFSTOPPED(stat)) 
-      {
-          printf("stopped by signal %d\n", WSTOPSIG(stat));
-      } 
-      else if (WIFCONTINUED(stat)) 
-      {
-          printf("continued\n");
-      }
-    } while (!WIFEXITED(stat) && !WIFSIGNALED(stat));
-    */
-    exit_code = WEXITSTATUS(stat);
+    if (WIFEXITED(stat)) 
+    {
+        //std::cout << " WEXITSTATUS : " << WEXITSTATUS(stat) << std::endl; std::cout.flush();
+        exit_code = WEXITSTATUS(stat);
+    } 
+    else if (WIFSIGNALED(stat)) 
+    {
+        //std::cout << " WTERMSIG : " << WTERMSIG(stat) << std::endl; std::cout.flush();
+        exit_code = 128 + WTERMSIG(stat);
+    }
+    else if (WIFSTOPPED(stat)) 
+    {
+        //std::cout << " STOPSIG : " << WSTOPSIG(stat) << std::endl; std::cout.flush();
+        exit_code = 128 + WSTOPSIG(stat);
+    }
+    else
+    {
+        //std::cout << " else : " << std::endl; std::cout.flush();
+        exit_code = 255;
+    }
+    
+    //std::cout << " exit_code : " << exit_code << std::endl; std::cout.flush();
 
   }
 #endif // WIN32