You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Paul Burba <pa...@softlanding.com> on 2006/02/27 20:26:12 UTC
[PATCH] #9 OS400/EBCDIC Port: Running Hook Scripts
Hello All,
In the home stretch now, this is the second to last of the core
OS400/EBCDIC patches.
(Don't know what the OS400/EBCDIC port is about? See:
http://svn.haxx.se/dev/archive-2006-02/0519.shtml)
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,
Paul B.
[[[
OS400/EBCDIC Port:
This is one of several patches to allow Subversion to run on IBM's
OS400 V5R4. It provides a workaround of a limitation with
apr_proc_wait() which always indicates a process exited
successfully, regardless of whether it did or not.
* subversion\include\svn_io.h
(svn_io_run_os400_cmd): New function declaration.
* subversion\libsvn_repos\hooks.c
(run_hook_cmd): Use new function in place of svn_io_start_cmd()
and svn_io_wait_for_cmd().
* subversion\libsvn_subr\io.c
(SVN_UTF_ETOU_XLATE_HANDLE): New xlate key for EBCDIC to UTF-8
conversions.
(svn_io_run_os400_cmd): New function definition.
]]]
_____________________________________________________________________________
Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs.
_____________________________________________________________________________
Re: [PATCH] #9 OS400/EBCDIC Port: Running Hook Scripts
Posted by Paul Burba <pa...@softlanding.com>.
Peter Lundblad <pe...@famlundblad.se> wrote on 02/28/2006 03:09:16 AM:
> Because that'd be a deadlock:-) We create pipes to read the hooks'
> stderr output. We must read that output before waiting for the
> process, else the pipe buffers might fil up. (I think Paul's patch has
> the same problem.)
Philip Martin <ph...@codematters.co.uk> wrote on 02/28/2006 05:12:30 PM:
> Paul Burba <pa...@softlanding.com> writes:
> 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 :-/
Hi Gents,
Thanks for taking a look at this patch, I probably won't have time to
address your comments until Thursday.
Sorry for the delay, I'll get back to this soon, but I didn't want you to
think I was ignoring your comments!
Thanks,
Paul B.
_____________________________________________________________________________
Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs.
_____________________________________________________________________________
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] #9 OS400/EBCDIC Port: Running Hook Scripts
Posted by Philip Martin <ph...@codematters.co.uk>.
Paul Burba <pa...@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
Re: [PATCH] #9 OS400/EBCDIC Port: Running Hook Scripts
Posted by Peter Lundblad <pe...@famlundblad.se>.
> > * subversion\include\svn_io.h
> > (svn_io_run_os400_cmd): New function declaration.
>
> Can't you just use the API svn_io_run_cmd() and adjust its implementation?
>
> Hmm, why aren't we using svn_io_run_cmd() already? Because it's inadequate?
> Can we make it adequate?
Because that'd be a deadlock:-) We create pipes to read the hooks'
stderr output. We must read that output before waiting for the
process, else the pipe buffers might fil up. (I think Paul's patch has
the same problem.)
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] #9 OS400/EBCDIC Port: Running Hook Scripts
Posted by Paul Burba <pa...@softlanding.com>.
Julian Foad <ju...@btopenworld.com> wrote on 03/22/2006 05:40:09 PM:
> Paul Burba wrote:
> > My description of these is likely not clear enough. They are keys for
> > caching xlate_handle_node_t structs for conversions between EBCDIC and
> > UTF-8 and vice-versa when calling svn_utf_cstring_to_utf8_ex() and
> > svn_utf_cstring_from_utf8_ex(), i.e. the convset_key argument to these
> > functions. I've tried to improve the comments for these in the
attached
> > patch.
>
> Thanks for the further info. I've posted a request for more info about
their
> usage in those function calls as I don't think those functions are
> sufficiently
> documented. I'm not sure that we need or want to make these things
> part of the
> public API, but you can just do something that works for now and we
> can resolve
> the theoretical issue in due course.
>
> As for the issue of where/how to define the char-set-conversion
> cache keys, go
> with whatever seems to be the best way of defining them for now. If
> we want to
> change it later (e.g. make them private), then we can change it later.
For now it seems safest to not make these public until the larger
questions regarding svn_utf_cstring_*_utf8_ex() are resolved. To that end
I changed the patch so it exclusively deals only with the hook script
problem. The two calls to svn_utf_cstring_*_utf8_ex() in hooks.c now use
string literals for the convset_key args.
> There are a few other style nits (open-paren at EOL, space before
> close-paren,
> space at EOL) if you care to look for them, but they are unimportant.
>
> I can't see any logic problems at all.
>
> With the redundant initialisers removed, +1 to commit, with or without
the
> trivial style fixes.
I fixed (hopefully all) the style problems. Unless anyone objects to the
changes regarding conversion set keys I'll commit this this afternoon.
Thanks again,
Paul B.
[[[
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/libsvn_repos/hooks.c
Include spawn.h and fcntl.h
(run_hook_cmd): New "APR-free" implementation for OS400.
]]]
_____________________________________________________________________________
Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs.
_____________________________________________________________________________
Re: [PATCH] #9 OS400/EBCDIC Port: Running Hook Scripts
Posted by Julian Foad <ju...@btopenworld.com>.
Paul Burba wrote:
> Julian Foad <ju...@btopenworld.com> wrote on 03/17/2006 05:20:07 PM:
>
> I think I followed Philip regarding that. I was trying to point out that
> option #3...
>
> 3. Move the patch entirely within hooks.c. Just add a second
> definition for run_hook_cmd() along with the existing
> implementation within a #ifndef AS400...#else...#endif
> block.
>
> ...requires that SVN_UTF_UTOE_XLATE_HANDLE and SVN_UTF_ETOU_XLATE_HANDLE
> be public, since both libsvn_repos and libsvn_subr would now require them.
Oh yes - I didn't notice that.
>>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.
>
> My description of these is likely not clear enough. They are keys for
> caching xlate_handle_node_t structs for conversions between EBCDIC and
> UTF-8 and vice-versa when calling svn_utf_cstring_to_utf8_ex() and
> svn_utf_cstring_from_utf8_ex(), i.e. the convset_key argument to these
> functions. I've tried to improve the comments for these in the attached
> patch.
Thanks for the further info. I've posted a request for more info about their
usage in those function calls as I don't think those functions are sufficiently
documented. I'm not sure that we need or want to make these things part of the
public API, but you can just do something that works for now and we can resolve
the theoretical issue in due course.
>>>+ 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.)
>
> Ah, because the subscripting and postfix increment operators have the same
> precedence(?).
No; it is because the argument "args[i++]" might be evaluated (which includes
all of its side effects being completed) before the argument "&native_args[i]",
and so the value of "i" used in evaluating the latter might be the incremented
rather than the original value.
> Changed it to a for loop.
Lovely.
>>>+ /* 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
[...]
>
> I'm not familiar with *nix versions of read() so possibly IBM's versions
> behave quite differently. Reading IBM's docs for pipe() and read() it's
> my understanding that read() would block until it had something to read.
> My testing of this patch appears to confirm that (if a script sleeps the
> read will block till it wakes up).
Oh, good. I didn't realise it would be in blocking mode by default. It
probably is the same on Unix.
> Index: subversion/libsvn_repos/hooks.c
> ===================================================================
> --- subversion/libsvn_repos/hooks.c (revision 18977)
> +++ subversion/libsvn_repos/hooks.c (working copy)
[...]
> +{
> + const char *script_stderr_utf8 = "";
> + const char **native_args = NULL;
> + char buffer[20];
> + int rc, fd_map[3], stderr_pipe[2], stdin_pipe[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;
You can remove the initialiser from 'i', now. And from 'native_args' which is
also initialised at its first use.
[...]
> + else
> + {
> + /* Just dump stderr to /dev/null if we don't want it. */
> + fd_map[2] = open(dev_null_ebcdic, O_WRONLY);
> + if (fd_map[2] == -1)
> + return svn_error_createf
> + (SVN_ERR_EXTERNAL_PROGRAM, NULL,
Odd indentation there...
> + "Error opening /dev/null for hook script '%s'", cmd);
> + }
> +
> + /* Spawn the hook command. */
> + child_pid = spawn(native_args[0], 3, fd_map, &xmp_inherit, native_args,
> + xmp_envp);
> + if (child_pid == -1)
> + {
> + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + "Error spawning process for hook script '%s'",
... and there.
> + 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;
Those two variables might as well go inside the "while" loop...
> +
> + while (1)
> + {
> + int wc;
> + err = svn_io_file_read(stdin_handle, buffer, &bytes_read, pool);
> + if (err && APR_STATUS_IS_EOF(err->apr_err))
> + break;
> +
> + if (err)
> + return svn_error_createf
> + (SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + "Error piping stdin to hook script '%s'", cmd);
> +
> + wc = write(stdin_pipe[1], buffer, bytes_read);
> + if (wc == -1)
> + return svn_error_createf
> + (SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + "Error piping stdin to hook script '%s'", cmd);
> +
> + bytes_read = sizeof(buffer);
... making that statement redundant.
> + }
There are a few other style nits (open-paren at EOL, space before close-paren,
space at EOL) if you care to look for them, but they are unimportant.
I can't see any logic problems at all.
With the redundant initialisers removed, +1 to commit, with or without the
trivial style fixes.
As for the issue of where/how to define the char-set-conversion cache keys, go
with whatever seems to be the best way of defining them for now. If we want to
change it later (e.g. make them private), then we can change it later.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] #9 OS400/EBCDIC Port: Running Hook Scripts
Posted by Paul Burba <pa...@softlanding.com>.
Julian Foad <ju...@btopenworld.com> wrote on 03/17/2006 05:20:07 PM:
> Paul Burba wrote:
> > Philip Martin <ph...@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
theyshould go
> in a private header that is local to libsvn_subr.
I think I followed Philip regarding that. I was trying to point out that
option #3...
3. Move the patch entirely within hooks.c. Just add a second
definition for run_hook_cmd() along with the existing
implementation within a #ifndef AS400...#else...#endif
block.
...requires that SVN_UTF_UTOE_XLATE_HANDLE and SVN_UTF_ETOU_XLATE_HANDLE
be public, since both libsvn_repos and libsvn_subr would now require them.
> 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.
My description of these is likely not clear enough. They are keys for
caching xlate_handle_node_t structs for conversions between EBCDIC and
UTF-8 and vice-versa when calling svn_utf_cstring_to_utf8_ex() and
svn_utf_cstring_from_utf8_ex(), i.e. the convset_key argument to these
functions. I've tried to improve the comments for these in the attached
patch.
> > 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.
Done.
> > +#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...".)
Fixed that.
> > + 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.)
Ah, because the subscripting and postfix increment operators have the same
precedence(?). Changed it to a for loop.
> > + 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".)
Ok, done. Also removed any trailing periods in the error messages and put
the script name in all error messages, which wasn't consistently done in
the last patch.
> Also, you probably want to mark either all of them for translation (with
> "_(...)") or none of them.
We don't support translation as of yet on the iSeries port so I removed
all of them.
> > +#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 "="?
Ouch, not sure what I was thinking on that...Fixed.
> > +#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.
Agreed, it certainly will hang, changed that.
> > + }
> > +
> > +
> > + /* 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.
I changed this to be a while (1) loop that correctly catches errors on
both the read of stdin_handle and the write to stdin_pipe[1].
> > + {
> > + 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 techniqueto
avoid
> spinning at 100% CPU usage) and only stop when it sees EOF?
I'm not familiar with *nix versions of read() so possibly IBM's versions
behave quite differently. Reading IBM's docs for pipe() and read() it's
my understanding that read() would block until it had something to read.
My testing of this patch appears to confirm that (if a script sleeps the
read will block till it wakes up).
Re: [PATCH] #9 OS400/EBCDIC Port: Running Hook Scripts
Posted by Julian Foad <ju...@btopenworld.com>.
Paul Burba wrote:
> Philip Martin <ph...@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
Re: [PATCH] #9 OS400/EBCDIC Port: Running Hook Scripts
Posted by Paul Burba <pa...@softlanding.com>.
Philip Martin <ph...@codematters.co.uk> wrote on 03/14/2006 02:31:37 PM:
> Paul Burba <pa...@softlanding.com> writes:
>
> >> > #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
> >
> > Yes, but how does that help me in io.c? Unless you mean these should
be
> > made public...should svn_utf.h have all of these?
> >
> > #define SVN_UTF_NTOU_XLATE_HANDLE "svn-utf-ntou-xlate-handle"
> > #define SVN_UTF_UTON_XLATE_HANDLE "svn-utf-uton-xlate-handle"
>
> No, those are only used in one file.
>
> > #ifdef AS400
> > #define SVN_UTF_UTOE_XLATE_HANDLE "svn-utf-utoe-xlate-handle"
> > #define SVN_UTF_ETOU_XLATE_HANDLE "svn-utf-etou-xlate-handle"
> > #endif
>
> 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 feel a bit guilty about suggesting that since I seem to be asking
> >> for all your patches to be re-written :-/
> >
> > Heh, I'm not worried about that, I just want it done correctly. I see
3
> > possible options to solve this:
> >
> > 1. Tweak the existing patch to avoid the deadlock issue and call it a
day.
> >
> > 2. Figure out how to make apr_file_t and spawn() work together.
> >
> > 3. Move the patch entirely within hooks.c. Just add a second
definition
> > for run_hook_cmd() along with the existing implementation within a
#ifndef
> > AS400...#else...#endif block.
> >
> > FWIW I like option 3, it is certainly the smallest patch and
introduces no
> > new APIs.
>
> 3 is fine as far as I am concerned.
Julian Foad <ju...@btopenworld.com> wrote on 03/14/2006 12:47:53 PM:
> I'd be happy with whatever minimises the impact of these work-arounds on
our
> code base. (Not just for OS400 but any work-arounds for any stuff that
APR
> ought to handle.)
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.
Thanks to both of you for the reviews, please let me know what you think
of this new patch.
Paul B.
======================================================================
[[[
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.
]]]
_____________________________________________________________________________
Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs.
_____________________________________________________________________________
Re: [PATCH] #9 OS400/EBCDIC Port: Running Hook Scripts
Posted by Philip Martin <ph...@codematters.co.uk>.
Paul Burba <pa...@softlanding.com> writes:
>> > #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
>
> Yes, but how does that help me in io.c? Unless you mean these should be
> made public...should svn_utf.h have all of these?
>
> #define SVN_UTF_NTOU_XLATE_HANDLE "svn-utf-ntou-xlate-handle"
> #define SVN_UTF_UTON_XLATE_HANDLE "svn-utf-uton-xlate-handle"
No, those are only used in one file.
> #ifdef AS400
> #define SVN_UTF_UTOE_XLATE_HANDLE "svn-utf-utoe-xlate-handle"
> #define SVN_UTF_ETOU_XLATE_HANDLE "svn-utf-etou-xlate-handle"
> #endif
I'd suggest putting SVN_UTF_ETOU_XLATE_HANDLE into a libsvn_subr
header, it doesn't need to be visible outside libsvn_subr.
>> I feel a bit guilty about suggesting that since I seem to be asking
>> for all your patches to be re-written :-/
>
> Heh, I'm not worried about that, I just want it done correctly. I see 3
> possible options to solve this:
>
> 1. Tweak the existing patch to avoid the deadlock issue and call it a day.
>
> 2. Figure out how to make apr_file_t and spawn() work together.
>
> 3. Move the patch entirely within hooks.c. Just add a second definition
> for run_hook_cmd() along with the existing implementation within a #ifndef
> AS400...#else...#endif block.
>
> FWIW I like option 3, it is certainly the smallest patch and introduces no
> new APIs.
3 is fine as far as I am concerned.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] #9 OS400/EBCDIC Port: Running Hook Scripts
Posted by Julian Foad <ju...@btopenworld.com>.
Paul Burba wrote:
[...lots of stuff...]
I'm afraid I don't have anything to say about all that stuff ... about IBM's
idea of APR, etc., except that it all seems a bit sad at the moment ... unless
they regard the whole thing as an impressive result to have achieved so far,
with just a few rough edges remaining, in which case hopefully they will sort
this out soon. Have IBM given you any further reply yet? (At the time of your
last message, they'd said stuff like "I've found a comment..." which is
obviously not from a sufficiently high level of staff :-)
> 1. Tweak the existing patch to avoid the deadlock issue and call it a day.
>
> 2. Figure out how to make apr_file_t and spawn() work together.
>
> 3. Move the patch entirely within hooks.c. Just add a second definition
> for run_hook_cmd() along with the existing implementation within a #ifndef
> AS400...#else...#endif block.
>
> FWIW I like option 3, it is certainly the smallest patch and introduces no
> new APIs.
I'd be happy with whatever minimises the impact of these work-arounds on our
code base. (Not just for OS400 but any work-arounds for any stuff that APR
ought to handle.)
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] #9 OS400/EBCDIC Port: Running Hook Scripts
Posted by Paul Burba <pa...@softlanding.com>.
> Julian Foad <ju...@btopenworld.com> wrote on 02/27/2006 09:22:15
PM:
>
> > Paul Burba wrote:
> > Again, I have to ask, are you sure this is a known limitation and not
a
> bug
> > that IBM could fix next week if told about it? This is a
> > considerable chunk of
> > work-around code.
>
> Julian,
>
> We've approached IBM about this before but didn't get a response. As I
> write this we're attempting to get some answers via a different channel.
> I'll update you as soon as I hear something.
>
> One other thing; while putting a new description of the problem together
> for IBM I found the problem isn't that apr_proc_wait() always sets
> exitcode to 0, but rather that it doesn't set it at all (the
uninitialized
> value was simply 0). The impact of the problem is still the same.
Hi All,
Ok, down to the *LAST* core patch for OS400 V5R4 on trunk! Sorry for the
delay in getting back to this, I've been busy with other work and waiting
on responses from IBM. And please bear with me on this, I'm a neophyte
with APR processes but I'll do my best to address the various concerns
you've each raised.
First, re IBM fixing this, I heard back from them and here is what they
had to say:
"In the case where a pid is passed to apr_proc_wait, we see if there is a
helper job. If so, we free it and return APR_CHILD_DONE. You are correct
that we do not set the exitcode in this case. However if this function is
called with a -1 for a pid meaning to wait for all processes to end, we do
in some cases set the exitcode. Our platform has a different
implementation because our child processes are handled through helper
jobs."
(BTW none of this is documented in IBM's header files - see
http://publib.boulder.ibm.com/infocenter/iseries/v5r3/index.jsp?topic=/rzaie/apr_core_api/apr__thread__proc_8h-source.html
if you are curious)
The "helper job" mentioned is a new IBM addition to apr_proc_t:
typedef struct apr_proc_t {
.
.
.
#ifdef AS400
apr_int32_t cmdtype; /* @A1A */
void *myHelperJob;
#endif
} apr_proc_t;
Sure enough, if apr_proc_t *cmd_proc->pid is set to -1, that exitcode_val
is set properly when calling apr_proc_wait(cmd_proc, &exitcode_val,
&exitwhy_val, APR_WAIT) in svn_io_wait_for_cmd(). The problem is, of
course, IBM's claim that "we do in *some* cases set the exitcode". That's
not very reassuring! So back to IBM, I asked them:
* If there is documentation of how these helper jobs work in the
context of APR, apr_thread_proc.h doesn't have much to say.
* In what cases the exitcode gets set and when it doesn't.
* My first post mentioned that "there are some problems" besides
the above when running hooks on OS400. I should mention the
other one now; the script's stderr is not written to the
errfile specified by apr_procattr_child_err_set(). So even if
apr_proc_wait() could be relied upon to return the scripts exit
code (which is doubtful) I can't get the script's stderr. So my
final question for IBM was if it's even possible to read a child
process' stdout/stderr using APR calls.
This is the only answer I've gotten to any of these questions:
"I have been digging around in our code and see that apr_proc_create()
uses
spawn() via apr_spawn() to start the helper job. In apr_proc_create, we
set the environment variable QIBM_USE_DESCRIPTOR_STDIO to Y so that file
descriptors 0, 1,and 2 are used for stdin, stdout, and stderr. In
apr_proc_create, we will over-ride attr->child_in (stdin) with what is
passed to apr_proc_create which in turn gets used on the spawn().
Another significant comment I found is that we use socketpair instead of
pipes for better performance on our platform."
So basically IBM has tossed the "P" in APR out the window. The point of
this lengthy semi-rant is that *some* manner of work-around is needed for
OS400. Running a script, getting its exit code, and reading the script's
stderr just doesn't seem possible using IBM's APR.
Peter Lundblad <pe...@famlundblad.se> wrote on 02/28/2006 03:09:16 AM:
> Because that'd be a deadlock:-) We create pipes to read the hooks'
> stderr output. We must read that output before waiting for the
> process, else the pipe buffers might fil up. (I think Paul's patch has
> the same problem.)
Philip Martin <ph...@codematters.co.uk> wrote on 02/28/2006 05:12:30 PM:
> 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?
You two are of course correct regarding the deadlock potential. I wrote a
script that writes several 1000 lines to strderr and that's exactly what
happens. This is fairly easy to fix in the context of my current patch or
within whatever approach is eventually adopted...
> 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.
I did it to avoid adding two new OS400 specific functions when I could
just add one. Also, I wasn't aware of the deadlock issue at the time.
> 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.
I'm hesitant to call it a "bug" so much as IBM going their own merry
(undocumented) way. Since I never thought of it as a bug, it seemed
appropriate to put it in libsvn_subr, just like svn_io_start_cmd() and
svn_io_wait_for_cmd(), which are also only called from libsvn_repos.
> Does the OS400 port support an external diff or
> external editor? Your patch doesn't seem to have addressed those.
The OS400 ports don't currently support either.
> 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've looked at this and momentarily (for about 15 glorious minutes earlier
today) I thought it was doable. But now I'm not sure how to make it work.
svn_io_start_cmd() works with apr_file_t * but spawn() works with int
file descriptors. I'm not sure how I can make spawn() and apr_file_t *
play nicely together, specifically how would I connect the spawn()ed
process' stderr to the apr_file_t *errfile passed to svn_io_start_cmd()?
Worse, if svn_io_proc_t is opaque, each item of additional information I
store in it would need a public settor function in libsvn_subr no?
> > 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
Yes, but how does that help me in io.c? Unless you mean these should be
made public...should svn_utf.h have all of these?
#define SVN_UTF_NTOU_XLATE_HANDLE "svn-utf-ntou-xlate-handle"
#define SVN_UTF_UTON_XLATE_HANDLE "svn-utf-uton-xlate-handle"
#ifdef AS400
#define SVN_UTF_UTOE_XLATE_HANDLE "svn-utf-utoe-xlate-handle"
#define SVN_UTF_ETOU_XLATE_HANDLE "svn-utf-etou-xlate-handle"
#endif
> I feel a bit guilty about suggesting that since I seem to be asking
> for all your patches to be re-written :-/
Heh, I'm not worried about that, I just want it done correctly. I see 3
possible options to solve this:
1. Tweak the existing patch to avoid the deadlock issue and call it a day.
2. Figure out how to make apr_file_t and spawn() work together.
3. Move the patch entirely within hooks.c. Just add a second definition
for run_hook_cmd() along with the existing implementation within a #ifndef
AS400...#else...#endif block.
FWIW I like option 3, it is certainly the smallest patch and introduces no
new APIs.
Thoughts?
Paul B.
P.S. Why do I feel like Grasshopper on Kung Fu? You guys can fight over
who gets to be Master Po.
_____________________________________________________________________________
Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs.
_____________________________________________________________________________
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] #9 OS400/EBCDIC Port: Running Hook Scripts
Posted by Paul Burba <pa...@softlanding.com>.
Julian Foad <ju...@btopenworld.com> wrote on 02/27/2006 09:22:15 PM:
> Paul Burba wrote:
> > [[[
> > OS400/EBCDIC Port:
> >
> > This is one of several patches to allow Subversion to run on IBM's
> > OS400 V5R4. It provides a workaround of a limitation with
> > apr_proc_wait() which always indicates a process exited
> > successfully, regardless of whether it did or not.
>
> Again, I have to ask, are you sure this is a known limitation and not a
bug
> that IBM could fix next week if told about it? This is a
> considerable chunk of
> work-around code.
Julian,
We've approached IBM about this before but didn't get a response. As I
write this we're attempting to get some answers via a different channel.
I'll update you as soon as I hear something.
One other thing; while putting a new description of the problem together
for IBM I found the problem isn't that apr_proc_wait() always sets
exitcode to 0, but rather that it doesn't set it at all (the uninitialized
value was simply 0). The impact of the problem is still the same.
Paul B.
_____________________________________________________________________________
Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs.
_____________________________________________________________________________
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] #9 OS400/EBCDIC Port: Running Hook Scripts
Posted by Julian Foad <ju...@btopenworld.com>.
Paul Burba wrote:
> [[[
> OS400/EBCDIC Port:
>
> This is one of several patches to allow Subversion to run on IBM's
> OS400 V5R4. It provides a workaround of a limitation with
> apr_proc_wait() which always indicates a process exited
> successfully, regardless of whether it did or not.
Again, I have to ask, are you sure this is a known limitation and not a bug
that IBM could fix next week if told about it? This is a considerable chunk of
work-around code.
>
> * subversion\include\svn_io.h
> (svn_io_run_os400_cmd): New function declaration.
Can't you just use the API svn_io_run_cmd() and adjust its implementation?
Hmm, why aren't we using svn_io_run_cmd() already? Because it's inadequate?
Can we make it adequate?
> Index: subversion/libsvn_subr/io.c
> ===================================================================
[...]
> +#include <spawn.h> /* For spawn() and struct inheritance. */
Minor nit: comments that say why a header was included never get updated and so
are best omitted.
This isn't a review, just some initial comments. I might be able to look at it
tomorrow.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org