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

[Cezar Andrei]

Fix for bug 1086323.

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.

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

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

Using the exit codes convention.

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

Also fix xqdoc.

Unmerged revisions

=== 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 17:32:23 +0000
@@ -56,6 +56,11 @@
  : @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: (-1000) - termination signal code. If process is stoped:
+ : (-2000) - stop signal code or -1999 othewise.
+ : 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 +80,11 @@
  : @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: (-1000) - termination signal code. If process is stoped:
+ : (-2000) - stop signal code or -1999 othewise.
+ : 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 17:32:23 +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