Julian Foad <julianfoad@btopenworld.com> wrote on 03/17/2006 05:20:07 PM:
> 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
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).
From the pipe() docs -
http://publib.boulder.ibm.com/infocenter/iseries/v5r4/index.jsp?topic=/apis/pipe2.htm:
The pipe() function creates a data pipe and places two
file descriptors, one each into the arguments fildes[0]
and fildes[1], that refer to the open file descriptions
for the read and write ends of the pipe, respectively.
Their integer values will be the two lowest available at
the time of the pipe() call. The O_NONBLOCK and
FD_CLOEXEC flags will be clear on both descriptors.
NOTE: these flags can, however, be set by the fcntl()
function.
From the read() docs -
http://publib.boulder.ibm.com/infocenter/iseries/v5r4/index.jsp?topic=/apis/read.htm:
When attempting to read from an empty pipe or FIFO:
* If no job has the pipe or FIFO open for writing,
read() return 0 to indicate end-of-file.
* If some job has the pipe or FIFO open for writing
and O_NONBLOCK was not specified, read() will block
the calling thread until some data is written or
until the pipe or FIFO is closed by all jobs that
had the pipe or FIFO open for writing.
Regardless of the above there is another issue: there is no error check on
the read(), so I added one.
> > + {
> > + 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.
Done.
> > + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> > + "Error waiting for process completion
of " \
>
> No need for the line-continuation backslash.
That's gone.
> > + "hook script %s.", cmd);
> > + }
> > +
> > + if (script_output->len > 1)
>
> Why not also if len == 1?
Stupid mistake on my part, 0 is what I wanted. In the interest of even
greater clarity I replaced it with !svn_stringbuf_isempty().
> > + 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.)
Fixed those three formatting errors.
> The rest looks OK but this is only a read-through review, I haven't
tried
> running it.
>
> - Julian
Thanks a lot for the feedback. Here is the latest version which
incorporates your comments, I've also cleaned up a few other things as
well (renamed some variable, got rid of unnecessary pipe for stdout,
etc.).
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.
_____________________________________________________________________________
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Mar 21 18:28:02 2006