The implementation of fs::relative resolves symlinks which is not intended
in ListWalletDir. The replacement does what is required, and listwalletdir RPC
tests are fixed accordingly.
Also, fs::recursive_directory_iterator iteration is fixed to build with boost 1.47.
54 | + fs::path relative, parent = path; 55 | + while (!parent.empty() && parent != base) { 56 | + relative = parent.filename() / relative; 57 | + parent = parent.parent_path(); 58 | + } 59 | + return relative;
please add a unit test for this function
Done.
<!--e57a25ab6845829454e8d69fc972939a-->Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
72 | @@ -73,7 +73,7 @@ def wallet_file(name): 73 | wallet_names.remove('w7_symlink') 74 | extra_args = ['-wallet={}'.format(n) for n in wallet_names] 75 | self.start_node(0, extra_args) 76 | - assert_equal(set(map(lambda w: w['name'], self.nodes[0].listwalletdir()['wallets'])), set(['', 'w3', 'w2', 'sub/w5', 'w7', 'w7', 'w1', 'w8', 'w'])) 77 | + assert_equal(sorted(map(lambda w: w['name'], self.nodes[0].listwalletdir()['wallets'])), ['', 'sub/w5', 'w', 'w1', 'w2', 'w3', 'w7', 'w7_symlink', 'w8'])
Note to reviewers, I've removed the set() because it removes duplicate and that's not the point — w7 was duplicate — sorted() is what's needed.
192 | + if (!path.is_absolute() || !base.is_absolute()) return false; 193 | + fs::path relative, parent = path; 194 | + boost::system::error_code ec; 195 | + while (!parent.empty() && !fs::equivalent(parent, base, ec)) { 196 | + relative = parent.filename() / relative; 197 | + parent = parent.parent_path();
On Windows, std::filesystem::path("C:\\").parent_path() == std::filesystem::path("C:\\"). It would be an infinity loop. (Only test std::filesystem in MSVC, doesn't test boost::filesystem)
Added parent != parent.root_path() condition to the loop.
On Windows, std::filesystem::path("C:\").parent_path() == std::filesystem::path("C:\")
Workaround seems ok, but is this is a bug? Both boost filesystem and std::filesystem are supposed to return empty if the path contains a single element:
https://en.cppreference.com/w/cpp/experimental/fs/path/parent_path https://www.boost.org/doc/libs/1_68_0/libs/filesystem/doc/reference.html#path-parent_path
https://en.cppreference.com/w/cpp/filesystem/path/parent_path The C++17 standard one is a little different from the experimental one.
1231 | + 1232 | + BOOST_CHECK_EQUAL(GetRelativePath(path, base / "bar", base), true); 1233 | + BOOST_CHECK_EQUAL(path, "bar"); 1234 | + 1235 | + BOOST_CHECK_EQUAL(GetRelativePath(path, base / "foo/bar", base), true); 1236 | + BOOST_CHECK_EQUAL(path, "foo/bar");
I assume this would cause infinity loop on Windows if base / "bar" exist.
GetRelativePath(path, base, base / "bar");
1216 | @@ -1217,6 +1217,25 @@ BOOST_AUTO_TEST_CASE(test_DirIsWritable) 1217 | fs::remove(tmpdirname); 1218 | } 1219 | 1220 | +BOOST_AUTO_TEST_CASE(test_GetRelativePath) 1221 | +{ 1222 | + fs::path base = fs::temp_directory_path();
Use SetDataDir instead
I had that but datadir seemed unrelated to this function. I can revert if there is a reason to do that.
The implementation of fs::relative resolves symlinks which is not intended
in ListWalletDir. The replacement does what is required, and listwalletdir
tests are fixed accordingly.
Also, building with boost 1.47 required 2 changes:
- replace fs::relative with an alternative implementation;
- fix fs::recursive_directory_iterator iteration.
24 | #include <db_cxx.h>
25 |
26 | static const unsigned int DEFAULT_WALLET_DBLOGSIZE = 100;
27 | static const bool DEFAULT_WALLET_PRIVDB = true;
28 |
29 | +struct BerkeleyFileid {
BerkeleyFileId?
825 | @@ -826,6 +826,10 @@ void BerkeleyDatabase::Flush(bool shutdown) 826 | LOCK(cs_db); 827 | g_dbenvs.erase(env->Directory().string()); 828 | env = nullptr; 829 | + } else { 830 | + // To avoid accessing a map that has already deconstructed, do not call this when shutdown is true. g_dbenvs.erase would also erase this value. 831 | + // TODO: get rid of wild pointers
what are "wild pointers"?
Actually it's #14320
aka prchain.
Agree @ryanofsky.