Date: Wed, 21 Nov 2018 14:55:23 +0000
From: Daniel Shahaf <d...@daniel.shahaf.name>
To: dev@subversion.apache.org
Subject: [PATCH] Re: [PATCH] A test for "Can't get entries" error
Message-ID: <20181121145523.kgwfhlrhiffm5ru7@tarpaulin.shahaf.local2>
Daniel Shahaf wrote on Tue, Nov 20, 2018 at 09:28:19 +0000:
Julian Foad wrote on Tue, 20 Nov 2018 08:38 +0000:
Daniel Shahaf wrote:
Could you please clarify whether the bug reproduces under other backends
(FSX and BDB)?
I found that the test passes under FSX and BDB; it only fails under FSFS.
The test XPASSes with SVN_X_DOES_NOT_MARK_THE_SPOT=1 (see notes/knobs),
so it's something to do with the caches.
So, looking at subversion/libsvn_fs_fs/tree.c:
1076 /* If we found a directory entry, follow it. First, we
1077 check our node cache, and, failing that, we hit the DAG
1078 layer. Don't bother to contact the cache for the last
1079 element if we already know the lookup to fail for the
1080 complete path. */
1081 if (next || !(flags & open_path_uncached))
1082 SVN_ERR(dag_node_cache_get(&cached_node, root,
path_so_far->data,
1083 pool));
1084 if (cached_node)
1085 child = cached_node;
1086 else
1087 SVN_ERR(svn_fs_fs__dag_open(&child, here, entry, pool,
iterpool));
1088
1089 /* "file not found" requires special handling. */
1090 if (child == NULL)
1091 {
⋮
1103 else if (flags & open_path_allow_null)
1104 {
1105 parent_path = NULL;
1106 break;
1107 }
⋮
1114 }
This is what happens:
[[[
(lldb) breakpoint set -f tree.c -l 1087 -c 'root->rev == 2 && !(int)strcmp(path,
"/B/mu/iota")'
(lldb) r
Process 17504 stopped
* thread #1, name = 'ra-test', stop reason = breakpoint 1.1
frame #0: 0x00007ffff5b8be7a
libsvn_fs_fs-1.so.0`open_path(parent_path_p=0x00007fffffffcb18, root=0x00007ffff7f6c9d8,
path="/B/mu/iota", flags=12, is_txn_path=0, pool=0x00007ffff7f6c028) at
tree.c:1087
1084 if (cached_node)
1085 child = cached_node;
1086 else
-> 1087 SVN_ERR(svn_fs_fs__dag_open(&child, here, entry, pool,
iterpool));
1088
1089 /* "file not found" requires special handling. */
1090 if (child == NULL)
(lldb) p root->is_txn_root
(svn_boolean_t) $2 = 0
(lldb) p root->rev
(svn_revnum_t) $3 = 2
(lldb) p stringify_node(here, pool)
(const char *) $0 = 0x00007ffff7f6cc08 "2.0.r1/0"
(lldb) pla shell svnlook tree --show-ids --full-paths
cant_get_entries_of_non_directory -r2 | fgrep 2.0.r1/0
A/mu <2.0.r1/0>
B/mu <2.0.r1/0>
(lldb) p entry
(char *) $1 = 0x00007ffff7f6cbd8 "iota"
(lldb) n
Process 2969 stopped
* thread #1, name = 'ra-test', stop reason = step over
frame #0: 0x00007ffff5b8c2b2
libsvn_fs_fs-1.so.0`open_path(parent_path_p=0x00007fffffffcb18, root=0x00007ffff7f6c9d8,
path="/B/mu/iota", flags=12, is_txn_path=0, pool=0x00007ffff7f6c028) at
tree.c:1176
1173 svn_pool_destroy(iterpool);
1174 *parent_path_p = parent_path;
1175 return SVN_NO_ERROR;
-> 1176 }
1177
1178
1179 /* Make the node referred to by PARENT_PATH mutable, if it isn't
(lldb)
]]]
In words: svn_fs_fs__dag_open() is asked to find the child "iota" of
r2:/B/mu, and instead of setting 'child' to NULL as the code expects, it
returns an error which percolates all the way to the client.
The reason SVN_X_DOES_NOT_MARK_THE_SPOT fixes it is that it bypasses the
optimization earlier in the function. That optimization causes the the
very first iteration of the loop is to process "/B/mu". With caches
disabled, the first iteration of the loop processes "/" and the second
iteration processes "/B" and exits early, here:
1144 /* The path isn't finished yet; we'd better be in a directory. */
1145 if (svn_fs_fs__dag_node_kind(child) != svn_node_dir)
1146 {
1147 const char *msg;
1148
1149 /* Since this is not a directory and we are looking for some
1150 sub-path, that sub-path will not exist. That will be o.k.,
1151 if we are just here to check for the path's existence. */
1152 if (flags & open_path_allow_null)
1153 {
1154 parent_path = NULL;
1155 break;
1156 }
So, we just need to make the svn_fs_fs__dag_open() call set child=NULL
so it can fall back to the existing logic for handling FLAGS:
[[[
Index: subversion/libsvn_fs_fs/tree.c
===================================================================
--- subversion/libsvn_fs_fs/tree.c (revision 1845259)
+++ subversion/libsvn_fs_fs/tree.c (working copy)
@@ -1083,8 +1083,10 @@
pool));
if (cached_node)
child = cached_node;
- else
- SVN_ERR(svn_fs_fs__dag_open(&child, here, entry, pool, iterpool));
+ else if (svn_fs_fs__dag_node_kind(here) == svn_node_dir)
+ SVN_ERR(svn_fs_fs__dag_open(&child, here, entry, pool, iterpool));
+ else
+ child = NULL;
/* "file not found" requires special handling. */
if (child == NULL)
]]]
Makes sense?
