Hyrum K. Wright wrote:
> 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;
>> }
>>
After some discussion on IRC, I've committed this patch, along with a
few tweaks, as r23219.
-Hyrum
Received on Wed Jan 24 22:47:18 2007