Good points...I will rework it tonight.
Thanks!
DJ
Philip Martin wrote:
> 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.
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Aug 28 16:17:17 2003