Re: AW: Python bindings: Use of svn_swig_py_make_stream() - avoiding leaking fds

Thanks for reviewing.

On 09/05/11 15:53, Daniel Shahaf wrote:
> Nick Piper wrote on Mon, May 09, 2011 at 15:31:53 +0100:

>> + /* This is used to close the stream if the pool is being destroyed.
>> + It was first introduced for the Python API, to allow implicit
>> + closing.
>> + */
>> +
>
> The comment about history belongs in the log message, not in the code.
> (the opposite of the usual problem :-))

Should we keep the first line of it?

>> + err = svn_stream_close(stream);
>> + if (err)
>> + {
>> + apr_err = err->apr_err;
>> + svn_error_clear(err);
>> + }
>> +
>> + return apr_err;
>> +}
>> +
>> svn_stream_t *
>> svn_stream_create(void *baton, apr_pool_t *pool)
>> {
>> @@ -73,6 +95,9 @@ svn_stream_create(void *baton, apr_pool_t *pool)
>> stream->mark_fn = NULL;
>> stream->seek_fn = NULL;
>> stream->buffered_fn = NULL;
>> + stream->pool = pool;
>> + apr_pool_pre_cleanup_register(stream->pool, stream,
>> + close_stream_cleanup);
>
> Why did you switch to apr_pool_pre_cleanup_register() from
> apr_pool_cleanup_register()?

Otherwise tests segfault, with backtrace:

[...]
#0 0xb6ff7901 in apr_pool_destroy () from /usr/lib/libapr-1.so.0
#1 0xb6ff7934 in apr_pool_destroy () from /usr/lib/libapr-1.so.0
#2 0xb6ff7934 in apr_pool_destroy () from /usr/lib/libapr-1.so.0
[...]
#174381 0xb6ff7934 in apr_pool_destroy () from /usr/lib/libapr-1.so.0
#174382 0xb6ff7934 in apr_pool_destroy () from /usr/lib/libapr-1.so.0
#174383 0xb6ff7844 in apr_pool_clear () from /usr/lib/libapr-1.so.0
#174384 0xb6daca86 in write_final_rev (new_id_p=0xbf8eed20,
file=0x90826e8, rev=4, fs=0x905bfa8, id=0x90b4498, start_node_id=0x0,
    start_copy_id=0x0, initial_offset=231, reps_to_cache=0x909e358,
reps_pool=0x9066f30, pool=0x9101810)
    at subversion/libsvn_fs_fs/fs_fs.c:6050
#174385 0xb6dacaeb in write_final_rev (new_id_p=0xbf8eef40,
file=0x90826e8, rev=4, fs=0x905bfa8, id=0x90842f8, start_node_id=0x0,
    start_copy_id=0x0, initial_offset=231, reps_to_cache=0x909e358,
reps_pool=0x9066f30, pool=0x90b2cd0)
    at subversion/libsvn_fs_fs/fs_fs.c:6051
[...]

I'm not familiar with the pool API, but reading
http://apr.apache.org/docs/apr/1.3/group___pool_cleanup.html#g64114542989d8872c7fd3c62f2a68678
suggests that if we want to use the data that's in the pool during the
cleanup, we have to do that in pre.

-- 
Sorry for this disclaimer:
Think green - keep it on the screen.
This e-mail and any attachment is for authorised use by the intended recipient(s) only. It may contain proprietary material, confidential information and/or be subject to legal privilege. It should not be copied, disclosed to, retained or used by, any other party. If you are not an intended recipient then please promptly delete this e-mail and any attachment and all copies and inform the sender. Thank you.