Justin Erenkrantz wrote:
> --On Friday, November 28, 2003 2:35 PM +0000 Julian Foad
> <julianfoad@btopenworld.com> wrote:
>
>> I believe there are definite mis-uses of both "pool" and "sub-pool" in the
>> function. Here is an abbreviated and annotated version of the function; all
>> comments are mine:
>>
>> copy_versioned_files (..., apr_pool_t *pool)
>> {
>> apr_pool_t *subpool = svn_pool_create (pool);
>>
>> SVN_ERR (svn_wc_adm_probe_open (..., pool)); /* OK: needed
>> throughout */
>> err = svn_wc_entry (..., subpool); /* ### Wrong: needed throughout */
>
> entry is only used once at first - the subpool is indeed safe. Then,
Yes, you are right: that use of "subpool" was safe. Nevertheless, now that we have decided that the sub-pool should be for iteration, I think it is poor style to use it also for this unrelated purpose. Doing so was more space-efficient (because this allocation was cleared at the beginning of iteration), but the correct use of pools is not simple and I don't think this small optimisation justifies the extra complexity.
> look at when we obtain entry in the svn_node_file path with subpool again.
>
>> if (entry)
>> {
>> SVN_ERR (svn_io_stat (&finfo, ..., subpool)); /* Safe (used only
>> once), but might as well go in "pool" */
>
> svn_io_stat is a ridiculous memory hog. It needs to be in the subpool.
I'm not sure what you mean here. As far as I can see, svn_io_stat allocates in the order of 100 bytes. This call is not within the iteration; it is only done once within the body of this function, so once per level of recursion overall.
The pool "pool" passed into copy_versioned_files is intended to be used for allocating (one-off) temporary stuff. Therefore this should go in "pool". If the caller is going to call multiple times, then the caller should provide a sub-pool (and this function, when calling itself in an iteration, does so). I believe this is standard practice and not a problem.
>> SVN_ERR (svn_subst_build_keywords
>> (...,
>> apr_psprintf (pool, ...), /* Safe but
>> should go
>> in "subpool" */
>
> Agreed. That printf is very recent (I had to update my WC to get it),
> so that'll probably explain why it has improper memory usage.
OK, I'll fix that.
> I'm also concerned that adm_access could be used after it is closed in
> copy_versioned_files. Philip? -- justin
Yes. I'll fix that too.
Patch attached for review.
- Julian
Fix access baton lifetime and use pools more consistently.
* subversion/libsvn_client/export.c
(copy_versioned_files): Use the sub-pool only for iteration, and the main
pool for all other allocations.
Rename "subpool" to "iterpool" to make this intention clear.
Do not close the adm_access baton until we have finished using it.
Eliminate the wrong error handling that would have occurred if
svn_wc_adm_close failed.
Index: subversion/libsvn_client/export.c
===================================================================
--- subversion/libsvn_client/export.c (revision 7899)
+++ subversion/libsvn_client/export.c (working copy)
@@ -49,16 +49,13 @@
svn_client_ctx_t *ctx,
apr_pool_t *pool)
{
- apr_pool_t *subpool = svn_pool_create (pool);
- apr_hash_t *dirents;
svn_wc_adm_access_t *adm_access;
const svn_wc_entry_t *entry;
svn_error_t *err;
SVN_ERR (svn_wc_adm_probe_open (&adm_access, NULL, from, FALSE,
FALSE, pool));
- err = svn_wc_entry (&entry, from, adm_access, FALSE, subpool);
- SVN_ERR (svn_wc_adm_close (adm_access));
+ err = svn_wc_entry (&entry, from, adm_access, FALSE, pool);
if (err)
{
if (err->apr_err != SVN_ERR_WC_NOT_DIRECTORY)
@@ -70,15 +67,17 @@
/* We don't want to copy some random non-versioned directory. */
if (entry)
{
+ apr_pool_t *iterpool = svn_pool_create (pool);
+ apr_hash_t *dirents;
apr_hash_index_t *hi;
apr_finfo_t finfo;
- SVN_ERR (svn_io_stat (&finfo, from, APR_FINFO_PROT, subpool));
+ SVN_ERR (svn_io_stat (&finfo, from, APR_FINFO_PROT, pool));
/* Try to make the new directory. If this fails because the
directory already exists, check our FORCE flag to see if we
care. */
- err = svn_io_dir_make (to, finfo.protection, subpool);
+ err = svn_io_dir_make (to, finfo.protection, pool);
if (err)
{
if (! APR_STATUS_IS_EEXIST (err->apr_err))
@@ -119,20 +118,20 @@
}
else
{
- const char *new_from = svn_path_join (from, key, subpool);
- const char *new_to = svn_path_join (to, key, subpool);
+ const char *new_from = svn_path_join (from, key, iterpool);
+ const char *new_to = svn_path_join (to, key, iterpool);
SVN_ERR (copy_versioned_files (new_from, new_to, revision,
- force, ctx, subpool));
+ force, ctx, iterpool));
}
}
else if (*type == svn_node_file)
{
- const char *copy_from = svn_path_join (from, item, subpool);
- const char *copy_to = svn_path_join (to, item, subpool);
+ const char *copy_from = svn_path_join (from, item, iterpool);
+ const char *copy_to = svn_path_join (to, item, iterpool);
err = svn_wc_entry (&entry, copy_from, adm_access, FALSE,
- subpool);
+ iterpool);
if (err)
{
@@ -158,9 +157,9 @@
if (revision->kind != svn_opt_revision_working)
{
SVN_ERR (svn_wc_get_pristine_copy_path
- (copy_from, &base, subpool));
+ (copy_from, &base, iterpool));
SVN_ERR (svn_wc_get_prop_diffs
- (NULL, &props, copy_from, adm_access, subpool));
+ (NULL, &props, copy_from, adm_access, iterpool));
}
else
{
@@ -168,8 +167,8 @@
base = copy_from;
SVN_ERR (svn_wc_prop_list (&props, copy_from,
- adm_access, subpool));
- SVN_ERR (svn_wc_status (&status, copy_from, adm_access, subpool));
+ adm_access, iterpool));
+ SVN_ERR (svn_wc_status (&status, copy_from, adm_access, iterpool));
if (status->text_status != svn_wc_status_normal)
local_mod = TRUE;
}
@@ -189,7 +188,7 @@
if (local_mod)
/* Use the modified time from the working copy if the file */
- SVN_ERR (svn_io_file_affected_time (&time, copy_from, subpool));
+ SVN_ERR (svn_io_file_affected_time (&time, copy_from, iterpool));
else
time = entry->cmt_date;
@@ -215,32 +214,32 @@
SVN_ERR (svn_subst_build_keywords
(&kw, keywords->data,
- apr_psprintf (pool,
+ apr_psprintf (iterpool,
fmt,
entry->cmt_rev),
entry->url,
time,
author,
- subpool));
+ iterpool));
}
SVN_ERR (svn_subst_copy_and_translate (base, copy_to,
eol, FALSE,
&kw, TRUE,
- subpool));
+ iterpool));
if (executable)
- SVN_ERR (svn_io_set_file_executable (copy_to, TRUE, FALSE, subpool));
+ SVN_ERR (svn_io_set_file_executable (copy_to, TRUE, FALSE, iterpool));
- SVN_ERR (svn_io_set_file_affected_time (time, copy_to, subpool));
+ SVN_ERR (svn_io_set_file_affected_time (time, copy_to, iterpool));
}
}
- svn_pool_clear (subpool);
+ svn_pool_clear (iterpool);
}
+ svn_pool_destroy (iterpool);
}
- svn_pool_destroy (subpool);
-
+ SVN_ERR (svn_wc_adm_close (adm_access));
return SVN_NO_ERROR;
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Dec 2 23:41:56 2003