You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by Bert Huijben <be...@qqmail.nl> on 2013/07/25 22:39:23 UTC
RE: svn commit: r1507044 -
/subversion/trunk/subversion/bindings/swig/perl/libsvn_swig_perl/swigutil_pl.c
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
This patch will allocate the string twice, because hash puts evaluates
its argument twice.
This should use a tempvar or the Apr hash function directly From:
rschupp@apache.org
Sent: =E2=80=8E25/=E2=80=8E07/=E2=80=8E2013 18:10
To: commits@subversion.apache.org
Subject: svn commit: r1507044
- /subversion/trunk/subversion/bindings/swig/perl/libsvn_swig_perl/swigutil=
_pl.c
Author: rschupp
Date: Thu Jul 25 16:09:39 2013
New Revision: 1507044
URL: http://svn.apache.org/r1507044
Log:
Fix a Perl-to-APR conversion function.
* subversion/bindings/swig/perl/libsvn_swig_perl/swigutil_pl.c
(svn_swig_pl_to_hash): This function is the universal converter
from "Perl hash of some values" to "APR hash of (converted) values".
It uses hv_iternextsv(H, &KEY, &LEN) to retrieve a Perl=20
hash key. This does _not_ copy the key string, but simply sets KEY
to a pointer into Perl internal memory. svn_hash_sets() also does _not_
copy KEY, so the returned apt_hash_t now contains a pointer into Perl mem=
ory.
If Perl later garbage collects this memory this apt_hash_t key will point
to random bytes and apr_hash_get() for this key will fail.
Hence make a copy of the string and use that for svn_hash_sets().
Modified:
subversion/trunk/subversion/bindings/swig/perl/libsvn_swig_perl/swiguti=
l_pl.c
Modified: subversion/trunk/subversion/bindings/swig/perl/libsvn_swig_perl/s=
wigutil_pl.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/swig=
/perl/libsvn_swig_perl/swigutil_pl.c?rev=3D1507044&r1=3D1507043&r2=3D150704=
4&view=3Ddiff
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D
--- subversion/trunk/subversion/bindings/swig/perl/libsvn_swig_perl/swiguti=
l_pl.c (original)
+++ subversion/trunk/subversion/bindings/swig/perl/libsvn_swig_perl/swiguti=
l_pl.c Thu Jul 25 16:09:39 2013
@@ -187,7 +187,7 @@ static apr_hash_t *svn_swig_pl_to_hash(S
while (cnt--) {
SV* item =3D hv_iternextsv(h, &key, &retlen);
void *val =3D cv(item, ctx, pool);
- svn_hash_sets(hash, key, val);
+ svn_hash_sets(hash, apr_pstrmemdup(pool, key, retlen), val);
}
=20
return hash;
Re: svn commit: r1507044 -
Posted by Ben Reser <be...@reser.org>.
On Thu, Jul 25, 2013 at 2:37 PM, Ben Reser <be...@reser.org> wrote:
> Well looks like we've got quite a bit of cleanup to do then...
>
> [breser@fmri subversion]$ ack 'svn_hash_sets.*apr_' --noheading --nobreak
> svn/notify.c:95: svn_hash_sets(hash,
> apr_pstrdup(nb->conflict_stats->stats_pool, path), "");
> svn/cl-conflicts.c:320: svn_hash_sets(att_hash, "revision",
> apr_ltoa(pool, version->peg_rev));
> bindings/swig/perl/libsvn_swig_perl/swigutil_pl.c:190:
> svn_hash_sets(hash, apr_pstrmemdup(pool, key, retlen), val);
> bindings/swig/ruby/libsvn_swig_ruby/swigutil_rb.c:1529:
> svn_hash_sets(data->apr_hash, apr_pstrdup(data->pool,
> StringValuePtr(key)),
> libsvn_client/prop_commands.c:702: svn_hash_sets(b->props,
> apr_pstrdup(b->pool, local_abspath),
> libsvn_client/commit_util.c:931: svn_hash_sets(danglers,
> apr_pstrdup(result_pool, parent_abspath),
> libsvn_client/locking_commands.c:452: svn_hash_sets(path_tokens,
> path, apr_pstrdup(pool, lock->token));
> libsvn_client/copy_foreign.c:168: svn_hash_sets(db->properties,
> apr_pstrdup(db->pool, name),
> libsvn_client/copy_foreign.c:314: svn_hash_sets(fb->properties,
> apr_pstrdup(fb->pool, name),
> libsvn_client/merge.c:11087: svn_hash_sets(new_catalog,
> apr_pstrdup(scratch_pool, source_path),
> libsvn_client/commit.c:380: svn_hash_sets(wc_items,
> apr_pstrdup(scratch_pool, wcroot_abspath),
> libsvn_subr/mergeinfo.c:1877: svn_hash_sets(*mergeinfo,
> apr_pstrdup(result_pool, path),
> libsvn_subr/mergeinfo.c:2009:
> svn_hash_sets(new_mergeinfo_catalog, apr_pstrdup(pool, key),
> libsvn_subr/mergeinfo.c:2496:
> svn_hash_sets(*adjusted_mergeinfo, apr_pstrdup(result_pool, path),
> libsvn_subr/subst.c:1521: svn_hash_sets(copy,
> apr_pstrdup(result_pool, key),
> libsvn_subr/types.c:328:
> svn_hash_sets(new_entry->changed_paths2, apr_pstrdup(pool, key),
> libsvn_subr/dso.c:95: svn_hash_sets(dso_cache,
> apr_pstrdup(dso_pool, fname), NOT_THERE);
> libsvn_subr/dso.c:101: svn_hash_sets(dso_cache,
> apr_pstrdup(dso_pool, fname), *dso);
> libsvn_repos/log.c:1875: svn_hash_sets(mergeinfo,
> apr_pstrdup(processed_pool, path), ranges);
> libsvn_repos/fs-wrap.c:619: svn_hash_sets(b->locks,
> apr_pstrdup(hash_pool, lock->path),
> libsvn_repos/hooks.c:384: svn_hash_sets(bo->hooks_env,
> apr_pstrdup(result_pool, bo->section),
> libsvn_repos/hooks.c:387: svn_hash_sets(hook_env,
> apr_pstrdup(result_pool, name),
> libsvn_fs_fs/tree.c:3847: svn_hash_sets(result_catalog,
> apr_pstrdup(result_pool, kid_path),
> libsvn_fs_base/tree.c:5302: svn_hash_sets(result_catalog,
> apr_pstrdup(result_pool, path),
> libsvn_wc/diff_editor.c:1552: svn_hash_sets(pb->compared,
> apr_pstrdup(pb->pool, db->name), "");
> libsvn_wc/diff_editor.c:1629: svn_hash_sets(pb->compared,
> apr_pstrdup(pb->pool, db->name), "");
> libsvn_wc/diff_editor.c:1837: svn_hash_sets(pb->compared,
> apr_pstrdup(pb->pool, fb->name), "");
> libsvn_wc/diff_editor.c:1910: svn_hash_sets(pb->compared,
> apr_pstrdup(pb->pool, fb->name), "");
> libsvn_wc/status.c:1562: svn_hash_sets(stat_hash, apr_pstrdup(hash_pool, path),
> libsvn_wc/status.c:1645: svn_hash_sets(statushash,
> apr_pstrdup(pool, local_abspath), statstruct);
> libsvn_wc/wc_db.c:8647: svn_hash_sets(nodes,
> apr_pstrdup(result_pool, name), child);
> libsvn_wc/wc_db.c:8730: svn_hash_sets(conflicts,
> apr_pstrdup(result_pool, name), "");
> libsvn_wc/wc_db.c:8972: svn_hash_sets(*nodes,
> apr_pstrdup(result_pool, name), child);
> libsvn_wc/workqueue.c:1658: svn_hash_sets(wqb->record_map,
> apr_pstrdup(wqb->result_pool, local_abspath),
> libsvn_wc/upgrade.c:205: svn_hash_sets(*all_wcprops,
> apr_pstrdup(result_pool, name), wcprops);
> libsvn_wc/update_editor.c:1857:
> svn_hash_sets(pb->deletion_conflicts, apr_pstrdup(pb->pool, base),
> libsvn_wc/update_editor.c:3169:
> svn_hash_sets(pb->not_present_files, apr_pstrdup(pb->pool, fb->name),
> libsvn_wc/update_editor.c:3277:
> svn_hash_sets(pb->not_present_files, apr_pstrdup(pb->pool, fb->name),
> libsvn_wc/update_editor.c:3382:
> svn_hash_sets(pb->not_present_files, apr_pstrdup(pb->pool, fb->name),
> libsvn_wc/entries.c:1923: svn_hash_sets(tree_conflicts,
> apr_pstrdup(result_pool, key),
> libsvn_wc/wc_db_wcroot.c:869: svn_hash_sets(db->dir_data,
> apr_pstrdup(db->state_pool, parent_dir),
> libsvn_wc/wc_db_wcroot.c:908: svn_hash_sets(db->dir_data,
> svn__apr_hash_index_key(hi), NULL);
> svnrdump/dump_editor.c:672: svn_hash_sets(pb->deleted_entries,
> apr_pstrdup(pb->eb->pool, path), pb);
> svnrdump/dump_editor.c:903: svn_hash_sets(db->deleted_props,
> apr_pstrdup(db->pool, name), "");
> svnrdump/dump_editor.c:931: svn_hash_sets(fb->deleted_props,
> apr_pstrdup(fb->pool, name), "");
> libsvn_delta/compat.c:1050: svn_hash_sets(changes,
> apr_pstrdup(result_pool, relpath), change);
The preceding may have picked up some false positives since it's ok to
dup the memory in the value variable since it's only evaluated once in
the macro (but probably not since those are usually wrapped on another
line).
The list below have the same problem as well, they didn't show up on
my initial search because they are either split across multiple lines
or do something other than call an apr call. Some of them are far
worse than the simple duplication of memory since they actually do
work to produce the value. I didn't investigate the cases where there
are macros used within the macros. Which happens quite often, but I'm
pretty sure all of those are just raw strings.
There are some macros that wrap svn_hash_sets which may be bad based
on the user of the macro (see libsvn_delta/editor.c:113), at current
this particular one looks ok but it's liable to duplicate unless every
future user is knowledgable about this.
Rather than trying to fix all of this I think we should just convert
svn_hash_sets from a macro to a function that's properly flagged so
that it can be inlined.
libsvn_client/prop_commands.c
620: svn_hash_sets(new_iprop->prop_hash,
633: svn_hash_sets(props,
libsvn_client/mergeinfo.c
408: svn_hash_sets(*mergeinfo_cat,
1610: svn_hash_sets(full_path_mergeinfo,
libsvn_client/commit_util.c
223: svn_hash_sets(committables->by_repository,
libsvn_client/switch.c
72: svn_hash_sets(conflicted_paths,
libsvn_client/iprops.c
220: svn_hash_sets(*wcroot_iprops,
libsvn_client/merge.c
1383: svn_hash_sets(parent_baton->new_tree_conflicts,
5223: svn_hash_sets(result_catalog,
8032: svn_hash_sets(log_baton->operative_children,
10744: svn_hash_sets(log_baton->unmerged_catalog,
11159: svn_hash_sets(new_catalog,
libsvn_client/add.c
565: svn_hash_sets(autoprops_baton->autoprops,
569: svn_hash_sets(pattern_hash,
libsvn_client/update.c
177: svn_hash_sets(conflicted_paths,
libsvn_client/list.c
140: svn_hash_sets(externals,
libsvn_subr/mergeinfo.c
2201: svn_hash_sets(*out_catalog,
2229: svn_hash_sets(*out_mergeinfo,
2405: svn_hash_sets(*filtered_cat,
2450: svn_hash_sets(*filtered_mergeinfo,
libsvn_subr/auth.c
271: svn_hash_sets(auth_baton->creds_cache,
410: svn_hash_sets(auth_baton->creds_cache,
libsvn_ra_svn/client.c
1128: svn_hash_sets(new_iprop->prop_hash,
1363: svn_hash_sets(*catalog, path[0] == '/' ? path + 1
:path, for_path); // this one isn't so bad
libsvn_repos/rev_hunt.c
1209: svn_hash_sets(duplicate_path_revs,
libsvn_fs_base/tree.c
5034: svn_hash_sets(args->result_catalog,
5057: svn_hash_sets(args->children_atop_mergeinfo_trees,
libsvn_wc/diff_editor.c
1613: svn_hash_sets(pb->compared,
1892: svn_hash_sets(pb->compared,
libsvn_wc/wc_db.c
1229: svn_hash_sets(*children,
3580: svn_hash_sets(*externals,
9553: svn_hash_sets(*values,
14885: svn_hash_sets(*lock_tokens,
libsvn_wc/info.c
437: svn_hash_sets(fe_baton->tree_conflicts,
libsvn_wc/upgrade.c
701: svn_hash_sets(*conflicts,
1770: svn_hash_sets(repos_cache,
2264: svn_hash_sets(repos_cache,
libsvn_wc/update_editor.c
278: svn_hash_sets(eb->skipped_trees,
4937: svn_hash_sets(eb->dir_dirents,
4993: svn_hash_sets(eb->dir_dirents,
libsvn_wc/wc_db_wcroot.c
841: svn_hash_sets(db->dir_data,
908: svn_hash_sets(db->dir_data, svn__apr_hash_index_key(hi), NULL);
libsvn_wc/wc_db_update_move.c
2246: svn_hash_sets(src_done,
libsvn_wc/conflicts.c
894: svn_hash_sets(*conflicted_prop_names,
svndumpfilter/svndumpfilter.c
564: svn_hash_sets(pb->dropped_nodes,
842: svn_hash_sets(rb->props,
svnrdump/dump_editor.c
899: svn_hash_sets(db->props,
927: svn_hash_sets(fb->props,
svnrdump/load_editor.c
861: svn_hash_sets(rb->revprop_table,
libsvn_ra_serf/xml.c
614: svn_hash_sets(scan->attrs,
libsvn_ra_serf/mergeinfo.c
112: svn_hash_sets(mergeinfo_ctx->result_catalog,
libsvn_ra_serf/inherited_props.c
183: svn_hash_sets(iprops_ctx->curr_iprop->prop_hash,
libsvn_ra_serf/commit.c
1605: svn_hash_sets(dir->commit->deleted_entries,
2314: svn_hash_sets(ctx->revprop_table,
2317: svn_hash_sets(ctx->revprop_table,
libsvn_ra_serf/update.c
2646: svn_hash_sets(report->lock_path_tokens,
libsvn_fs_util/fs-util.c
217: svn_hash_sets(*output,
libsvn_delta/editor.c
116: svn_hash_sets((editor)->completed_nodes, \
libsvn_delta/compat.c
275: svn_hash_sets(change->props,
Re: svn commit: r1507044 -
Posted by Ben Reser <be...@reser.org>.
On Thu, Jul 25, 2013 at 2:37 PM, Ben Reser <be...@reser.org> wrote:
> Well looks like we've got quite a bit of cleanup to do then...
>
> [breser@fmri subversion]$ ack 'svn_hash_sets.*apr_' --noheading --nobreak
> svn/notify.c:95: svn_hash_sets(hash,
> apr_pstrdup(nb->conflict_stats->stats_pool, path), "");
> svn/cl-conflicts.c:320: svn_hash_sets(att_hash, "revision",
> apr_ltoa(pool, version->peg_rev));
> bindings/swig/perl/libsvn_swig_perl/swigutil_pl.c:190:
> svn_hash_sets(hash, apr_pstrmemdup(pool, key, retlen), val);
> bindings/swig/ruby/libsvn_swig_ruby/swigutil_rb.c:1529:
> svn_hash_sets(data->apr_hash, apr_pstrdup(data->pool,
> StringValuePtr(key)),
> libsvn_client/prop_commands.c:702: svn_hash_sets(b->props,
> apr_pstrdup(b->pool, local_abspath),
> libsvn_client/commit_util.c:931: svn_hash_sets(danglers,
> apr_pstrdup(result_pool, parent_abspath),
> libsvn_client/locking_commands.c:452: svn_hash_sets(path_tokens,
> path, apr_pstrdup(pool, lock->token));
> libsvn_client/copy_foreign.c:168: svn_hash_sets(db->properties,
> apr_pstrdup(db->pool, name),
> libsvn_client/copy_foreign.c:314: svn_hash_sets(fb->properties,
> apr_pstrdup(fb->pool, name),
> libsvn_client/merge.c:11087: svn_hash_sets(new_catalog,
> apr_pstrdup(scratch_pool, source_path),
> libsvn_client/commit.c:380: svn_hash_sets(wc_items,
> apr_pstrdup(scratch_pool, wcroot_abspath),
> libsvn_subr/mergeinfo.c:1877: svn_hash_sets(*mergeinfo,
> apr_pstrdup(result_pool, path),
> libsvn_subr/mergeinfo.c:2009:
> svn_hash_sets(new_mergeinfo_catalog, apr_pstrdup(pool, key),
> libsvn_subr/mergeinfo.c:2496:
> svn_hash_sets(*adjusted_mergeinfo, apr_pstrdup(result_pool, path),
> libsvn_subr/subst.c:1521: svn_hash_sets(copy,
> apr_pstrdup(result_pool, key),
> libsvn_subr/types.c:328:
> svn_hash_sets(new_entry->changed_paths2, apr_pstrdup(pool, key),
> libsvn_subr/dso.c:95: svn_hash_sets(dso_cache,
> apr_pstrdup(dso_pool, fname), NOT_THERE);
> libsvn_subr/dso.c:101: svn_hash_sets(dso_cache,
> apr_pstrdup(dso_pool, fname), *dso);
> libsvn_repos/log.c:1875: svn_hash_sets(mergeinfo,
> apr_pstrdup(processed_pool, path), ranges);
> libsvn_repos/fs-wrap.c:619: svn_hash_sets(b->locks,
> apr_pstrdup(hash_pool, lock->path),
> libsvn_repos/hooks.c:384: svn_hash_sets(bo->hooks_env,
> apr_pstrdup(result_pool, bo->section),
> libsvn_repos/hooks.c:387: svn_hash_sets(hook_env,
> apr_pstrdup(result_pool, name),
> libsvn_fs_fs/tree.c:3847: svn_hash_sets(result_catalog,
> apr_pstrdup(result_pool, kid_path),
> libsvn_fs_base/tree.c:5302: svn_hash_sets(result_catalog,
> apr_pstrdup(result_pool, path),
> libsvn_wc/diff_editor.c:1552: svn_hash_sets(pb->compared,
> apr_pstrdup(pb->pool, db->name), "");
> libsvn_wc/diff_editor.c:1629: svn_hash_sets(pb->compared,
> apr_pstrdup(pb->pool, db->name), "");
> libsvn_wc/diff_editor.c:1837: svn_hash_sets(pb->compared,
> apr_pstrdup(pb->pool, fb->name), "");
> libsvn_wc/diff_editor.c:1910: svn_hash_sets(pb->compared,
> apr_pstrdup(pb->pool, fb->name), "");
> libsvn_wc/status.c:1562: svn_hash_sets(stat_hash, apr_pstrdup(hash_pool, path),
> libsvn_wc/status.c:1645: svn_hash_sets(statushash,
> apr_pstrdup(pool, local_abspath), statstruct);
> libsvn_wc/wc_db.c:8647: svn_hash_sets(nodes,
> apr_pstrdup(result_pool, name), child);
> libsvn_wc/wc_db.c:8730: svn_hash_sets(conflicts,
> apr_pstrdup(result_pool, name), "");
> libsvn_wc/wc_db.c:8972: svn_hash_sets(*nodes,
> apr_pstrdup(result_pool, name), child);
> libsvn_wc/workqueue.c:1658: svn_hash_sets(wqb->record_map,
> apr_pstrdup(wqb->result_pool, local_abspath),
> libsvn_wc/upgrade.c:205: svn_hash_sets(*all_wcprops,
> apr_pstrdup(result_pool, name), wcprops);
> libsvn_wc/update_editor.c:1857:
> svn_hash_sets(pb->deletion_conflicts, apr_pstrdup(pb->pool, base),
> libsvn_wc/update_editor.c:3169:
> svn_hash_sets(pb->not_present_files, apr_pstrdup(pb->pool, fb->name),
> libsvn_wc/update_editor.c:3277:
> svn_hash_sets(pb->not_present_files, apr_pstrdup(pb->pool, fb->name),
> libsvn_wc/update_editor.c:3382:
> svn_hash_sets(pb->not_present_files, apr_pstrdup(pb->pool, fb->name),
> libsvn_wc/entries.c:1923: svn_hash_sets(tree_conflicts,
> apr_pstrdup(result_pool, key),
> libsvn_wc/wc_db_wcroot.c:869: svn_hash_sets(db->dir_data,
> apr_pstrdup(db->state_pool, parent_dir),
> libsvn_wc/wc_db_wcroot.c:908: svn_hash_sets(db->dir_data,
> svn__apr_hash_index_key(hi), NULL);
> svnrdump/dump_editor.c:672: svn_hash_sets(pb->deleted_entries,
> apr_pstrdup(pb->eb->pool, path), pb);
> svnrdump/dump_editor.c:903: svn_hash_sets(db->deleted_props,
> apr_pstrdup(db->pool, name), "");
> svnrdump/dump_editor.c:931: svn_hash_sets(fb->deleted_props,
> apr_pstrdup(fb->pool, name), "");
> libsvn_delta/compat.c:1050: svn_hash_sets(changes,
> apr_pstrdup(result_pool, relpath), change);
The preceding may have picked up some false positives since it's ok to
dup the memory in the value variable since it's only evaluated once in
the macro (but probably not since those are usually wrapped on another
line).
The list below have the same problem as well, they didn't show up on
my initial search because they are either split across multiple lines
or do something other than call an apr call. Some of them are far
worse than the simple duplication of memory since they actually do
work to produce the value. I didn't investigate the cases where there
are macros used within the macros. Which happens quite often, but I'm
pretty sure all of those are just raw strings.
There are some macros that wrap svn_hash_sets which may be bad based
on the user of the macro (see libsvn_delta/editor.c:113), at current
this particular one looks ok but it's liable to duplicate unless every
future user is knowledgable about this.
Rather than trying to fix all of this I think we should just convert
svn_hash_sets from a macro to a function that's properly flagged so
that it can be inlined.
libsvn_client/prop_commands.c
620: svn_hash_sets(new_iprop->prop_hash,
633: svn_hash_sets(props,
libsvn_client/mergeinfo.c
408: svn_hash_sets(*mergeinfo_cat,
1610: svn_hash_sets(full_path_mergeinfo,
libsvn_client/commit_util.c
223: svn_hash_sets(committables->by_repository,
libsvn_client/switch.c
72: svn_hash_sets(conflicted_paths,
libsvn_client/iprops.c
220: svn_hash_sets(*wcroot_iprops,
libsvn_client/merge.c
1383: svn_hash_sets(parent_baton->new_tree_conflicts,
5223: svn_hash_sets(result_catalog,
8032: svn_hash_sets(log_baton->operative_children,
10744: svn_hash_sets(log_baton->unmerged_catalog,
11159: svn_hash_sets(new_catalog,
libsvn_client/add.c
565: svn_hash_sets(autoprops_baton->autoprops,
569: svn_hash_sets(pattern_hash,
libsvn_client/update.c
177: svn_hash_sets(conflicted_paths,
libsvn_client/list.c
140: svn_hash_sets(externals,
libsvn_subr/mergeinfo.c
2201: svn_hash_sets(*out_catalog,
2229: svn_hash_sets(*out_mergeinfo,
2405: svn_hash_sets(*filtered_cat,
2450: svn_hash_sets(*filtered_mergeinfo,
libsvn_subr/auth.c
271: svn_hash_sets(auth_baton->creds_cache,
410: svn_hash_sets(auth_baton->creds_cache,
libsvn_ra_svn/client.c
1128: svn_hash_sets(new_iprop->prop_hash,
1363: svn_hash_sets(*catalog, path[0] == '/' ? path + 1
:path, for_path); // this one isn't so bad
libsvn_repos/rev_hunt.c
1209: svn_hash_sets(duplicate_path_revs,
libsvn_fs_base/tree.c
5034: svn_hash_sets(args->result_catalog,
5057: svn_hash_sets(args->children_atop_mergeinfo_trees,
libsvn_wc/diff_editor.c
1613: svn_hash_sets(pb->compared,
1892: svn_hash_sets(pb->compared,
libsvn_wc/wc_db.c
1229: svn_hash_sets(*children,
3580: svn_hash_sets(*externals,
9553: svn_hash_sets(*values,
14885: svn_hash_sets(*lock_tokens,
libsvn_wc/info.c
437: svn_hash_sets(fe_baton->tree_conflicts,
libsvn_wc/upgrade.c
701: svn_hash_sets(*conflicts,
1770: svn_hash_sets(repos_cache,
2264: svn_hash_sets(repos_cache,
libsvn_wc/update_editor.c
278: svn_hash_sets(eb->skipped_trees,
4937: svn_hash_sets(eb->dir_dirents,
4993: svn_hash_sets(eb->dir_dirents,
libsvn_wc/wc_db_wcroot.c
841: svn_hash_sets(db->dir_data,
908: svn_hash_sets(db->dir_data, svn__apr_hash_index_key(hi), NULL);
libsvn_wc/wc_db_update_move.c
2246: svn_hash_sets(src_done,
libsvn_wc/conflicts.c
894: svn_hash_sets(*conflicted_prop_names,
svndumpfilter/svndumpfilter.c
564: svn_hash_sets(pb->dropped_nodes,
842: svn_hash_sets(rb->props,
svnrdump/dump_editor.c
899: svn_hash_sets(db->props,
927: svn_hash_sets(fb->props,
svnrdump/load_editor.c
861: svn_hash_sets(rb->revprop_table,
libsvn_ra_serf/xml.c
614: svn_hash_sets(scan->attrs,
libsvn_ra_serf/mergeinfo.c
112: svn_hash_sets(mergeinfo_ctx->result_catalog,
libsvn_ra_serf/inherited_props.c
183: svn_hash_sets(iprops_ctx->curr_iprop->prop_hash,
libsvn_ra_serf/commit.c
1605: svn_hash_sets(dir->commit->deleted_entries,
2314: svn_hash_sets(ctx->revprop_table,
2317: svn_hash_sets(ctx->revprop_table,
libsvn_ra_serf/update.c
2646: svn_hash_sets(report->lock_path_tokens,
libsvn_fs_util/fs-util.c
217: svn_hash_sets(*output,
libsvn_delta/editor.c
116: svn_hash_sets((editor)->completed_nodes, \
libsvn_delta/compat.c
275: svn_hash_sets(change->props,
Re: svn commit: r1507044 -
Posted by Ben Reser <be...@reser.org>.
On Thu, Jul 25, 2013 at 1:39 PM, Bert Huijben <be...@qqmail.nl> wrote:
> This patch will allocate the string twice, because hash puts evaluates
> its argument twice.
>
> This should use a tempvar or the Apr hash function directly
Well looks like we've got quite a bit of cleanup to do then...
[breser@fmri subversion]$ ack 'svn_hash_sets.*apr_' --noheading --nobreak
svn/notify.c:95: svn_hash_sets(hash,
apr_pstrdup(nb->conflict_stats->stats_pool, path), "");
svn/cl-conflicts.c:320: svn_hash_sets(att_hash, "revision",
apr_ltoa(pool, version->peg_rev));
bindings/swig/perl/libsvn_swig_perl/swigutil_pl.c:190:
svn_hash_sets(hash, apr_pstrmemdup(pool, key, retlen), val);
bindings/swig/ruby/libsvn_swig_ruby/swigutil_rb.c:1529:
svn_hash_sets(data->apr_hash, apr_pstrdup(data->pool,
StringValuePtr(key)),
libsvn_client/prop_commands.c:702: svn_hash_sets(b->props,
apr_pstrdup(b->pool, local_abspath),
libsvn_client/commit_util.c:931: svn_hash_sets(danglers,
apr_pstrdup(result_pool, parent_abspath),
libsvn_client/locking_commands.c:452: svn_hash_sets(path_tokens,
path, apr_pstrdup(pool, lock->token));
libsvn_client/copy_foreign.c:168: svn_hash_sets(db->properties,
apr_pstrdup(db->pool, name),
libsvn_client/copy_foreign.c:314: svn_hash_sets(fb->properties,
apr_pstrdup(fb->pool, name),
libsvn_client/merge.c:11087: svn_hash_sets(new_catalog,
apr_pstrdup(scratch_pool, source_path),
libsvn_client/commit.c:380: svn_hash_sets(wc_items,
apr_pstrdup(scratch_pool, wcroot_abspath),
libsvn_subr/mergeinfo.c:1877: svn_hash_sets(*mergeinfo,
apr_pstrdup(result_pool, path),
libsvn_subr/mergeinfo.c:2009:
svn_hash_sets(new_mergeinfo_catalog, apr_pstrdup(pool, key),
libsvn_subr/mergeinfo.c:2496:
svn_hash_sets(*adjusted_mergeinfo, apr_pstrdup(result_pool, path),
libsvn_subr/subst.c:1521: svn_hash_sets(copy,
apr_pstrdup(result_pool, key),
libsvn_subr/types.c:328:
svn_hash_sets(new_entry->changed_paths2, apr_pstrdup(pool, key),
libsvn_subr/dso.c:95: svn_hash_sets(dso_cache,
apr_pstrdup(dso_pool, fname), NOT_THERE);
libsvn_subr/dso.c:101: svn_hash_sets(dso_cache,
apr_pstrdup(dso_pool, fname), *dso);
libsvn_repos/log.c:1875: svn_hash_sets(mergeinfo,
apr_pstrdup(processed_pool, path), ranges);
libsvn_repos/fs-wrap.c:619: svn_hash_sets(b->locks,
apr_pstrdup(hash_pool, lock->path),
libsvn_repos/hooks.c:384: svn_hash_sets(bo->hooks_env,
apr_pstrdup(result_pool, bo->section),
libsvn_repos/hooks.c:387: svn_hash_sets(hook_env,
apr_pstrdup(result_pool, name),
libsvn_fs_fs/tree.c:3847: svn_hash_sets(result_catalog,
apr_pstrdup(result_pool, kid_path),
libsvn_fs_base/tree.c:5302: svn_hash_sets(result_catalog,
apr_pstrdup(result_pool, path),
libsvn_wc/diff_editor.c:1552: svn_hash_sets(pb->compared,
apr_pstrdup(pb->pool, db->name), "");
libsvn_wc/diff_editor.c:1629: svn_hash_sets(pb->compared,
apr_pstrdup(pb->pool, db->name), "");
libsvn_wc/diff_editor.c:1837: svn_hash_sets(pb->compared,
apr_pstrdup(pb->pool, fb->name), "");
libsvn_wc/diff_editor.c:1910: svn_hash_sets(pb->compared,
apr_pstrdup(pb->pool, fb->name), "");
libsvn_wc/status.c:1562: svn_hash_sets(stat_hash, apr_pstrdup(hash_pool, path),
libsvn_wc/status.c:1645: svn_hash_sets(statushash,
apr_pstrdup(pool, local_abspath), statstruct);
libsvn_wc/wc_db.c:8647: svn_hash_sets(nodes,
apr_pstrdup(result_pool, name), child);
libsvn_wc/wc_db.c:8730: svn_hash_sets(conflicts,
apr_pstrdup(result_pool, name), "");
libsvn_wc/wc_db.c:8972: svn_hash_sets(*nodes,
apr_pstrdup(result_pool, name), child);
libsvn_wc/workqueue.c:1658: svn_hash_sets(wqb->record_map,
apr_pstrdup(wqb->result_pool, local_abspath),
libsvn_wc/upgrade.c:205: svn_hash_sets(*all_wcprops,
apr_pstrdup(result_pool, name), wcprops);
libsvn_wc/update_editor.c:1857:
svn_hash_sets(pb->deletion_conflicts, apr_pstrdup(pb->pool, base),
libsvn_wc/update_editor.c:3169:
svn_hash_sets(pb->not_present_files, apr_pstrdup(pb->pool, fb->name),
libsvn_wc/update_editor.c:3277:
svn_hash_sets(pb->not_present_files, apr_pstrdup(pb->pool, fb->name),
libsvn_wc/update_editor.c:3382:
svn_hash_sets(pb->not_present_files, apr_pstrdup(pb->pool, fb->name),
libsvn_wc/entries.c:1923: svn_hash_sets(tree_conflicts,
apr_pstrdup(result_pool, key),
libsvn_wc/wc_db_wcroot.c:869: svn_hash_sets(db->dir_data,
apr_pstrdup(db->state_pool, parent_dir),
libsvn_wc/wc_db_wcroot.c:908: svn_hash_sets(db->dir_data,
svn__apr_hash_index_key(hi), NULL);
svnrdump/dump_editor.c:672: svn_hash_sets(pb->deleted_entries,
apr_pstrdup(pb->eb->pool, path), pb);
svnrdump/dump_editor.c:903: svn_hash_sets(db->deleted_props,
apr_pstrdup(db->pool, name), "");
svnrdump/dump_editor.c:931: svn_hash_sets(fb->deleted_props,
apr_pstrdup(fb->pool, name), "");
libsvn_delta/compat.c:1050: svn_hash_sets(changes,
apr_pstrdup(result_pool, relpath), change);
Re: svn commit: r1507044 -
Posted by Ben Reser <be...@reser.org>.
On Thu, Jul 25, 2013 at 1:39 PM, Bert Huijben <be...@qqmail.nl> wrote:
> This patch will allocate the string twice, because hash puts evaluates
> its argument twice.
>
> This should use a tempvar or the Apr hash function directly
Well looks like we've got quite a bit of cleanup to do then...
[breser@fmri subversion]$ ack 'svn_hash_sets.*apr_' --noheading --nobreak
svn/notify.c:95: svn_hash_sets(hash,
apr_pstrdup(nb->conflict_stats->stats_pool, path), "");
svn/cl-conflicts.c:320: svn_hash_sets(att_hash, "revision",
apr_ltoa(pool, version->peg_rev));
bindings/swig/perl/libsvn_swig_perl/swigutil_pl.c:190:
svn_hash_sets(hash, apr_pstrmemdup(pool, key, retlen), val);
bindings/swig/ruby/libsvn_swig_ruby/swigutil_rb.c:1529:
svn_hash_sets(data->apr_hash, apr_pstrdup(data->pool,
StringValuePtr(key)),
libsvn_client/prop_commands.c:702: svn_hash_sets(b->props,
apr_pstrdup(b->pool, local_abspath),
libsvn_client/commit_util.c:931: svn_hash_sets(danglers,
apr_pstrdup(result_pool, parent_abspath),
libsvn_client/locking_commands.c:452: svn_hash_sets(path_tokens,
path, apr_pstrdup(pool, lock->token));
libsvn_client/copy_foreign.c:168: svn_hash_sets(db->properties,
apr_pstrdup(db->pool, name),
libsvn_client/copy_foreign.c:314: svn_hash_sets(fb->properties,
apr_pstrdup(fb->pool, name),
libsvn_client/merge.c:11087: svn_hash_sets(new_catalog,
apr_pstrdup(scratch_pool, source_path),
libsvn_client/commit.c:380: svn_hash_sets(wc_items,
apr_pstrdup(scratch_pool, wcroot_abspath),
libsvn_subr/mergeinfo.c:1877: svn_hash_sets(*mergeinfo,
apr_pstrdup(result_pool, path),
libsvn_subr/mergeinfo.c:2009:
svn_hash_sets(new_mergeinfo_catalog, apr_pstrdup(pool, key),
libsvn_subr/mergeinfo.c:2496:
svn_hash_sets(*adjusted_mergeinfo, apr_pstrdup(result_pool, path),
libsvn_subr/subst.c:1521: svn_hash_sets(copy,
apr_pstrdup(result_pool, key),
libsvn_subr/types.c:328:
svn_hash_sets(new_entry->changed_paths2, apr_pstrdup(pool, key),
libsvn_subr/dso.c:95: svn_hash_sets(dso_cache,
apr_pstrdup(dso_pool, fname), NOT_THERE);
libsvn_subr/dso.c:101: svn_hash_sets(dso_cache,
apr_pstrdup(dso_pool, fname), *dso);
libsvn_repos/log.c:1875: svn_hash_sets(mergeinfo,
apr_pstrdup(processed_pool, path), ranges);
libsvn_repos/fs-wrap.c:619: svn_hash_sets(b->locks,
apr_pstrdup(hash_pool, lock->path),
libsvn_repos/hooks.c:384: svn_hash_sets(bo->hooks_env,
apr_pstrdup(result_pool, bo->section),
libsvn_repos/hooks.c:387: svn_hash_sets(hook_env,
apr_pstrdup(result_pool, name),
libsvn_fs_fs/tree.c:3847: svn_hash_sets(result_catalog,
apr_pstrdup(result_pool, kid_path),
libsvn_fs_base/tree.c:5302: svn_hash_sets(result_catalog,
apr_pstrdup(result_pool, path),
libsvn_wc/diff_editor.c:1552: svn_hash_sets(pb->compared,
apr_pstrdup(pb->pool, db->name), "");
libsvn_wc/diff_editor.c:1629: svn_hash_sets(pb->compared,
apr_pstrdup(pb->pool, db->name), "");
libsvn_wc/diff_editor.c:1837: svn_hash_sets(pb->compared,
apr_pstrdup(pb->pool, fb->name), "");
libsvn_wc/diff_editor.c:1910: svn_hash_sets(pb->compared,
apr_pstrdup(pb->pool, fb->name), "");
libsvn_wc/status.c:1562: svn_hash_sets(stat_hash, apr_pstrdup(hash_pool, path),
libsvn_wc/status.c:1645: svn_hash_sets(statushash,
apr_pstrdup(pool, local_abspath), statstruct);
libsvn_wc/wc_db.c:8647: svn_hash_sets(nodes,
apr_pstrdup(result_pool, name), child);
libsvn_wc/wc_db.c:8730: svn_hash_sets(conflicts,
apr_pstrdup(result_pool, name), "");
libsvn_wc/wc_db.c:8972: svn_hash_sets(*nodes,
apr_pstrdup(result_pool, name), child);
libsvn_wc/workqueue.c:1658: svn_hash_sets(wqb->record_map,
apr_pstrdup(wqb->result_pool, local_abspath),
libsvn_wc/upgrade.c:205: svn_hash_sets(*all_wcprops,
apr_pstrdup(result_pool, name), wcprops);
libsvn_wc/update_editor.c:1857:
svn_hash_sets(pb->deletion_conflicts, apr_pstrdup(pb->pool, base),
libsvn_wc/update_editor.c:3169:
svn_hash_sets(pb->not_present_files, apr_pstrdup(pb->pool, fb->name),
libsvn_wc/update_editor.c:3277:
svn_hash_sets(pb->not_present_files, apr_pstrdup(pb->pool, fb->name),
libsvn_wc/update_editor.c:3382:
svn_hash_sets(pb->not_present_files, apr_pstrdup(pb->pool, fb->name),
libsvn_wc/entries.c:1923: svn_hash_sets(tree_conflicts,
apr_pstrdup(result_pool, key),
libsvn_wc/wc_db_wcroot.c:869: svn_hash_sets(db->dir_data,
apr_pstrdup(db->state_pool, parent_dir),
libsvn_wc/wc_db_wcroot.c:908: svn_hash_sets(db->dir_data,
svn__apr_hash_index_key(hi), NULL);
svnrdump/dump_editor.c:672: svn_hash_sets(pb->deleted_entries,
apr_pstrdup(pb->eb->pool, path), pb);
svnrdump/dump_editor.c:903: svn_hash_sets(db->deleted_props,
apr_pstrdup(db->pool, name), "");
svnrdump/dump_editor.c:931: svn_hash_sets(fb->deleted_props,
apr_pstrdup(fb->pool, name), "");
libsvn_delta/compat.c:1050: svn_hash_sets(changes,
apr_pstrdup(result_pool, relpath), change);
Re: svn commit: r1507044 -
Posted by Ben Reser <be...@reser.org>.
On Fri, Jul 26, 2013 at 12:22 AM, Ben Reser <be...@reser.org> wrote:
> I haven't nominated it for backport yet because I wanted to figure out
> what to do about svn_hash_gets(). But I ended up realizing that there
> isn't a good solution there.
Just realized there's no reason to backport this. 1.8.x uses
APR_HASH_KEY_STRING and not strlen() since this was done after we
branched and never merged over.
Re: svn commit: r1507044 -
Posted by Ben Reser <be...@reser.org>.
On Thu, Jul 25, 2013 at 11:42 PM, Roderich Schupp
<ro...@gmail.com> wrote:
> Thanks for catching this, but with Ben's r1507155 (Fix possible duplication
> of work when using svn_hash_sets() macro)
> there's no change required here, right?
Right, this is a bigger problem than just your change.
I haven't nominated it for backport yet because I wanted to figure out
what to do about svn_hash_gets(). But I ended up realizing that there
isn't a good solution there.
Re: svn commit: r1507044 -
Posted by Roderich Schupp <ro...@gmail.com>.
On Thu, Jul 25, 2013 at 10:39 PM, Bert Huijben <be...@qqmail.nl> wrote:
> This patch will allocate the string twice, because hash puts evaluates
> its argument twice.
>
Thanks for catching this, but with Ben's r1507155 (Fix possible duplication
of work when using svn_hash_sets() macro)
there's no change required here, right?
This whole svn_hash_{gets,sets} business looks like a very dumb idea to me.
Cheers, Roderich