Paul Burba wrote:
> Julian Foad <julianfoad@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
Received on Wed Mar 22 23:40:45 2006