You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2018/11/21 14:55:23 UTC
[PATCH] Re: [PATCH] A test for "Can't get entries" error
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?
Cheers,
Daniel
SVN-4791 FSFS Can't get entries of non-directory
Posted by Julian Foad <ju...@apache.org>.
(Subject line changed.)
I have filed this issue as https://issues.apache.org/jira/browse/SVN-4791 "FSFS Can't get entries of non-directory".
I have run the test on older versions. On the 1.8.x branch the test passes; from 1.9.0 onwards it fails.
--
- Julian
Re: [PATCH] Re: [PATCH] A test for "Can't get entries" error
Posted by Julian Foad <ju...@apache.org>.
FWIW: I also looked around in FSX to see if there was any comparable code, in case there was some correct version of this pattern or some other clue.
The comparable area of code is svn_fs_x__get_dag_path().
In its loop, it seems the 'here' variable is always supposed to point to a directory; the 'dag_step' call returns an error if not.
- Julian
Re: [PATCH] Re: [PATCH] A test for "Can't get entries" error
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Thu, 22 Nov 2018 00:51 +0000:
> While we're at this function, does anyone understand why directory[1] is
> accessed without checking whether directory[0] is not NUL? There is
> a comment there, but it doesn't enlighten me. (However, I haven't run
> 'blame' on that comment yet.) Even if it's correct, is there any reason
> not to add an SVN_ERR_ASSERT(directory[0]) there?
Sorry, that's not quite the issue. directory[0] is almost certainly '/', and
that's a fundamental enough aspect of canonical paths that we shouldn't
need to assert it everywhere; but I'm still not certain what
.
/* root nodes are covered anyway */
.
means.
Cheers,
Daniel
Re: [PATCH] Re: [PATCH] A test for "Can't get entries" error
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Julian Foad wrote on Fri, 23 Nov 2018 13:56 +0000:
> Daniel Shahaf wrote:
> > Julian Foad wrote on Wed, 21 Nov 2018 16:00 +0000:
> > > In reviewing the code I was unable to keep track of all the nuances of
> > > what happens (and should happen) in all the edge cases. Especially when
> > > 'flags & open_path_allow_null' is true and the requested path is a child
> > > of a non-directory like the "/B/mu/iota" in this case: that combination
> > > doesn't seem to be well documented, which makes me wonder what the
> > > callers expect it to do.
> >
> > Have you read the docstring of the open_path_allow_null enumerator?
>
> Maybe I was thinking of open_path_last_optional. "If FLAGS &
> open_path_last_optional is ... non-zero, require all the parent
> directories to exist as normal ..." when in this case the parent is not
> a directory.
Ah, yes, I see. The docstring has a lacuna.
By code inspection, all three callsites that pass
open_path_last_optional expect the following postcondition:
.
(*parent_path_p)->node != NULL || svn_fs_fs__dag_node_kind((*parent_path_p)->parent->node) == svn_node_dir
.
which agrees with open_path() of r2:/B/mu/iota returning
SVN_ERR_FS_NOT_DIRECTORY.
That said, that postcondition doesn't seem to be a good API: the two
possible outcomes are very different from each other, to the point that
every single caller branches on them and handles them differently. (The
function does not "do one thing well".) If a caller of open_path(open_path_last_optional)
forgets to check whether parent->node is NULL or not, we'll probably
have a bug (hopefully nothing worse than a segfault or an error from the
DAG layer). I don't see why we couldn't remove open_path_allow_null
entirely and have callsites that pass it just call open_path(..., dirname(path), ...)
and do the existence check on the child's basename explicitly. The case
that 'path' exists is an error anyway.
All that said, the callers' code does appear to be correct, in a
quick skim.
Cheers,
Daniel
Re: [PATCH] Re: [PATCH] A test for "Can't get entries" error
Posted by Julian Foad <ju...@apache.org>.
Daniel Shahaf wrote:
> Julian Foad wrote on Wed, 21 Nov 2018 16:00 +0000:
> > In reviewing the code I was unable to keep track of all the nuances of
> > what happens (and should happen) in all the edge cases. Especially when
> > 'flags & open_path_allow_null' is true and the requested path is a child
> > of a non-directory like the "/B/mu/iota" in this case: that combination
> > doesn't seem to be well documented, which makes me wonder what the
> > callers expect it to do.
>
> Have you read the docstring of the open_path_allow_null enumerator?
Maybe I was thinking of open_path_last_optional. "If FLAGS & open_path_last_optional is ... non-zero, require all the parent directories to exist as normal ..." when in this case the parent is not a directory.
--
- Julian
Re: [PATCH] Re: [PATCH] A test for "Can't get entries" error
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Julian Foad wrote on Wed, 21 Nov 2018 16:00 +0000:
> The top-of-loop comment says:
> "/* Whenever we are at the top of this loop:
> - HERE is our current directory, ..."
>
> As HERE is apparently NOT a directory in this case, I wonder if the
> comment simply should say something like "current path (usually a
> directory)", or whether anything else is amiss too.
>
The comment is somewhat out of date, as it refers to a local variable ID
that doesn't exist. I share your concern, however: I wondered if after
taking the 'shortcut' the loop was entered with some invariant not
holding and if patching the svn_fs_fs__dag_open() call was simply
covering up that underlying issue; however, in the end I think the
issue is real.
It _does_ look odd that open_path_allow_null is checked in two places,
though. I suppose the second check could be removed and deferred to the
next iteration. At that point we might also be able to find a way to
reproduce the original error even in a codepath that doesn't take the
'shortcut', in order to increase our confidence in the patch.
While we're at this function, does anyone understand why directory[1] is
accessed without checking whether directory[0] is not NUL? There is
a comment there, but it doesn't enlighten me. (However, I haven't run
'blame' on that comment yet.) Even if it's correct, is there any reason
not to add an SVN_ERR_ASSERT(directory[0]) there?
> In reviewing the code I was unable to keep track of all the nuances of
> what happens (and should happen) in all the edge cases. Especially when
> 'flags & open_path_allow_null' is true and the requested path is a child
> of a non-directory like the "/B/mu/iota" in this case: that combination
> doesn't seem to be well documented, which makes me wonder what the
> callers expect it to do.
Have you read the docstring of the open_path_allow_null enumerator?
Cheers,
Daniel
Re: [PATCH] Re: [PATCH] A test for "Can't get entries" error
Posted by Julian Foad <ju...@apache.org>.
Daniel Shahaf wrote:
> Daniel Shahaf wrote on Tue, Nov 20, 2018 at 09:28:19 +0000:
> > 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:
[...]
> 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?
The top-of-loop comment says:
"/* Whenever we are at the top of this loop:
- HERE is our current directory, ..."
As HERE is apparently NOT a directory in this case, I wonder if the comment simply should say something like "current path (usually a directory)", or whether anything else is amiss too.
In reviewing the code I was unable to keep track of all the nuances of what happens (and should happen) in all the edge cases. Especially when 'flags & open_path_allow_null' is true and the requested path is a child of a non-directory like the "/B/mu/iota" in this case: that combination doesn't seem to be well documented, which makes me wonder what the callers expect it to do.
--
- Julian