Philip Martin wrote:
> "D.J. Heap" <dj@shadyvale.net> writes:
>
>
>>>Index: subversion/libsvn_repos/hooks.c
>>>===================================================================
>>>--- subversion/libsvn_repos/hooks.c (revision 6898)
>>>+++ subversion/libsvn_repos/hooks.c (working copy)
>>>@@ -42,7 +42,8 @@
>>> program that is to be run. If CHECK_EXITCODE is TRUE then the hook's
>>> exit status will be checked, and if an error occurred the hook's stderr
>>> output will be added to the returned error. If CHECK_EXITCODE is FALSE
>>>- the hook's exit status will be ignored. */
>>>+ the hook's exit status will be ignored. Unclosed files and pipes will
>>>+ be handled at pool cleanup. */
>>> static svn_error_t *
>>> run_hook_cmd (const char *name,
>>> const char *cmd,
>>>@@ -50,7 +51,7 @@
>>> svn_boolean_t check_exitcode,
>>> apr_pool_t *pool)
>>> {
>>>- apr_file_t *read_errhandle, *write_errhandle;
>>>+ apr_file_t *read_errhandle, *write_errhandle, *null_handle;
>>> apr_status_t apr_err;
>>> svn_error_t *err;
>>> int exitcode;
>>>@@ -62,12 +63,20 @@
>>> return svn_error_createf
>>> (apr_err, NULL, "can't create pipe for '%s' hook", cmd);
>>> + /* Redirect stdout to the null device */
>>>+ apr_err = apr_file_open (&null_handle, SVN_NULL_DEVICE_NAME, APR_WRITE,
>>>+ APR_OS_DEFAULT, pool);
>>>+ if (apr_err)
>>>+ return svn_error_createf
>>>+ (apr_err, NULL, "can't create null stdout for '%s' hook", cmd);
>>>+
>>> err = svn_io_run_cmd (".", cmd, args, &exitcode, &exitwhy, FALSE,
>>>- NULL, NULL, write_errhandle, pool);
>>>+ NULL, null_handle, write_errhandle, pool);
>>> /* 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. */
>>>+ not support those parameters. We need to close the write handle
>>>+ so we don't hang if we need to read it later. */
>
>
> I don't understand the comment, why would one read from a closed write
> handle? Do you mean that reading from read_handle will hang if
> write_handle is open?
>
Right, I should make that clearer.
>
>>> apr_err = apr_file_close (write_errhandle);
>>> if (!err && apr_err)
>>> return svn_error_create
>>>@@ -97,13 +106,6 @@
>>> }
>>> }
>>> - /* 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 */
>>>- apr_err = apr_file_close (read_errhandle);
>>>- if (!err && apr_err)
>>>- return svn_error_create
>>>- (apr_err, NULL, "can't close read end of stdout pipe");
>>>-
>
>
> I suppose you are relying on pool cleanup to close this? When is the
> pool cleared? As far as I can tell the the txn->pool is being used.
> Is it OK to leave these files open that long? At the very least
> run_hook_cmd ought to be document this behaviour.
I did add the 'unclosed files and pipes will be handled at pool cleanup'
(which is probably not clear enough about exactly what is left around),
and each hook is called only once on a commit, so it's not going to
leave much around for long. But since they are basically temporary for
this function, it does seem cleaner as you outlined below (and follows
the HACKING guidelines better, as I understand them).
I'm wondering, though, how error conditions are supposed to be handled
-- in some areas it seems error conditions mean it's ok to exit without
explicit cleanup -- ie, creating a subpool for temp stuff but then using
the SVN_ERR macro for error handling (in libsvn_repos/log.c the
detect_changed function, for example). But in other areas, error
handling is more explicit and cleanup of temps is done as much as
possible before exiting -- like this function used to be (which tends to
make the code messy as you noted with the previous patch).
Is the more explicit cleanup desired in this area because it's dealing
with pipes and files, or was it just the way it was done at the time or
something? Is it ok to create a subpool and use SVN_ERR to bail on
error conditions, or should I explicitly clear it before exiting if any
errors are encountered so that the files/pipes are guaranteed closed
even in the face of errors?
I'm not trying to be a bother, just clarify my understanding of how
things should be done.
Thanks!
DJ
>
> When I suggested leaving it to pool cleanup I had in mind something
> like
>
> run_hook(...)
> {
> sub_pool = svn_pool_create(...);
> err = do_run_hook(... subpool);
> svn_pool_destroy(sub_pool);
> return err;
> }
>
> so that the lifetime of the open files was precisely defined.
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Sep 5 18:16:07 2003