Re: [PATCH] Hooks

dj <dj@shadyvale.net> writes:

> Index: subversion/libsvn_repos/hooks.c
> ===================================================================
> --- subversion/libsvn_repos/hooks.c (revision 6898)
> +++ subversion/libsvn_repos/hooks.c (working copy)
> @@ -50,7 +50,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,8 +62,15 @@
> 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,

This line is too long, it should be split.

> + 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
> @@ -72,6 +79,10 @@
> if (!err && apr_err)
> return svn_error_create
> (apr_err, NULL, "can't close write end of stderr pipe");
> + apr_err = apr_file_close (null_handle);
> + if (!err && apr_err)
> + return svn_error_create
> + (apr_err, NULL, "can't close null stdout file");

If the hook fails (err != 0) we attempt to close write_errhandle,
however if closing write_errhandle fails no attempt is made to close
null_handle. Is there a danger that open files will accumulate?
There is a similar problem if opening null_handle fails.

read_errhandle has a similar problem (not caused by your patch), I
think at one stage in the past the error handling arranged for it to
be closed whatever happened.

I think the error handling in this function needs to revisited.
Either we get rid of the attempts to ensure that files are always
closed, on the basis that they will get closed eventually, or we use a
subpool to ensure that they really are closed.

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