Nick Piper wrote on Mon, May 09, 2011 at 16:19:23 +0100:
> 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?
>
It would make a good docstring.
> >> + 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.
>
Fair enough. I'll run tests too, etc, and commit.
Received on 2011-05-09 17:32:17 CEST