Mailing-List: contact stdcxx-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: stdcxx-dev@incubator.apache.org
Received-SPF: neutral (asf.osuosl.org: local policy)
Message-ID: <44DBDEF7.9030301@roguewave.com>
Date: Thu, 10 Aug 2006 19:35:51 -0600
From: Martin Sebor <sebor@roguewave.com>
Organization: Rogue Wave Software
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.12) Gecko/20050920
MIME-Version: 1.0
To: stdcxx-dev@incubator.apache.org
Subject: Re: [patch] exec utility windows port
References: <44DA6A90.50006@roguewave.com> <44DA7D4D.7080100@roguewave.com>
 <44DB9766.4030004@roguewave.com>
In-Reply-To: <44DB9766.4030004@roguewave.com>
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit

Andrew Black wrote:
> Greetings
> 
> Attached is a reworked version of the patch with the requested changes 
> included.  Unfortunately, the change log has changed enough that a new 
> one is required.

That's okay, we don't mind :)

> If it is decided that the windows file structure is to 
> be altered (with regards to the location of object files), this patch 
> will need to be altered slightly.
> 
> One that was asked was 'Why can't we use backslash as the escape 
> character on Windows?'  My answer is that the character is used as a 
> path separator,

Ugh.

> and it would be impossible to disambiguate the 
> conflicting uses.  I use ^ as the escape character on windows, as that's 
> the escape character used elsewhere in windows.
> 
> Another question that was asked was about the block I #if 0 ed out. This 
> change was intentional.  The block in question is/was used to check 
> compile only targets, but we don't have any of those at this time.  IF 
> this check were to be kept, it would need platform specific logic to 
> handle the differences in suffixes for object files between windows and 
> unix.

Okay. Thanks for adding the comment.

> 
> --Andrew Black
> 
> Log:

Wow! Capitalized first letter in every sentence and all. I'm so
proud of you! ;-)

>     * util.h (rw_snprintf): Add macro hack to work around lack of 
> snprintf definition on windows.
>     * util.h (reference_name, output_name): Declare functions to 
> generate the names for reference and output files respectively.
>     * util.cpp (reference_name, output_name): Define above.
>     * cmdopt.h (escape_code, default_path_sep, suffix_len): Declare 
> platform specific constants related to the file system.
>     * cmdopt.cpp (escape_code, default_path_sep, suffix_len): Define 
> above, conditional on platform.
>     * cmdopt.cpp: Alter #includes to include windows.h on windows in 
> place of unistd.h.
>     * cmdopt.cpp (rw_sleep, rw_signal): Define platform abstraction 
> functions for sleep and signal/sigaction, conditional on platform.
>     * cmdopt.cpp (eval_options): use use rw_sleep() in place of sleep(), 
> rw_signal() in place of sigaction().
>     * cmdopt.cpp (split_opt_string): use escape_code as escape character 
> in place of '\'.
>     * exec.h (exec_attrs): Alter structure for windows builds.
>     * exec.cpp: Alter #includes to include windows.h and process.h on 
> windows in place of unistd.h and sys/wait.h
>     * exec.cpp (get_signame): Use rw_snprintf in place of snprintf.
>     * exec.cpp (handle_alrm, wait_for_child, open_input, replace_file, 
> exec_file): Compile (existing) versions only on non-windows platforms.
>     * exec.cpp (open_input, exec_file): Alter (existing) version to use 
> reference_name, output_name respectively.
>     * exec.cpp (open_input, merge_argv, exec_file): Define new functions 
> using windows native API, which will compile only on windows platforms.
>     * output.cpp (check_example): Use reference_name to determine 
> reference file location.
>     * runall.cpp (S_IXUSR, S_IXGRP, S_IXOTH): Define if not defined for 
> windows.
>     * runall.cpp (check_target_ok): Disable (unused) logic for output 
> only targets.  Alter compile check on windows systems to correctly 
> locate .obj file.
>     * runall.cpp (process_results): Handle windows result codes correctly.
>     * runall.cpp (rw_basename): Use default_path_sep as separator for 
> non-windows builds.
> 
> 
> ------------------------------------------------------------------------
> 
> Index: util.h
> ===================================================================
> --- util.h	(revision 430088)
> +++ util.h	(working copy)
> @@ -28,6 +28,15 @@
>  #define RW_UTIL_H
>  
>  /**
> +   Ugly workaround for windows non-definition of snprintf

snprintf() is a C95 thing and we would be better off not relying
on it. AFAICS, rw_snprintf() is only being used in exec.cpp (so
the macro shouldn't be defined in a header) and it should be both
easy and safe to replace it with sprintf.

> +*/
> +#if !defined (_WIN32) && !defined (_WIN64)
> +#  define rw_snprintf snprintf
> +#else
> +#  define rw_snprintf _snprintf
> +#endif
> +
> +/**
>     Generates a non-terminal error message on stderr.
>  
>     @param format printf () format string to display on stderr
> @@ -52,4 +61,24 @@
>  void* guarded_realloc (void* source, const size_t size, 
>                         const char* const file, const unsigned line);
>  
> +/**
> +   Generates the name of a reference (input/output) file, based on dir and mode.
> +
> +   This function allocates memory which is to be freed by the caller.
> +
> +   @param dir example subdirectory to reference
> +   @param mode type of file to generate name for (should be 'in' or 'out')
> +   @return translation of 'in_root/dir/mode/target_name.mode'
> +*/
> +char* reference_name(const char* dir, const char* mode);

