Daniel Rall wrote:
> On Tue, 23 Jan 2007, Hyrum K. Wright wrote:
>> It still bugs me that we use the pool passed to setup_copy() to
>> allocate unbounded data structures.
>
> Yeah, we use an iterpool allocated from the pool passed to
> setup_copy() for disposable allocations with a loop, but the size of
> the data returned in *commit_info_p is also unbounded. In the case
> where we ignore the first contents of *commit_info_p -- when we're
> okay with copying/moving the sources under the destination (as
> children) -- is it typically already fully populated? Or do we detect
> the "error" condition (the presence of the destination) before
> populating it?
>
>> Would it be possible/appropriate to use a subpool within
>> setup_copy() itself? We could use if for most of the operations,
>> just making sure that we don't allocate commit_info_p with the
>> subpool.
>>
>> Or does in that way madness lie?
>
> If we're actually populating *commit_info_p, then ignoring it and
> re-populating it, IMO it would be okay to pass a scratch pool to
> setup_copy() (as it's not part of the public API, only part of the
> implementation).
We only populate *commit_info_p in the case of a commit, which happens
after we've done destination existence checks. In other words, if there
is an error after populating commit_info_p, it is not one of the ones
that we are checking for in the API.
> I also didn't notice us setting *commit_info_p anywhere when there are
> no commitables (WC -> WC and REPOS -> WC ops, where no commits occur).
> Do we need something like this patch?
The API docs don't make any guarantees as to what becomes *commit_info_p
when it is not used. When combined with improved documentation, this
patch seems reasonable.
> Index: subversion/libsvn_client/copy.c
> ===================================================================
> --- subversion/libsvn_client/copy.c (revision 23190)
> +++ subversion/libsvn_client/copy.c (working copy)
> @@ -1503,6 +1503,7 @@
> /* Now, call the right handler for the operation. */
> if ((! srcs_are_urls) && (! dst_is_url))
> {
> + *commit_info_p = NULL;
> SVN_ERR(wc_to_wc_copy(copy_pairs, is_move,
> ctx, pool));
> }
> @@ -1513,6 +1514,7 @@
> }
> else if ((srcs_are_urls) && (! dst_is_url))
> {
> + *commit_info_p = NULL;
> SVN_ERR(repos_to_wc_copy(copy_pairs, ctx, pool));
> }
> else
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.
-Hyrum
Received on Tue Jan 23 22:59:35 2007