Discussion:
[Daniel Shahaf: [PATCH] Re: [PATCH] A test for "Can't get entries" error]
Stefan Fuhrmann
2018-12-01 10:08:02 UTC
Permalink
Good morning Stefan,
tl;dr: False positive SVN_ERR_FS_NOT_DIRECTORY error, with test¹,
workaround, analysis, and patch.
The error doesn't happen with caches disabled so I thought you might be
interested.
Cheers,
Daniel
¹ The test was posted upthread by Julian (based on Dmitry's work).
Date: Wed, 21 Nov 2018 14:55:23 +0000
Subject: [PATCH] Re: [PATCH] A test for "Can't get entries" error
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.
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 }
[[[
(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
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
[[[
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?
Yes, that does make sense. Thank you for fixing this.

My mental image was: Stop at the last parent level (re-usable
between siblings) and attempt a leaf lookup, which would then
handle the error cases. This is obviously incomplete for the
case that the parent is not a directory but has been cached.

What I do not understand, yet, is why this has not been
reported earlier and why it is not present in FSX. I plan to dive
into that code tonight and come back with the result.

-- Stefan^2.
Daniel Shahaf
2018-12-02 08:28:24 UTC
Permalink
Post by Stefan Fuhrmann
Yes, that does make sense. Thank you for fixing this.
My mental image was: Stop at the last parent level (re-usable
between siblings) and attempt a leaf lookup, which would then
handle the error cases. This is obviously incomplete for the
case that the parent is not a directory but has been cached.
What I do not understand, yet, is why this has not been
reported earlier and why it is not present in FSX. I plan to dive
into that code tonight and come back with the result.
Thanks, Stefan.

To avoid misunderstanding, please note that I haven't committed this
patch yet, and that kotkov has committed a different fix in r1847572.
I assume the patch I posted is still applicable, though, as it fixes
a (now latent) bug in the for(;;) loop.

Cheers,

Daniel

Loading...