Could you please try to more consistently follow the formatting
conventions I outlined in http://tinyurl.com/mmqgv (specifically
2. bullet 5 :) Thanks!

> +
> +/**
> +   Generates the name of the output file for the executable target.
> +
> +   This function allocates memory which is to be freed by the caller.
> +
> +   @param path of target to generate output name for
> +   @return translation of 'target.out'
> +*/
> +char* output_name(const char* target);

Same here: missing space before the opening parenthesis.

>  #endif   // RW_UTIL_H
> Index: exec.cpp
> ===================================================================
> --- exec.cpp	(revision 430088)
> +++ exec.cpp	(working copy)
> @@ -37,10 +37,15 @@
[...]
>  #include <sys/stat.h> /* for S_* */

FWIW, comments like the one above aren't terribly helpful. I would
suggest to name the first in a series or related symbols and append
an ellipsis to indicate that other related symbols from the header
might be needed.

[...]
> @@ -350,10 +355,11 @@
>      }
>  
>      /* We've run out of known signal numbers, so use a default name */
> -    snprintf (def, sizeof def, "SIG#%d", signo);
> +    rw_snprintf (def, sizeof def, "SIG#%d", signo);

Given a large enough buffer we don't need to worry about overflow
here since we know the maximum length of the string.

[..]
> @@ -727,3 +721,216 @@
[...]
> +static HANDLE
> +open_input (const char* exec_name, SECURITY_ATTRIBUTES* child_sa)

Do we actually use these security attributes for anything? I didn't
look very carefully but it seems to me that we're just passing the
thing around w/o ever using it for anything. Or did I miss something?

[...]
> +struct exec_attrs 
> +exec_file (char** argv)
> +{
> +    char* merged = merge_argv(argv);
[...]
> +        context.hStdOutput = CreateFile (tmp_name, GENERIC_WRITE, 
> +                FILE_SHARE_WRITE, &child_sa, CREATE_ALWAYS, 
> +                FILE_ATTRIBUTE_NORMAL, NULL);
> +        if (INVALID_HANDLE_VALUE == context.hStdOutput)
> +        { 

The brace goes on the line above (here and everywhere else).

> +            status.status = -1;
> +            status.error = GetLastError ();
> +            return status;

This looks like a memory leak -- the merged pointer is not freed
before returning.

> +        }
> +
> +        context.hStdError = context.hStdOutput;
> +        free (tmp_name);
> +
> +        /* Input redirection */
> +        context.hStdInput = open_input (target_name, &child_sa);
> +        if (INVALID_HANDLE_VALUE == context.hStdInput)
> +        { 
> +            status.status = -1;
> +            status.error = GetLastError ();
> +            return status;

Same here.

> +        }
> +    }    
> +
> +    /* Create the child process */
> +    CreateProcess (argv[0], merged, 0, 0, 1, 
> +        DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP, 0, 0, &context, &child);

Should we be handling errors from CreateProcess?

> +
> +    /* Clean up handles */
> +    CloseHandle(context.hStdInput);
> +    CloseHandle(context.hStdOutput);

And issue warnings when these fail? (We should if we do it on the
UNIX side.)

> +
> +    /* Clean up argument string*/
> +    free (merged);
> +
> +    /* Wait for the child process to terminate */
> +    if(WAIT_OBJECT_0 == WaitForSingleObject (child.hProcess, (timeout > 0) ? timeout * 1000 : INFINITE)) {

I would suggest to break out the conditional expression into a separate
statement and reducing the length of the line below 79 characters for
readability.

> +        GetExitCodeProcess (child.hProcess, &status.status);
> +        status.error = 0;
> +        return status;
> +    }    
> +
> +    {/* Try to soft kill child process group if it didn't terminate, but only on NT */
> +        OSVERSIONINFO OSVer;
> +        OSVer.dwOSVersionInfoSize = sizeof (OSVERSIONINFO);
> +        GetVersionEx (&OSVer);
> +        if(VER_PLATFORM_WIN32_NT == OSVer.dwPlatformId) {

Space after if please (same below).

> +            GenerateConsoleCtrlEvent (CTRL_C_EVENT, child.dwProcessId);
> +            if(WAIT_OBJECT_0 == WaitForSingleObject (child.hProcess, 1000)) {
> +                GetExitCodeProcess (child.hProcess, &status.status);
> +                status.error = 1;
> +                return status;
> +            }
> +            GenerateConsoleCtrlEvent (CTRL_BREAK_EVENT, child.dwProcessId);
> +            if(WAIT_OBJECT_0 == WaitForSingleObject (child.hProcess, 1000)) {
> +                GetExitCodeProcess (child.hProcess, &status.status);
> +                status.error = 2;
> +                return status;
> +            }
> +        }
> +    }
> +    /* Then hard kill the child process */
> +    TerminateProcess (child.hProcess, 3);

