Re: svn commit: r1002470 - /subversion/trunk/subversion/svnrdump/dump_editor.c

[Daniel Shahaf <d....@daniel.shahaf.name>]

So, you use a top-level pool but never destroy it?

That's not good.

Can you please find another solution?  (Either get the caller to
guarantee something about the lifetime of the pool they provide (there
is precedent for this), or figure out why close_edit() isn't called (and
possibly patch the faulty driver).

artagnon@apache.org wrote on Wed, Sep 29, 2010 at 05:08:42 -0000:
> Author: artagnon
> Date: Wed Sep 29 05:08:42 2010
> New Revision: 1002470
> 
> URL: http://svn.apache.org/viewvc?rev=1002470&view=rev
> Log:
> svnrdump: dump_editor: Fix issues related to per-revision pool
> 
> * subversion/svnrdump/dump_editor.c
> 
>   (open_root): Mention that `eb->pool` is a per-revision pool and
>   clear it in every iteration. Make it a toplevel pool since the
>   lifetime of `pool` is not known.
> 
>   (close_edit): Remove useless svn_pool_destroy cruft since logs show
>   that this function is never actually called.
> 
> Modified:
>     subversion/trunk/subversion/svnrdump/dump_editor.c
> 
> Modified: subversion/trunk/subversion/svnrdump/dump_editor.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svnrdump/dump_editor.c?rev=1002470&r1=1002469&r2=1002470&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/svnrdump/dump_editor.c (original)
> +++ subversion/trunk/subversion/svnrdump/dump_editor.c Wed Sep 29 05:08:42 2010
> @@ -367,10 +367,13 @@ open_root(void *edit_baton,
>            void **root_baton)
>  {
>    struct dump_edit_baton *eb = edit_baton;
> -  /* Allocate a special pool for the edit_baton to avoid pool
> -     lifetime issues */
>  
> -  eb->pool = svn_pool_create(pool);
> +  /* Special toplevel per-revision pool */
> +  if (eb->pool)
> +    svn_pool_clear(eb->pool);
> +  else
> +    eb->pool = svn_pool_create(NULL);
> +
>    eb->props = apr_hash_make(eb->pool);
>    eb->deleted_props = apr_hash_make(eb->pool);
>    eb->propstring = svn_stringbuf_create("", eb->pool);
> @@ -833,10 +836,6 @@ close_file(void *file_baton,
>  static svn_error_t *
>  close_edit(void *edit_baton, apr_pool_t *pool)
>  {
> -  struct dump_edit_baton *eb = edit_baton;
> -  LDR_DBG(("close_edit\n"));
> -  svn_pool_destroy(eb->pool);
> -
>    return SVN_NO_ERROR;
>  }
>  
> 
> 

Re: svn commit: r1002470 - /subversion/trunk/subversion/svnrdump/dump_editor.c

[Ramkumar Ramachandra <ar...@gmail.com>]

Hi Daniel,

Daniel Shahaf writes:
> So, you use a top-level pool but never destroy it?
> 
> That's not good.
> 
> Can you please find another solution?  (Either get the caller to
> guarantee something about the lifetime of the pool they provide (there
> is precedent for this), or figure out why close_edit() isn't called (and
> possibly patch the faulty driver).

Fixed in r1002502 :)

Thanks for the review.

-- Ram