On 2 January 2014 at 17:01, <stef...@apache.org> wrote: > Author: stefan2 > Date: Thu Jan 2 14:01:43 2014 > New Revision: 1554807 > > URL: http://svn.apache.org/r1554807 > Log: > Replace the use of svn_fs_node_id + svn_fs_compare_ids with > simple calls to svn_fs_node_relation. > Stefan,
I was looking to one of compiler warnings and noticed unbounded memory usage problem in this commit. See my review inline. [...] > Modified: subversion/trunk/subversion/libsvn_repos/rev_hunt.c > URL: > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/rev_hunt.c?rev=1554807&r1=1554806&r2=1554807&view=diff > ============================================================================== > --- subversion/trunk/subversion/libsvn_repos/rev_hunt.c (original) > +++ subversion/trunk/subversion/libsvn_repos/rev_hunt.c Thu Jan 2 14:01:43 > 2014 > @@ -309,11 +309,11 @@ svn_repos_deleted_rev(svn_fs_t *fs, > apr_pool_t *pool) > { > apr_pool_t *subpool; > - svn_fs_root_t *root, *copy_root; > + svn_fs_root_t *start_root, *root, *copy_root; > const char *copy_path; > svn_revnum_t mid_rev; > - const svn_fs_id_t *start_node_id, *curr_node_id; > - svn_error_t *err; > + svn_node_kind_t kind; > + svn_fs_node_relation_t node_relation; > > /* Validate the revision range. */ > if (! SVN_IS_VALID_REVNUM(start)) > @@ -334,32 +334,19 @@ svn_repos_deleted_rev(svn_fs_t *fs, > } > > /* Ensure path exists in fs at start revision. */ > - SVN_ERR(svn_fs_revision_root(&root, fs, start, pool)); > - err = svn_fs_node_id(&start_node_id, root, path, pool); > - if (err) > + SVN_ERR(svn_fs_revision_root(&start_root, fs, start, pool)); > + SVN_ERR(svn_fs_check_path(&kind, start_root, path, pool)); > + if (kind == svn_node_none) > { > - if (err->apr_err == SVN_ERR_FS_NOT_FOUND) > - { > - /* Path must exist in fs at start rev. */ > - *deleted = SVN_INVALID_REVNUM; > - svn_error_clear(err); > - return SVN_NO_ERROR; > - } > - return svn_error_trace(err); > + /* Path must exist in fs at start rev. */ > + *deleted = SVN_INVALID_REVNUM; > + return SVN_NO_ERROR; > } > > /* Ensure path was deleted at or before end revision. */ > SVN_ERR(svn_fs_revision_root(&root, fs, end, pool)); > - err = svn_fs_node_id(&curr_node_id, root, path, pool); > - if (err && err->apr_err == SVN_ERR_FS_NOT_FOUND) > - { > - svn_error_clear(err); > - } > - else if (err) > - { > - return svn_error_trace(err); > - } > - else > + SVN_ERR(svn_fs_check_path(&kind, root, path, pool)); > + if (kind != svn_node_none) > { > /* path exists in the end node and the end node is equivalent > or otherwise equivalent to the start node. This can mean > @@ -386,8 +373,9 @@ svn_repos_deleted_rev(svn_fs_t *fs, > 5) The start node was deleted and replaced by a node which > it does not share any history with. > */ > - SVN_ERR(svn_fs_node_id(&curr_node_id, root, path, pool)); > - if (svn_fs_compare_ids(start_node_id, curr_node_id) != -1) > + SVN_ERR(svn_fs_node_relation(&node_relation, start_root, path, > + root, path, pool)); > + if (node_relation != svn_fs_node_unrelated) > { > SVN_ERR(svn_fs_closest_copy(©_root, ©_path, root, > path, pool)); > @@ -450,28 +438,23 @@ svn_repos_deleted_rev(svn_fs_t *fs, > > /* Get revision root and node id for mid_rev at that revision. */ > SVN_ERR(svn_fs_revision_root(&root, fs, mid_rev, subpool)); > - err = svn_fs_node_id(&curr_node_id, root, path, subpool); > - > - if (err) > + SVN_ERR(svn_fs_check_path(&kind, root, path, pool)); You're using wrong pool here: POOL instead of SUBPOOL (which is actually iterpool). As result this function consume unbounded amount of memory. > + if (kind == svn_node_none) > { > - if (err->apr_err == SVN_ERR_FS_NOT_FOUND) > - { > - /* Case D: Look lower in the range. */ > - svn_error_clear(err); > - end = mid_rev; > - mid_rev = (start + mid_rev) / 2; > - } > - else > - return svn_error_trace(err); > + /* Case D: Look lower in the range. */ > + end = mid_rev; > + mid_rev = (start + mid_rev) / 2; > } > else > { > /* Determine the relationship between the start node > and the current node. */ > - int cmp = svn_fs_compare_ids(start_node_id, curr_node_id); > + SVN_ERR(svn_fs_node_relation(&node_relation, start_root, path, > + root, path, pool)); Same here. > + if (node_relation != svn_fs_node_unrelated) > SVN_ERR(svn_fs_closest_copy(©_root, ©_path, root, > path, subpool)); > - if (cmp == -1 || > + if (node_relation == svn_fs_node_unrelated || > (copy_root && > (svn_fs_revision_root_revision(copy_root) > start))) > { I've fixed these problems in r1664084 and nominated to 1.9.x branch. --- Ivan Zhakov
