Daniel Rall wrote:
> On Wed, 24 Jan 2007, Hyrum K. Wright wrote:
>
>> Daniel Rall wrote:
>>> On Tue, 23 Jan 2007, Hyrum K. Wright wrote:
>> ...
>>>> My main concern is that we have a unbounded number of copy_pair_t
>>>> structures which get allocated in setup_copy() from its main pool. They
>>>> are created before we do existence checking, and would not be destroyed
>>>> in the case of an error. We also have other resources, such as ra
>>>> sessions, which don't get cleaned up properly. Those could impact the
>>>> server, as well as the client.
>>>>
>>>> In the typical error case, we don't worry about it, because we assume
>>>> the pool is going to get destroyed quickly anyway. That assumption
>>>> isn't valid in this case.
>>> Sounds like some additional pool juggling wouldn't be out of line,
>>> then. Either that, or restructure the code to handle the
>>> copy_as_child parameter deeper in the call stack.
>> Or just use a subpool for calls to setup_copy(). We could then
>> duplicate *commit_info_t before returning from the client API, and
>> safely destroy the subpool. This approach seems a lot less invasive to me.
>
> Yeah, I like the sound of that. Something like this? We might also
> be able to finagle use of the sub-pool in move5() for allocations of
> some of the temporary data fed to setup_copy() (though how to do that
> with the pool clearing wasn't immediately obivous to me).
Looks good. I can take a look at the move5() subpool usage later today.
> Index: subversion/libsvn_client/copy.c
> ===================================================================
> --- subversion/libsvn_client/copy.c (revision 23212)
> +++ subversion/libsvn_client/copy.c (working copy)
> @@ -1538,17 +1538,19 @@
> apr_pool_t *pool)
> {
> svn_error_t *err;
> + svn_commit_info_t *commit_info;
> + apr_pool_t *subpool = svn_pool_create(pool);
>
> if (sources->nelts > 1 && !copy_as_child)
> return svn_error_create(SVN_ERR_CLIENT_MULTIPLE_SOURCES_DISALLOWED,
> NULL, NULL);
>
> - err = setup_copy(commit_info_p,
> + err = setup_copy(&commit_info,
> sources, dst_path,
> FALSE /* is_move */,
> TRUE /* force, set to avoid deletion check */,
> ctx,
> - pool);
> + subpool);
>
> /* If the destination exists, try to copy the sources as children of the
> destination. */
> @@ -1561,18 +1563,20 @@
> const char *src_basename;
>
> svn_error_clear(err);
> + svn_pool_clear(subpool);
>
> src_basename = svn_path_basename(src_path, pool);
> -
> - err = setup_copy(commit_info_p,
> + err = setup_copy(&commit_info,
> sources,
> svn_path_join(dst_path, src_basename, pool),
> FALSE /* is_move */,
> TRUE /* force, set to avoid deletion check */,
> ctx,
> - pool);
> + subpool);
> }
>
> + *commit_info_p = svn_commit_info_dup(commit_info, pool);
> + svn_pool_destroy(subpool);
> return err;
> }
>
> @@ -1663,10 +1667,12 @@
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
> {
> + svn_commit_info_t *commit_info;
> const svn_opt_revision_t head_revision
> = { svn_opt_revision_head, { 0 } };
> svn_error_t *err;
> int i;
> + apr_pool_t *subpool = svn_pool_create(pool);
> apr_array_header_t *sources = apr_array_make(pool, src_paths->nelts,
> sizeof(const svn_client_copy_source_t *));
>
> @@ -1687,11 +1693,11 @@
> APR_ARRAY_PUSH(sources, svn_client_copy_source_t *) = copy_source;
> }
>
> - err = setup_copy(commit_info_p, sources, dst_path,
> + err = setup_copy(&commit_info, sources, dst_path,
> TRUE /* is_move */,
> force,
> ctx,
> - pool);
> + subpool);
>
> /* If the destination exists, try to move the sources as children of the
> destination. */
> @@ -1703,17 +1709,19 @@
> const char *src_basename;
>
> svn_error_clear(err);
> + svn_pool_clear(subpool);
>
> src_basename = svn_path_basename(src_path, pool);
> -
> - err = setup_copy(commit_info_p, sources,
> + err = setup_copy(&commit_info, sources,
> svn_path_join(dst_path, src_basename, pool),
> TRUE /* is_move */,
> force,
> ctx,
> - pool);
> + subpool);
> }
>
> + *commit_info_p = svn_commit_info_dup(commit_info, pool);
> + svn_pool_destroy(subpool);
> return err;
> }
>
-Hyrum
Received on Wed Jan 24 19:23:34 2007