stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Martin Sebor <se...@roguewave.com>
Subject Re: string methods thread safety
Date Fri, 21 Jul 2006 00:34:09 GMT
Farid Zaripov wrote:
>  > -----Original Message-----
>  > From: Martin Sebor [mailto:sebor@roguewave.com]
>  > Sent: Friday, July 07, 2006 3:46 AM
>  > To: stdcxx-dev@incubator.apache.org
>  > Subject: Re: string methods thread safety
>  >
>  > Martin Sebor wrote:
> [...]
>  > This last part would be best implemented in the test driver
>  > under some generic interface similar to rw_thread_create()
>  > (e.g., rw_process_create()).
> 
>   Martin, I implemented the functions rw_process_create() and 
> rw_process_join():

Cool, this will be useful!

I would move these two functions to a different header/source file,
say rw_process.h and process.cpp. We can also move rw_system() to
the same files and get rid of system.h and system.cpp.

> 
> --------------------------------------
> typedef long rw_pid_t;
> 
> // returns pid of the created process
> // returns -1 if failed
> // note: argv[0] should be equal to path
> _TEST_EXPORT rw_pid_t
> rw_process_create (const char* path, char* const argv []);

I'm not sure about the second argument. Is it the most convenient
interface? Wouldn't something closer to rw_system() be easier to
use? (If so, you might want to move rw_process_create to the .cpp
file and make it static and call it from the new vararg function).

> 
> // returns pid of the specified process
> // returns -1 if failed
> // result is a pointer to a buffer where the result code
> // of the specified process will be stored, or NULL
> _TEST_EXPORT rw_pid_t
> rw_process_join (rw_pid_t pid, int* result);

Okay, this is close to waitpid() that I would suggest to rename
it to simply rw_waitpid() :)

[...]
> +static rw_pid_t spawnv(int, const char* path, char* const argv [])
> +{
> +    if (rw_pid_t child_pid = fork ())
> +        // the parent process
> +        return child_pid;

Make sure to check child_pid for failure and report it, probably
via a call to rw_error(), before calling exec.

> +
> +    // the child process
> +    execv (path, argv);
> +
> +    // the execvp returns only if an error occurs
> +    rw_fatal (0, __FILE__, __LINE__, "execvp failed: : errno = %{#m} (%{m})");
> +
> +    exit (1);

FYI: rw_fatal() doesn't return so there's no need to call exit().

I'm not sure rw_fatal() is appropriate, though. The test might be
able to continue with other things after a failure. I think I would
go with rw_error() instead. You might also want to add a return
statement to avoid compiler warnings.

(Actually, having said that, since (I assume) this is a Windows
compatibility layer to make it transparent to callers whether
they are calling the Win32 API or not, the rw_error() calls in
response to errors should probably be made in the callers below).

> +}
> +
> +static rw_pid_t cwait(int* status, rw_pid_t pid, int)
> +{
> +    const rw_pid_t res = waitpid (pid, status, 0);
> +
> +    if (res == pid && status)
> +        *status = WEXITSTATUS (*status);
> +
> +    return res;
> +}
> +
> +#endif
> +
> +extern "C" {
> +
> +_TEST_EXPORT rw_pid_t
> +rw_process_create (const char* path, char* const argv[])
> +{
> +    return spawnv (P_NOWAIT, path, argv);

This is where I think all errors should be handled (you might
end up with more #ifdef _WIN32 conditionals if the error codes
are different between Windows and UNIX).

> +}
> +
> +_TEST_EXPORT rw_pid_t
> +rw_process_join (rw_pid_t pid, int* result)
> +{
> +    return cwait (result, pid, WAIT_CHILD);

Same here.

Martin

Mime
View raw message