Paul Burba <paulb@softlanding.com> writes:
> When running hook scripts, svn_io_wait_for_cmd() calls:
>
> APR_DECLARE(apr_status_t)
> apr_proc_wait(apr_proc_t *proc,
> int *exitcode,
> apr_exit_why_e *exitwhy,
> apr_wait_how_e waithow)
>
> This function works on OS400 to the extent that the hook script is run,
> but there are some problems, the most significant being that exitcode is
> always set to 0, regardless of what the script returns. This is obviously
> problematic when we care if the script failed or not (e.g. pre-commit
> hook).
>
> This patch adds a new OS400 specific function to svn_io.h that uses
> spawn() and waitpid() to run scripts and obtain the script's exit code.
>
> Please take a look and let me know what you think, thanks,
First impression is that it's ugly. You have replaced the two
functions svn_io_start_cmd and svn_io_wait_for_cmd with the single
function svn_io_run_os400_cmd. I assume you did it like that because
the two functions communicate via an apr_proc_t and that doesn't allow
you to add extra data.
Since this is a workaround for an APR bug I'm not sure why you chose
to put a public function in libsvn_subr when the only caller is in
libsvn_repos. Does the OS400 port support an external diff or
external editor? Your patch doesn't seem to have addressed those.
An alternative would be to modify the existing svn_io_start_cmd and
svn_io_wait_for_cmd functions to pass svn_io_proc_t instead of
apr_proc_t, this would allow you to add any extra data you need. I
think svn_io_proc_t could be opaque and so all the OS400 stuff could
be private to io.c. It would mean that the external diff/editor code
get fixed as well, if that makes a difference.
I feel a bit guilty about suggesting that since I seem to be asking
for all your patches to be re-written :-/
> Index: subversion/libsvn_repos/hooks.c
> ===================================================================
> --- subversion/libsvn_repos/hooks.c (revision 18637)
> +++ subversion/libsvn_repos/hooks.c (working copy)
> @@ -60,6 +60,7 @@
> apr_exit_why_e exitwhy;
> apr_proc_t cmd_proc;
>
> +#ifndef AS400
> /* Create a pipe to access stderr of the child. */
> apr_err = apr_file_pipe_create(&read_errhandle, &write_errhandle, pool);
> if (apr_err)
> @@ -95,7 +96,13 @@
>
> err = svn_io_start_cmd(&cmd_proc, ".", cmd, args, FALSE,
> stdin_handle, null_handle, write_errhandle, pool);
> +#else
> + /* The functionality of svn_io_start_cmd() and svn_io_wait_for_cmd() is
> + * handled in svn_io_run_as400_cmd() on OS400. */
svn_io_run_as400_cmd should be svn_io_run_os400_cmd
> + err = NULL;
> +#endif /* AS400 */
>
> +#ifndef AS400
> /* This seems to be done automatically if we pass the third parameter of
> apr_procattr_child_in/out_set(), but svn_io_run_cmd()'s interface does
> not support those parameters. We need to close the write end of the
> @@ -104,6 +111,7 @@
> if (!err && apr_err)
> return svn_error_wrap_apr
> (apr_err, _("Error closing write end of stderr pipe"));
> +#endif /* AS400 */
>
> if (err)
> {
> @@ -116,9 +124,17 @@
> const char *error;
> svn_error_t *err2;
>
> +#ifndef AS400
> err2 = svn_stringbuf_from_aprfile(&native_error, read_errhandle, pool);
>
> err = svn_io_wait_for_cmd(&cmd_proc, cmd, &exitcode, &exitwhy, pool);
> +#else
> + err2 = SVN_NO_ERROR;
> +
> + err = svn_io_run_os400_cmd(cmd, args, &exitcode, &exitwhy, FALSE,
> + TRUE, &native_error, pool);
> +#endif /* AS400 */
Another alternative would be to adopt your svn_io_run_os400_cmd
interface on all platforms, that would be another way to remove the
AS400 stuff from this file. On the whole I think I prefer the
start/wait/svn_io_proc_t approach.
> +
> if (! err)
> {
> if (! APR_PROC_CHECK_EXIT(exitwhy) || exitcode != 0)
> @@ -150,6 +166,7 @@
> }
> }
>
> +#ifndef AS400
> /* Hooks are fallible, and so hook failure is "expected" to occur at
> times. When such a failure happens we still want to close the pipe
> and null file */
> @@ -161,6 +178,7 @@
> apr_err = apr_file_close(null_handle);
> if (!err && apr_err)
> return svn_error_wrap_apr(apr_err, _("Error closing null file"));
> +#endif
>
> return err;
> }
> Index: subversion/libsvn_subr/io.c
> ===================================================================
> --- subversion/libsvn_subr/io.c (revision 18637)
> +++ subversion/libsvn_subr/io.c (working copy)
> @@ -42,6 +42,10 @@
> #include <apr_portable.h>
> #include <apr_md5.h>
>
> +#if AS400
> +#include <spawn.h> /* For spawn() and struct inheritance. */
> +#endif
> +
> #include "svn_types.h"
> #include "svn_path.h"
> #include "svn_string.h"
> @@ -54,6 +58,7 @@
>
> #ifdef AS400
> #define SVN_UTF_UTOE_XLATE_HANDLE "svn-utf-utoe-xlate-handle"
> +#define SVN_UTF_ETOU_XLATE_HANDLE "svn-utf-etou-xlate-handle"
We already have one of those in libsvn_subr/cmdline.c
> #endif
>
> /*
> @@ -2058,7 +2063,164 @@
> }
>
>
> +#if AS400
> svn_error_t *
> +svn_io_run_os400_cmd(const char *cmd,
> + const char *const *args,
> + int *exitcode,
> + apr_exit_why_e *exitwhy,
> + svn_boolean_t read_stdout,
> + svn_boolean_t read_stderr,
> + svn_stringbuf_t **err_stream,
> + apr_pool_t *pool)
> +{
> + int rc, fd_map[3], ignoreFds[2], useFds[2], exitcode_val;
> + char buffer[20];
> + const char **native_args = NULL;
> + struct inheritance xmp_inherit = {0};
> + pid_t child_pid, wait_rv;
> + svn_stringbuf_t *script_output = svn_stringbuf_create("", pool);
> + apr_size_t args_arr_size = 0, i = 0;
> + apr_exit_why_e exitwhy_val;
> +#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)
> +
> + *err_stream = svn_stringbuf_create("", pool);
> +
> + /* 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,
> + 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);
> + }
> +
> + /* Map stdin, stdout, stderr. */
> + fd_map[0] = ignoreFds[1];
> + fd_map[1] = read_stdout ? useFds[1] : ignoreFds[1];
> + fd_map[2] = read_stderr ? useFds[1] : ignoreFds[1];
> +
> + /* Spawn the 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);
> + }
> +
> + /* ...and wait for it to finish. */
> + if ((wait_rv = waitpid(child_pid, &exitcode_val, 0)) == -1)
> + {
> + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + "Error waiting for process completion of " \
> + "hook script %s.", cmd);
> + }
The reason we have two functions, start and wait, on other platforms
is that if the script produces lots of output it fills the pipe and
then blocks waiting for the pipe to be read. If at the same time the
parent is blocked waiting for the child we get deadlock. It looks
like your implementation suffers from the same problem, perhaps you
should move the read before the wait?
> +
> + close(ignoreFds[1]);
> + close(useFds[1]);
> +
> + /* Create svn_stringbuf containing any messages the script sent to
> + * stderr and/or stdout. */
> + while ((rc = read(useFds[0], buffer, sizeof(buffer))) > 0)
> + {
> + buffer[rc] = '\0';
> + svn_stringbuf_appendcstr(script_output, buffer);
> + }
> +
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Feb 28 23:15:49 2006