[PATCH] RE: testsuite process helpers

[Farid Zaripov <Fa...@kyiv.vdiweb.com>]

   Attached is a new patch of process helpers.

 > -----Original Message-----
 > From: Martin Sebor [mailto:sebor@roguewave.com]
 > Sent: Wednesday, August 02, 2006 8:30 PM
 > To: stdcxx-dev@incubator.apache.org
 > Subject: Re: testsuite process helpers [PATCH]
[...]
 >
 > >     (rw_wait_pid): Added the timeout parameter.
 >
 > Excellent! Although I think the UNIX branch should probably
 > try to be more robust about handling an existing alarm (or
 > avoiding the function and using some other mechanism).

   In this patch SIGCHLD signal was using.

 > >     (rw_process_kill): New function to kill the specified process.
 >
 > FYI: sending a process the KILL signal is considered quite
 > drastic since the signal cannot be caught and so the process
 > cannot clean up after itself and release system resources.
 > That's why the default behavior of the POSIX kill utility
 > (i.e., when the -s option is not specified) is to send the
 > TERM signal. I think rw_process_kill should behave similarly.
 >
 > I would suggest to add a second argument like this
 >
 >      rw_process_kill (rw_pid_t, int signo = -1);
 >
 > When (signo == -1) the function should provide the same
 > behavior as the analogous code in wait_for_child() here
 > (i.e., start by sending SIGHUP, then SIGINT, then SIGTERM,
 > and only as the last resort, SIGKILL):
 > http://svn.apache.org/repos/asf/incubator/stdcxx/trunk/util/exec.cpp
 > (The Windows branch of the code can ignore the argument
 > unless there is some similar functionality in Win32).

   Done.

   ChangeLog:
      * rw_process.h (rw_pid_t): The type long changed to _RWSTD_SSIZE_T.
      (rw_wait_pid): Added the timeout parameter.
      (rw_process_kill): New function to terminate the specified process.
      * process.cpp [_WIN32] (__rw_map_errno): New function to get errno
      value from WinAPI last error code
      (__rw_split_cmd): Moved to #ifndef _WIN32/#endif
      [_WIN32] (_rw_vprocess_create): Used CreateProcess instead of
      rw_process_create(char*, char* [])
      [_WIN32] (rw_process_create): Used rw_process_create(char*, ...)
      instead of spawnp
      * 0.process.cpp: New test exercising the rw_process_create(),
      rw_process_kill() and rw_waitpid() functions.


Farid.

Re: [PATCH] RE: testsuite process helpers

[Martin Sebor <se...@roguewave.com>]

Farid Zaripov wrote:
>   Attached is a new patch of process helpers.

Sorry for the long delay in responding. This looks good. Just
a few questions/comments...

[...]
>   ChangeLog:
>      * rw_process.h (rw_pid_t): The type long changed to _RWSTD_SSIZE_T.

This will require all existing users that pass it to printf
to change the printf formatting specifier from %ld to %td.
We should probably have our own a format specifier macro for
rw_pid_t (and any other similar types) along the lines of
C's PRIx macros.

>      (rw_wait_pid): Added the timeout parameter.
>      (rw_process_kill): New function to terminate the specified process.
>      * process.cpp [_WIN32] (__rw_map_errno): New function to get errno
>      value from WinAPI last error code
>      (__rw_split_cmd): Moved to #ifndef _WIN32/#endif
>      [_WIN32] (_rw_vprocess_create): Used CreateProcess instead of
>      rw_process_create(char*, char* [])
>      [_WIN32] (rw_process_create): Used rw_process_create(char*, ...)
>      instead of spawnp
>      * 0.process.cpp: New test exercising the rw_process_create(),
>      rw_process_kill() and rw_waitpid() functions.
> 
> 
> Farid.
> 
> 
> ------------------------------------------------------------------------
> 
[...]
> Index: src/process.cpp
> ===================================================================
[...]
> @@ -51,16 +52,165 @@
>  
>  /**************************************************************************/
>  
> -#if defined (_WIN32) || defined (_WIN64)
> -#  include <process.h>      // for spawnvp(), cwait()
> -#else
> +#ifdef _WIN32
> +
> +#  include <windows.h>      // for WaitForSingleObject(), ...
> +
> +static int
> +_rw_map_errno (DWORD err)
> +{
> +    if (ERROR_WRITE_PROTECT <= err && ERROR_SHARING_BUFFER_EXCEEDED >= err)
> +        return EACCES;
> +
> +    if (ERROR_INVALID_STARTING_CODESEG <= err
> +        && ERROR_INFLOOP_IN_RELOC_CHAIN >= err)
> +    {

A style nit: this should be:

+    if (   ERROR_INVALID_STARTING_CODESEG <= err
+        && ERROR_INFLOOP_IN_RELOC_CHAIN >= err) {


> +        return ENOEXEC;
> +    }
> +
> +    switch (err)
> +    {

...and this:

+    switch (err) {

> +    case ERROR_FILE_NOT_FOUND:
[...]
> +        if ('\'' == *cmd || '\"' == *cmd) {
> +            char* const cmd1 = cmd + 1;
> +            // search the closing quote
> +            if (char* pos = strchr (cmd1, *cmd)) {

Did we decide that it was okay to use this syntax the last time
we discussed it ot or did we say we'd avoid it? (I don't recall
and am too lazy to look it up.)

[...]
> -        if (cmd != end)
> -            // found, replace to '\0'
> -            *cmd++ = '\0';
> +        if (ERROR_INVALID_NAME == err)
> +            errno = ENOENT;
> +        else
> +            errno = _rw_map_errno (err);

Just curious: why is ERROR_INVALID_NAME not handled in _rw_map_errno?

>      }
[...]
> @@ -246,77 +366,194 @@
>  _TEST_EXPORT rw_pid_t
>  rw_process_create (const char* path, char* const argv[])
>  {
> -#if defined (_WIN32) || defined (_WIN64)
> +#if defined (_WIN32)
>  
> -    const rw_pid_t child_pid = spawnvp (P_NOWAIT, path, argv);
> +    return rw_process_create ("\"%s\" %{ As}", path, argv + 1);

Is the space in the %{ As} formatting directive above intended?

>  
[...]
> -#else
> +    if (ERROR_INVALID_HANDLE == err)
> +        errno = ECHILD;
> +    else
> +        errno = _rw_map_errno (err);

Same here.

>  
[...]
> @@ -337,12 +574,90 @@
[...]
> +_TEST_EXPORT int
> +rw_process_kill (rw_pid_t pid, int signo)
[...]
> +        if (ERROR_INVALID_HANDLE == err)
> +            errno = ESRCH;
> +        else if (ERROR_ACCESS_DENIED == err)
> +            errno = EPERM;
> +        else
> +            errno = _rw_map_errno (err);

Same here.

Martin