Paul Burba wrote:
> Philip Martin <philip@codematters.co.uk> wrote on 03/14/2006 02:31:37 PM:
>
>>I'd suggest putting SVN_UTF_ETOU_XLATE_HANDLE into a libsvn_subr
>>header, it doesn't need to be visible outside libsvn_subr.
>
> Thanks, I understand now. Though for approach #3, both
> SVN_UTF_UTOE_XLATE_HANDLE and SVN_UTF_ETOU_XLATE_HANDLE are now required
> by libsvn_repos. So I placed these in svn_utf.h, is this the correct
> approach?
I think Philip means they need not be part of our public API so they should go
in a private header that is local to libsvn_subr. I haven't looked to see if
there is a suitable header already. I don't know the purpose of these "xlate"
handles, so I can't comment on whether they ought to be public.
> This patch implements option 3. I'm not sure if anyone noticed this or
> not, but the previous patch didn't handle stdin to a hook. This was an
> oversight on my part, the attached patch handles this now.
I hadn't noticed. I was waiting to get the basic approach sorted out before
actually reviewing the patch at an implementation level.
> [[[
> OS400/EBCDIC Port:
>
> This is one of several patches to allow Subversion to run on IBM's
> OS400 V5R4. It provides a workaround for various limitations with
> IBM's implementation of APR processes.
>
> * subversion/include/svn_utf.h
> (SVN_UTF_UTOE_XLATE_HANDLE, SVN_UTF_ETOU_XLATE_HANDLE): New public
> xlate keys for EBCDIC <--> UTF-8 conversions.
>
> * subversion/libsvn_repos/hooks.c
> Include spawn.h and fcntl.h
> (run_hook_cmd): New "APR-free" implementation for OS400.
>
> * subversion/libsvn_subr/cmdline.c
> (SVN_UTF_ETOU_XLATE_HANDLE): Remove.
>
> * subversion/libsvn_subr/io.c
> (SVN_UTF_UTOE_XLATE_HANDLE): Remove.
> ]]]
> Index: subversion/libsvn_repos/hooks.c
> ===================================================================
> --- subversion/libsvn_repos/hooks.c (revision 18908)
> +++ subversion/libsvn_repos/hooks.c (working copy)
> @@ -22,6 +22,11 @@
> #include <apr_pools.h>
> #include <apr_file_io.h>
>
> +#ifdef AS400
> +#include <spawn.h> /* For spawn() and struct inheritance. */
> +#include <fcntl.h> /* for open() and oflag parms. */
Style note: Please omit the "for ..." comments, since they always become out of
date and it's relatively easy to find out what facilities a particular header
is providing if anyone wants to know.
> +#endif
> +
> #include "svn_error.h"
> #include "svn_path.h"
> #include "svn_repos.h"
> @@ -52,6 +57,7 @@
> svn_boolean_t read_errstream,
> apr_file_t *stdin_handle,
> apr_pool_t *pool)
> +#ifndef AS400
> {
> apr_file_t *read_errhandle, *write_errhandle, *null_handle;
> apr_status_t apr_err;
> @@ -164,8 +170,215 @@
>
> return err;
> }
> +#else /* Run hooks with spawn() on OS400. */
> +{
> + const char* script_stderr_utf8;
(Style: "char *script...".)
> + const char **native_args = NULL;
> + char buffer[20];
> +#pragma convert(0)
> + /* Even with the UTF support in V5R4 a few functions don't support utf-8
> + * args, including spawn(), which takes this char array. */
> + char *xmp_envp[2] = {"QIBM_USE_DESCRIPTOR_STDIO=Y", NULL};
> +#pragma convert(1208)
> + int rc, fd_map[3], ignoreFds[2], useFds[2], stdinFds[2], exitcode;
> + svn_stringbuf_t *script_output = svn_stringbuf_create("", pool);
> + pid_t child_pid, wait_rv;
> + apr_size_t args_arr_size = 0, i = 0;
> + struct inheritance xmp_inherit = {0};
>
> + /* Find number of elements in args array. */
> + while (args[args_arr_size] != NULL)
> + args_arr_size++;
>
> + /* Allocate memory for the native_args string array plus one for
> + * the ending null element. */
> + native_args = apr_palloc(pool, sizeof(char *) * args_arr_size + 1);
> +
> + /* Convert utf-8 args to ebcdic for use by spawn(). */
> + while(args[i] != NULL)
> + {
> + SVN_ERR(svn_utf_cstring_from_utf8_ex((const char**)(&(native_args[i])),
> + args[i++], (const char *)0,
Move the "i++" to a separate statement (perhaps using a "for" loop would be
neatest), since the order of evaluation of arguments is undefined. (That is, a
compiler can perform "args[i++]" before "native_args[i]" and thus get the wrong
native_arg.)
> + SVN_UTF_UTOE_XLATE_HANDLE,
> + pool));
> + }
> +
> + /* Make the last element in the array a NULL pointer as required
> + * by spawn. */
> + native_args[args_arr_size] = NULL;
> +
> + /* Get two data pipes, allowing stdout and stderr to be separate. */
> + if (pipe(ignoreFds) != 0 || pipe(useFds) != 0 )
> + {
> + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + "Error piping hook script %s.", cmd);
In all of these error messages, put the hook script's name in (single)
quotation marks: "... script '%s'", to make it clear that it is being quoted
and is not part of the syntax of the sentence. (Otherwise, it could be
confusing if the script's name is, say, some English word like "error".)
Also, you probably want to mark either all of them for translation (with
"_(...)") or none of them.
> + }
> +
> +
> + /* Get a pipe for stdin if needed and map it. */
> + if (stdin_handle)
> + {
> + /* This is a bit cumbersome, but spawn can't work with apr_file_t, so
> + * if there is stdin for the script we open another pipe to the
> + * script's stdin which we can later write what we read from
> + * stdin_handle. Ugh! */
> + if (pipe(stdinFds) != 0 )
> + {
> + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + "Error piping hook script %s.", cmd);
> + }
> + fd_map[0] = stdinFds[0];
> + }
> + else
> + {
> +#pragma convert(0)
> + if (fd_map[0] = open("/dev/zero", O_RDONLY) == -1)
Does this line work as intended? Isn't the operator precedence of "==" higher
than "="?
> +#pragma convert(1208)
> + return svn_error_createf(
> + SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + "Error opening /dev/zero for hook script %s", cmd);
Isn't /dev/null more appropriate? It would be less likely to make the script
loop forever if it should try to read its input.
> + }
> +
> +
> + /* Map stdout. */
> +#pragma convert(0)
> + if (fd_map[1] = open("/dev/null", O_WRONLY) == -1)
> +#pragma convert(1208)
> + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + "Error opening /dev/null for hook script %s",
> + cmd);
> +
> + /* Map stderr. */
> + fd_map[2] = read_errstream ? useFds[1] : ignoreFds[1];
> +
> + /* Spawn the hook command. */
> + if ((child_pid = spawn(native_args[0], 3, fd_map, &xmp_inherit,
> + native_args, xmp_envp)) == -1)
> + {
> + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + "Error spawning process for hook script %s.",
> + cmd);
> + }
> +
> + /* If there is "APR stdin", read it and then write it to the hook's stdin
> + * pipe. */
> + if (stdin_handle)
> + {
> + apr_size_t bytes_read = sizeof(buffer);
> + svn_error_t *err;
> + while (!svn_io_file_read(stdin_handle, buffer, &bytes_read, pool))
Error leak - looks like you were intending to catch the error in variable "err"
but forgot to do so.
> + {
> + if (write(stdinFds[1], buffer, bytes_read) == -1)
> + return svn_error_createf(
> + SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + "Error piping stdin to hook script %s.", cmd);
> + bytes_read = sizeof(buffer);
> + }
> + if (close(stdinFds[1]) == -1)
> + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + "Error closing write end of stdin pipe");
> +
> + if (close(stdinFds[0]) == -1)
> + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + "Error closing read end of stdin pipe");
> + }
> +
> + /* Close the write ends of the stdout and stderr pipes so we don't hang
> + * on the read ends later. */
> + if (close(ignoreFds[1]) == -1)
> + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + "Error closing write end of stdout pipe");
> +
> + if (close(useFds[1]) == -1)
> + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + "Error closing write end of stderr pipe");
> +
> + /* Close the unused read end of our ignored pipe. */
> + if (close(ignoreFds[0]) == -1)
> + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + "Error closing read end of stdout pipe");
> +
> + /* Read the hook's stderr. */
> + while ((rc = read(useFds[0], buffer, sizeof(buffer))) > 0)
If the hook script is, at this point, executing asynchronously ("in the
background"), doesn't this loop give up immediately, before the script has a
chance to write anything? In other words, doesn't this need to keep going
while the size read is zero or more (preferably using some technique to avoid
spinning at 100% CPU usage) and only stop when it sees EOF?
> + {
> + buffer[rc] = '\0';
This can write to the byte after the end of the buffer...
> + svn_stringbuf_appendcstr(script_output, buffer);
... so just avoid using a null terminator and use svn_stringbuf_appendbytes()
instead.
> + }
> +
> + /* Close the read end of the stderr pipe. */
> + if (close(useFds[0]) == -1)
> + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + "Error closing read end of stderr pipe");
> +
> + /* Wait for the child process to complete. */
> + if ((wait_rv = waitpid(child_pid, &exitcode, 0)) == -1)
> + {
> + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + "Error waiting for process completion of " \
No need for the line-continuation backslash.
> + "hook script %s.", cmd);
> + }
> +
> + if (script_output->len > 1)
Why not also if len == 1?
> + {
> + SVN_ERR(svn_utf_cstring_to_utf8_ex(&script_stderr_utf8,
> + script_output->data,
> + (const char*)0,
> + SVN_UTF_ETOU_XLATE_HANDLE,
> + pool));
> + }
> +
> + if (WIFEXITED(exitcode))
> + {
> + if (WEXITSTATUS(exitcode))
> + {
> + if (read_errstream)
> + {
> + return svn_error_createf
> + (SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + _("'%s' hook failed with error output:\n%s"), name,
(Style trivia: one more space of indent here,
> + script_stderr_utf8);
and one less here,
> + }
> + else
> + {
> + return svn_error_createf
> + (SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + _("'%s' hook failed; no error output available"), name);
and one more here.)
> + }
> + }
> + else
> + return SVN_NO_ERROR;
> + }
> + else if (WIFSIGNALED(exitcode))
> + {
> + return svn_error_createf
> + (SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + _("Process '%s' failed because of an uncaught terminating signal"),
> + cmd);
> + }
> + else if (WIFEXCEPTION(exitcode))
> + {
> + return svn_error_createf
> + (SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + _("Process '%s' failed unexpectedly with OS400 exception %d"),
> + cmd, WEXCEPTNUMBER(exitcode));
> + }
> + else if (WIFSTOPPED(exitcode))
> + {
> + return svn_error_createf
> + (SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + _("Process '%s' stopped unexpectedly by signal %d"),
> + cmd, WSTOPSIG(exitcode));
> + }
> + else
> + {
> + return svn_error_createf
> + (SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + _("Process '%s' failed unexpectedly"), cmd);
> + }
> +}
> +#endif /* AS400 */
The rest looks OK but this is only a read-through review, I haven't tried
running it.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Mar 17 23:20:36 2006