foster/bitcoin-bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-03 09:56:38 -05:00
Code Issues Activity

Merge bitcoin/bitcoin#24265: Drop StripRedundantLastElementsOfPath() function

Browse source 
ebda2b8c81 util: Drop no longer needed StripRedundantLastElementsOfPath() function (Hennadii Stepanov)
ecd094e2b1 Use ArgsManager::GetPathArg() for "-walletdir" option (Hennadii Stepanov)
06fed4c21e Use ArgsManager::GetPathArg() for "-blocksdir" option (Hennadii Stepanov)
15b632bf16 Use ArgsManager::GetPathArg() for "-datadir" option (Hennadii Stepanov)
540ca5111f util: Add ArgsManager::GetPathArg() function (Hennadii Stepanov)

Pull request description:

  [Switching](https://github.com/bitcoin/bitcoin/pull/20744) to `std::filesystems` makes possible to leverage [`std::filesystem::path::lexically_normal`](https://en.cppreference.com/w/cpp/filesystem/path/lexically_normal) and get rid of ugly `StripRedundantLastElementsOfPath()` crutch.

  To make its usage simple and error-proof, a new `ArgsManager::GetPathArg()` member function introduced which guarantees to return a normalized with no trailing slashes paths provided via `-datadir`, `-blocksdir` or `-walletdir` command-line arguments or configure options.

ACKs for top commit:
  ryanofsky:
    Code review ACK ebda2b8c81. Only change since last review is rebase which simplified the last commit

Tree-SHA512: ed86959b6038b7152b5a1d473478667b72caab1716cc9149e1a75833d50511f22157e4e5e55a9465d1fa76b90bce5e5286f4e4f0d1ae65ebd9c012fae19d835f
This commit is contained in:
fanquake 2022-02-09 21:40:53 +00:00
commit 8c0f02c69d
No known key found for this signature in database
GPG key ID: 2EEB9F5CC09526C1
7 changed files with 121 additions and 26 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
src/init.cpp
View file

@ -1150,7 +1150,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
LogPrintf("Using at most %i automatic connections (%i file descriptors available)\n", nMaxConnections, nFD);
// Warn about relative -datadir path.
if (args.IsArgSet("-datadir") && !fs::PathFromString(args.GetArg("-datadir", "")).is_absolute()) {
if (args.IsArgSet("-datadir") && !args.GetPathArg("-datadir").is_absolute()) {
LogPrintf("Warning: relative datadir option '%s' specified, which will be interpreted relative to the " /* Continued */
"current working directory '%s'. This is fragile, because if bitcoin is started in the future "
"from a different location, it will be unable to locate the current data files. There could "

92
src/test/getarg_tests.cpp
View file

@ -159,6 +159,98 @@ BOOST_AUTO_TEST_CASE(intarg)
BOOST_CHECK_EQUAL(m_local_args.GetIntArg("-bar", 11), 0);
}
BOOST_AUTO_TEST_CASE(patharg)
{
const auto dir = std::make_pair("-dir", ArgsManager::ALLOW_ANY);
SetupArgs({dir});
ResetArgs("");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), fs::path{});
const fs::path root_path{"/"};
ResetArgs("-dir=/");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), root_path);
ResetArgs("-dir=/.");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), root_path);
ResetArgs("-dir=/./");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), root_path);
ResetArgs("-dir=/.//");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), root_path);
#ifdef WIN32
const fs::path win_root_path{"C:\\"};
ResetArgs("-dir=C:\\");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), win_root_path);
ResetArgs("-dir=C:/");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), win_root_path);
ResetArgs("-dir=C:\\\\");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), win_root_path);
ResetArgs("-dir=C:\\.");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), win_root_path);
ResetArgs("-dir=C:\\.\\");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), win_root_path);
ResetArgs("-dir=C:\\.\\\\");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), win_root_path);
#endif
const fs::path absolute_path{"/home/user/.bitcoin"};
ResetArgs("-dir=/home/user/.bitcoin");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), absolute_path);
ResetArgs("-dir=/root/../home/user/.bitcoin");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), absolute_path);
ResetArgs("-dir=/home/./user/.bitcoin");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), absolute_path);
ResetArgs("-dir=/home/user/.bitcoin/");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), absolute_path);
ResetArgs("-dir=/home/user/.bitcoin//");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), absolute_path);
ResetArgs("-dir=/home/user/.bitcoin/.");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), absolute_path);
ResetArgs("-dir=/home/user/.bitcoin/./");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), absolute_path);
ResetArgs("-dir=/home/user/.bitcoin/.//");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), absolute_path);
const fs::path relative_path{"user/.bitcoin"};
ResetArgs("-dir=user/.bitcoin");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path);
ResetArgs("-dir=somewhere/../user/.bitcoin");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path);
ResetArgs("-dir=user/./.bitcoin");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path);
ResetArgs("-dir=user/.bitcoin/");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path);
ResetArgs("-dir=user/.bitcoin//");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path);
ResetArgs("-dir=user/.bitcoin/.");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path);
ResetArgs("-dir=user/.bitcoin/./");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path);
ResetArgs("-dir=user/.bitcoin/.//");
BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path);
}
BOOST_AUTO_TEST_CASE(doubledash)
{
const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_ANY);

