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