Re: [PATCH] Re: [PATCH] A test for "Can't get entries" error
Julian Foad wrote on Fri, 23 Nov 2018 13:56 +: > Daniel Shahaf wrote: > > Julian Foad wrote on Wed, 21 Nov 2018 16:00 +: > > > 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
Daniel Shahaf wrote: > Julian Foad wrote on Wed, 21 Nov 2018 16:00 +: > > 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
Daniel Shahaf wrote on Thu, 22 Nov 2018 00:51 +: > 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
Julian Foad wrote on Wed, 21 Nov 2018 16:00 +: > 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
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
Daniel Shahaf wrote:
> Daniel Shahaf wrote on Tue, Nov 20, 2018 at 09:28:19 +:
> > 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
