subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Shahaf <...@daniel.shahaf.name>
Subject [PATCH] Re: [PATCH] A test for "Can't get entries" error
Date Wed, 21 Nov 2018 14:55:23 GMT
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

Mime
View raw message