31
src/util/system.cpp
View file

@ -245,19 +245,6 @@ static std::optional<util::SettingsValue> InterpretValue(const KeyInfo& key, con
return value;
}
namespace {
fs::path StripRedundantLastElementsOfPath(const fs::path& path)
{
auto result = path;
while (result.filename().empty() || fs::PathToString(result.filename()) == ".") {
result = result.parent_path();
}
assert(fs::equivalent(result, path.lexically_normal()));
return result;
}
} // namespace
// Define default constructor and destructor that are not inline, so code instantiating this class doesn't need to
// #include class definitions for all members.
// For example, m_settings has an internal dependency on univalue.
@ -399,6 +386,13 @@ std::optional<unsigned int> ArgsManager::GetArgFlags(const std::string& name) co
return std::nullopt;
}
fs::path ArgsManager::GetPathArg(std::string pathlike_arg) const
{
auto result = fs::PathFromString(GetArg(pathlike_arg, "")).lexically_normal();
// Remove trailing slash, if present.
return result.has_filename() ? result : result.parent_path();
}
const fs::path& ArgsManager::GetBlocksDirPath() const
{
LOCK(cs_args);
@ -409,7 +403,7 @@ const fs::path& ArgsManager::GetBlocksDirPath() const
if (!path.empty()) return path;
if (IsArgSet("-blocksdir")) {
path = fs::absolute(fs::PathFromString(GetArg("-blocksdir", "")));
path = fs::absolute(GetPathArg("-blocksdir"));
if (!fs::is_directory(path)) {
path = "";
return path;
@ -433,9 +427,9 @@ const fs::path& ArgsManager::GetDataDir(bool net_specific) const
// this function
if (!path.empty()) return path;
std::string datadir = GetArg("-datadir", "");
const fs::path datadir{GetPathArg("-datadir")};
if (!datadir.empty()) {
path = fs::absolute(StripRedundantLastElementsOfPath(fs::PathFromString(datadir)));
path = fs::absolute(datadir);
if (!fs::is_directory(path)) {
path = "";
return path;
@ -455,7 +449,6 @@ const fs::path& ArgsManager::GetDataDir(bool net_specific) const
}
}
path = StripRedundantLastElementsOfPath(path);
return path;
}
@ -816,8 +809,8 @@ fs::path GetDefaultDataDir()
bool CheckDataDirOption()
{
std::string datadir = gArgs.GetArg("-datadir", "");
return datadir.empty() || fs::is_directory(fs::absolute(fs::PathFromString(datadir)));
const fs::path datadir{gArgs.GetPathArg("-datadir")};
return datadir.empty() || fs::is_directory(fs::absolute(datadir));
}
fs::path GetConfigFile(const std::string& confPath)

10
src/util/system.h
View file

@ -264,6 +264,16 @@ protected:
*/
std::optional<const Command> GetCommand() const;
/**
* Get a normalized path from a specified pathlike argument
*
* It is guaranteed that the returned path has no trailing slashes.
*
* @param pathlike_arg Pathlike argument to get a path from (e.g., "-datadir", "-blocksdir" or "-walletdir")
* @return Normalized path which is get from a specified pathlike argument
*/
fs::path GetPathArg(std::string pathlike_arg) const;
/**
* Get blocks directory path
*

2
src/wallet/load.cpp
View file

@ -28,7 +28,7 @@ bool VerifyWallets(WalletContext& context)
ArgsManager& args = *Assert(context.args);
if (args.IsArgSet("-walletdir")) {
fs::path wallet_dir = fs::PathFromString(args.GetArg("-walletdir", ""));
const fs::path wallet_dir{args.GetPathArg("-walletdir")};
std::error_code error;
// The canonical path cleans the path, preventing >1 Berkeley environment instances for the same directory
// It also lets the fs::exists and fs::is_directory checks below pass on windows, since they return false

8
src/wallet/test/init_tests.cpp
View file

@ -18,7 +18,7 @@ BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_default)
SetWalletDir(m_walletdir_path_cases["default"]);
bool result = m_wallet_loader->verify();
BOOST_CHECK(result == true);
fs::path walletdir = fs::PathFromString(gArgs.GetArg("-walletdir", ""));
fs::path walletdir = gArgs.GetPathArg("-walletdir");
fs::path expected_path = fs::canonical(m_walletdir_path_cases["default"]);
BOOST_CHECK_EQUAL(walletdir, expected_path);
}
@ -28,7 +28,7 @@ BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_custom)
SetWalletDir(m_walletdir_path_cases["custom"]);
bool result = m_wallet_loader->verify();
BOOST_CHECK(result == true);
fs::path walletdir = fs::PathFromString(gArgs.GetArg("-walletdir", ""));
fs::path walletdir = gArgs.GetPathArg("-walletdir");
fs::path expected_path = fs::canonical(m_walletdir_path_cases["custom"]);
BOOST_CHECK_EQUAL(walletdir, expected_path);
}
@ -68,7 +68,7 @@ BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_no_trailing)
SetWalletDir(m_walletdir_path_cases["trailing"]);
bool result = m_wallet_loader->verify();
BOOST_CHECK(result == true);
fs::path walletdir = fs::PathFromString(gArgs.GetArg("-walletdir", ""));
fs::path walletdir = gArgs.GetPathArg("-walletdir");
fs::path expected_path = fs::canonical(m_walletdir_path_cases["default"]);
BOOST_CHECK_EQUAL(walletdir, expected_path);
}
@ -78,7 +78,7 @@ BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_no_trailing2)
SetWalletDir(m_walletdir_path_cases["trailing2"]);
bool result = m_wallet_loader->verify();
BOOST_CHECK(result == true);
fs::path walletdir = fs::PathFromString(gArgs.GetArg("-walletdir", ""));
fs::path walletdir = gArgs.GetPathArg("-walletdir");
fs::path expected_path = fs::canonical(m_walletdir_path_cases["default"]);
BOOST_CHECK_EQUAL(walletdir, expected_path);
}

2
src/wallet/walletutil.cpp
View file

@ -13,7 +13,7 @@ fs::path GetWalletDir()
fs::path path;
if (gArgs.IsArgSet("-walletdir")) {
path = fs::PathFromString(gArgs.GetArg("-walletdir", ""));
path = gArgs.GetPathArg("-walletdir");
if (!fs::is_directory(path)) {
// If the path specified doesn't exist, we return the deliberately
// invalid empty string.