Re: [PATCH] Hooks

"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?

>> 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.

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.

>> return err;
>> }

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org