Check if TerminateProcess succeeded?

[...]
> Index: cmdopt.cpp
[...]
> @@ -84,6 +88,43 @@
>      "  '--option=value' or '--option value'.\n"
>  };
>  
> +#if !defined (_WIN32) && !defined (_WIN64)
> +const char escape_code = '\\';
> +const char default_path_sep = '//';

Is it safe to assume that the path separator is always one char
long? (It might be, but I'm not sure. It seems the code would
be more robust if we avoided making the assumption.)

[...]
> Index: output.cpp
> ===================================================================
> --- output.cpp	(revision 430088)
> +++ output.cpp	(working copy)
> @@ -233,8 +233,7 @@
>  {
>      struct stat file_info;
>      const size_t root_len = strlen (in_root);
> -    char* const ref_name = (char*)RW_MALLOC (root_len 
> -                                             + strlen (target_name) + 19);
> +    char* ref_name;
>      FILE* reference;
>  
>      assert (0 != in_root);
> @@ -243,10 +242,7 @@
>      assert (0 != output);
>  
>      /* Try in_root/manual/out/target_name.out */
> -    memcpy (ref_name, in_root, root_len+1);
> -    strcat (ref_name, "/manual/out/");
> -    strcat (ref_name, target_name);
> -    strcat (ref_name, ".out");
> +    ref_name = reference_name("manual", "out");
>  
>      if (0 > stat (ref_name, &file_info)) {

FYI: apparently, there is no native stat on z/OS (see STDCXX-265:
http://issues.apache.org/jira/browse/STDCXX-265). We need to start
thinking about abstracting this to its own function that can be
implemented in terms of, say, fopen() on z/OS.

[...]
> +char*
> +reference_name(const char* dir, const char* mode)
> +{
> +    const size_t root_len = strlen (in_root);
> +    const size_t cmp_len = strlen (target_name) - exe_suffix_len;
> +    const size_t dir_len = strlen (dir);
> +    const size_t mode_len = strlen (mode);

We should assert the preconditions of the function before we make
any assumptions about them.

> +    char* const ref_name = (char*)RW_MALLOC (root_len + cmp_len + dir_len + 
> +                                             mode_len * 2 + 5);

I suggest to precompute the size of the array and saving it in
a local variable rather than computing it in the argument.

[...]
> +    *(tail++) = default_path_sep;

There is no need to parenthesize *(tail++): the expression has the
same meaning without the parentheses and it's so well-established
that using the parentheses will only be confusing.

> +    memcpy (tail , dir, dir_len);
> +    tail += dir_len;
> +    *(tail++) = default_path_sep;
> +    memcpy (tail , mode, mode_len);
> +    tail += mode_len;
> +    *(tail++) = default_path_sep;
> +    memcpy (tail , target_name, cmp_len);
> +    tail += cmp_len;
> +    *(tail++) = '.';

I think z/OS uses a period as the directory separator so if that's
true we should be prepared to avoid using it here...

[...]
> +char*
> +output_name(const char* target)
> +{
> +    const char* suffix = ".out";

...as well as here. Again, check with Scott Zhong, our z/OS guru,
about this. (This is just a to-do item and shouldn't hold up your
patch).

[...]
> Index: exec.h
> ===================================================================
> --- exec.h	(revision 430088)
> +++ exec.h	(working copy)
> @@ -28,8 +28,14 @@
>  #define RW_EXEC_H
>  
>  struct exec_attrs {
> +#if !defined (_WIN32) && !defined (_WIN64)
>      int status;
>      int killed;
> +#else
> +    /* AKA DWORD */

Won't defining status to a different type on different platforms
cause problems (e.g., warnings)? I would suggest to keep them
the same, especially given that they are the same size.

> +    unsigned long status;
> +    unsigned long error;
> +#endif  /* _WIN{32,64} */
>  };
>  
>  int get_signo (const char* signame);
[...]
> Index: runall.cpp
[...]
> @@ -281,6 +314,7 @@
>      if (0 == result->status) {
>          parse_output (target);
>      } 
> +#if !defined (_WIN32) && !defined (_WIN64)

I wonder if replacing the above with

     #if defined (WIFEXITED)

would make the code more robust.

[...]
> @@ -329,9 +371,13 @@
>  
>      assert (0 != path);
>  
> -    for (mark = pos = path; '\0' != *pos; ++pos)
> +    for (mark = pos = path; '\0' != *pos; ++pos) {
> +#if !defined (_WIN32) && !defined (_WIN64)
> +        mark = (default_path_sep == *pos) ? pos + 1 : mark;
> +#else
>          mark = ('/' == *pos || '\\' == *pos) ? pos + 1 : mark;

Should we be checking for equality to path_sep here instead of
hardcoding backslash (I know they are the same but just for
consistency).

Martin