You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Garrett Rooney <ro...@electricjellyfish.net> on 2008/03/13 18:50:28 UTC
[PATCH] fix small memory leak in base_dir_entries
Hey, just ran across this small memory leak in base_dir_entries. It's
been a while since I was in this code (or any Subversion code, for
that matter), so I figured I'd run it by the list before committing.
-garrett
Use an iteration pool when looking up entry kinds in base_dir_entries.
* subversion/libsvn_fs_base/tree.c
(base_dir_entries): Use an iteration pool instead of putting everything
in our main pool, since nothing we allocate in that loop needs to live any
longer than the loop body.
Re: [PATCH] fix small memory leak in base_dir_entries
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On Thu, Mar 13, 2008 at 3:10 PM, C. Michael Pilato <cm...@collab.net> wrote:
> Agree, patch is good. +1 on s/subpool/iterpool/.
>
> Though, it'd be unbearably difficult for me not to fix the "simple" ->
> "simply" typo while committing it.
Ha! Nice. Ok, I'll change to iterpool, fix the typo and commit.
Thanks for the review,
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix small memory leak in base_dir_entries
Posted by "C. Michael Pilato" <cm...@collab.net>.
Karl Fogel wrote:
> "Garrett Rooney" <ro...@electricjellyfish.net> writes:
>> --- subversion/libsvn_fs_base/tree.c (revision 29895)
>> +++ subversion/libsvn_fs_base/tree.c (working copy)
>> @@ -1451,6 +1451,7 @@
>> apr_pool_t *pool)
>> {
>> struct dir_entries_args args;
>> + apr_pool_t *subpool;
>> apr_hash_t *table;
>> svn_fs_t *fs = root->fs;
>> apr_hash_index_t *hi;
>> @@ -1461,6 +1462,8 @@
>> SVN_ERR(svn_fs_base__retry_txn(root->fs, txn_body_dir_entries, &args,
>> pool));
>>
>> + subpool = svn_pool_create(pool);
>> +
>> /* Add in the kind data. */
>> for (hi = apr_hash_first(pool, table); hi; hi = apr_hash_next(hi))
>> {
>> @@ -1468,16 +1471,20 @@
>> struct node_kind_args nk_args;
>> void *val;
>>
>> + svn_pool_clear(subpool);
>> +
>> /* KEY will be the entry name in ancestor (about which we
>> simple don't care), VAL the dirent. */
>> apr_hash_this(hi, NULL, NULL, &val);
>> entry = val;
>> nk_args.id = entry->id;
>> SVN_ERR(svn_fs_base__retry_txn(fs, txn_body_node_kind, &nk_args,
>> - pool));
>> + subpool));
>> entry->kind = nk_args.kind;
>> }
>>
>> + svn_pool_destroy(subpool);
>> +
>> *table_p = table;
>> return SVN_NO_ERROR;
>> }
>
> I think we call it 'iterpool' now as a matter of convention (this bit me
> a few times too, until I caught on to what the young 'uns were doing).
>
> Patch looks good otherwise. I can't see any opportunity for lifetime
> problems, since the only thing we save from the loop is the enum 'kind'.
Agree, patch is good. +1 on s/subpool/iterpool/.
Though, it'd be unbearably difficult for me not to fix the "simple" ->
"simply" typo while committing it.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: [PATCH] fix small memory leak in base_dir_entries
Posted by Karl Fogel <kf...@red-bean.com>.
"Garrett Rooney" <ro...@electricjellyfish.net> writes:
> --- subversion/libsvn_fs_base/tree.c (revision 29895)
> +++ subversion/libsvn_fs_base/tree.c (working copy)
> @@ -1451,6 +1451,7 @@
> apr_pool_t *pool)
> {
> struct dir_entries_args args;
> + apr_pool_t *subpool;
> apr_hash_t *table;
> svn_fs_t *fs = root->fs;
> apr_hash_index_t *hi;
> @@ -1461,6 +1462,8 @@
> SVN_ERR(svn_fs_base__retry_txn(root->fs, txn_body_dir_entries, &args,
> pool));
>
> + subpool = svn_pool_create(pool);
> +
> /* Add in the kind data. */
> for (hi = apr_hash_first(pool, table); hi; hi = apr_hash_next(hi))
> {
> @@ -1468,16 +1471,20 @@
> struct node_kind_args nk_args;
> void *val;
>
> + svn_pool_clear(subpool);
> +
> /* KEY will be the entry name in ancestor (about which we
> simple don't care), VAL the dirent. */
> apr_hash_this(hi, NULL, NULL, &val);
> entry = val;
> nk_args.id = entry->id;
> SVN_ERR(svn_fs_base__retry_txn(fs, txn_body_node_kind, &nk_args,
> - pool));
> + subpool));
> entry->kind = nk_args.kind;
> }
>
> + svn_pool_destroy(subpool);
> +
> *table_p = table;
> return SVN_NO_ERROR;
> }
I think we call it 'iterpool' now as a matter of convention (this bit me
a few times too, until I caught on to what the young 'uns were doing).
Patch looks good otherwise. I can't see any opportunity for lifetime
problems, since the only thing we save from the loop is the enum 'kind'.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org