Re: svn commit: r30743 - trunk/subversion/libsvn_client

On Mon, Apr 21, 2008 at 1:16 PM, <kfogel_at_tigris.org> wrote:
> Author: kfogel
> Date: Mon Apr 21 13:16:30 2008
> New Revision: 30743
>
> Log:
> Fix a memory leak in remove recursive propget. See thread starting here:
>
> http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=137529
> From: "Peter Wemm" <peter_at_wemm.org>
> To: dev_at_subversion.tigris.org
> Subject: svn propget -R on the root of a large repository == boom!
> Date: Mon, 21 Apr 2008 02:12:54 -0700
> Message-ID: <e7db6d980804210212h49b2a5a2g5b99093d2ec437a7_at_mail.gmail.com>
>
> * subversion/libsvn_client/prop_commands.c
> (remote_propget): Take perm_pool and work_pool arguments, instead of
> just one pool, and use them to manage memory more efficiently.
>
> Modified:
> trunk/subversion/libsvn_client/prop_commands.c
>
> Modified: trunk/subversion/libsvn_client/prop_commands.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_client/prop_commands.c?pathrev=30743&r1=30742&r2=30743
> ==============================================================================
> --- trunk/subversion/libsvn_client/prop_commands.c Mon Apr 21 12:12:51 2008 (r30742)
> +++ trunk/subversion/libsvn_client/prop_commands.c Mon Apr 21 13:16:30 2008 (r30743)
> @@ -628,7 +628,9 @@ propget_walk_cb(const char *path,
> * KIND is the kind of the node at "TARGET_PREFIX/TARGET_RELATIVE".
> * Yes, caller passes this; it makes the recursion more efficient :-).
> *
> - * Allocate the keys and values in POOL.
> + * Allocate the keys and values in PERM_POOL, but do all temporary
> + * work in WORK_POOL. The two pools can be the same; recursive
> + * calls may use a different WORK_POOL, however.
> */
> static svn_error_t *
> remote_propget(apr_hash_t *props,
> @@ -639,7 +641,8 @@ remote_propget(apr_hash_t *props,
> svn_revnum_t revnum,
> svn_ra_session_t *ra_session,
> svn_depth_t depth,
> - apr_pool_t *pool)
> + apr_pool_t *perm_pool,
> + apr_pool_t *work_pool)
> {
> apr_hash_t *dirents;
> apr_hash_t *prop_hash;
> @@ -649,41 +652,47 @@ remote_propget(apr_hash_t *props,
> SVN_ERR(svn_ra_get_dir2(ra_session,
> (depth >= svn_depth_files ? &dirents : NULL),
> NULL, &prop_hash, target_relative, revnum,
> - SVN_DIRENT_KIND, pool));
> + SVN_DIRENT_KIND, work_pool));
> }
> else if (kind == svn_node_file)
> {
> SVN_ERR(svn_ra_get_file(ra_session, target_relative, revnum,
> - NULL, NULL, &prop_hash, pool));
> + NULL, NULL, &prop_hash, work_pool));
> }
> else if (kind == svn_node_none)
> {
> return svn_error_createf
> (SVN_ERR_ENTRY_NOT_FOUND, NULL,
> _("'%s' does not exist in revision %ld"),
> - svn_path_join(target_prefix, target_relative, pool), revnum);
> + svn_path_join(target_prefix, target_relative, work_pool), revnum);
> }
> else
> {
> return svn_error_createf
> (SVN_ERR_NODE_UNKNOWN_KIND, NULL,
> _("Unknown node kind for '%s'"),
> - svn_path_join(target_prefix, target_relative, pool));
> + svn_path_join(target_prefix, target_relative, work_pool));
> }
>
> - apr_hash_set(props,
> - svn_path_join(target_prefix, target_relative, pool),
> - APR_HASH_KEY_STRING,
> - apr_hash_get(prop_hash, propname, APR_HASH_KEY_STRING));
> -
> + {
> + svn_string_t *val = apr_hash_get(prop_hash, propname,
> + APR_HASH_KEY_STRING);
> + if (val)
> + val = svn_string_dup(val, perm_pool);
> +
> + apr_hash_set(props,
> + svn_path_join(target_prefix, target_relative, perm_pool),
> + APR_HASH_KEY_STRING, val);

Would it make sense to conditionalize the apr_hash_set on val as well,
so that we don't end up allocating a path in perm_pool for every
single path (even those without the prop)?

--dave

-- 
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org