From 3673ca36ef84192b42d7e6acbdc8b5d2ffc7a0cf Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Wed, 4 Apr 2018 18:01:00 +1000 Subject: [PATCH] ArgsManager: keep command line and config file arguments separate --- src/test/util_tests.cpp | 79 +++++++++++++++++++----------- src/util.cpp | 105 +++++++++++++++++++++++++++++++--------- src/util.h | 9 ++-- 3 files changed, 137 insertions(+), 56 deletions(-) diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index d41c43a795..f33cda6ff5 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -187,13 +187,17 @@ BOOST_AUTO_TEST_CASE(util_FormatISO8601Time) struct TestArgsManager : public ArgsManager { - std::map& GetMapArgs() { return mapArgs; } - const std::map >& GetMapMultiArgs() { return mapMultiArgs; } + std::map >& GetOverrideArgs() { return m_override_args; } + std::map >& GetConfigArgs() { return m_config_args; } const std::unordered_set& GetNegatedArgs() { return m_negated_args; } void ReadConfigString(const std::string str_config) { - std::istringstream stream(str_config); - ReadConfigStream(stream); + std::istringstream streamConfig(str_config); + { + LOCK(cs_args); + m_config_args.clear(); + } + ReadConfigStream(streamConfig); } }; @@ -203,22 +207,26 @@ BOOST_AUTO_TEST_CASE(util_ParseParameters) const char *argv_test[] = {"-ignored", "-a", "-b", "-ccc=argument", "-ccc=multiple", "f", "-d=e"}; testArgs.ParseParameters(0, (char**)argv_test); - BOOST_CHECK(testArgs.GetMapArgs().empty() && testArgs.GetMapMultiArgs().empty()); + BOOST_CHECK(testArgs.GetOverrideArgs().empty() && testArgs.GetConfigArgs().empty()); testArgs.ParseParameters(1, (char**)argv_test); - BOOST_CHECK(testArgs.GetMapArgs().empty() && testArgs.GetMapMultiArgs().empty()); + BOOST_CHECK(testArgs.GetOverrideArgs().empty() && testArgs.GetConfigArgs().empty()); testArgs.ParseParameters(7, (char**)argv_test); // expectation: -ignored is ignored (program name argument), // -a, -b and -ccc end up in map, -d ignored because it is after // a non-option argument (non-GNU option parsing) - BOOST_CHECK(testArgs.GetMapArgs().size() == 3 && testArgs.GetMapMultiArgs().size() == 3); + BOOST_CHECK(testArgs.GetOverrideArgs().size() == 3 && testArgs.GetConfigArgs().empty()); BOOST_CHECK(testArgs.IsArgSet("-a") && testArgs.IsArgSet("-b") && testArgs.IsArgSet("-ccc") && !testArgs.IsArgSet("f") && !testArgs.IsArgSet("-d")); - BOOST_CHECK(testArgs.GetMapMultiArgs().count("-a") && testArgs.GetMapMultiArgs().count("-b") && testArgs.GetMapMultiArgs().count("-ccc") - && !testArgs.GetMapMultiArgs().count("f") && !testArgs.GetMapMultiArgs().count("-d")); + BOOST_CHECK(testArgs.GetOverrideArgs().count("-a") && testArgs.GetOverrideArgs().count("-b") && testArgs.GetOverrideArgs().count("-ccc") + && !testArgs.GetOverrideArgs().count("f") && !testArgs.GetOverrideArgs().count("-d")); - BOOST_CHECK(testArgs.GetMapArgs()["-a"] == "" && testArgs.GetMapArgs()["-ccc"] == "multiple"); + BOOST_CHECK(testArgs.GetOverrideArgs()["-a"].size() == 1); + BOOST_CHECK(testArgs.GetOverrideArgs()["-a"].front() == ""); + BOOST_CHECK(testArgs.GetOverrideArgs()["-ccc"].size() == 2); + BOOST_CHECK(testArgs.GetOverrideArgs()["-ccc"].front() == "argument"); + BOOST_CHECK(testArgs.GetOverrideArgs()["-ccc"].back() == "multiple"); BOOST_CHECK(testArgs.GetArgs("-ccc").size() == 2); } @@ -234,8 +242,8 @@ BOOST_AUTO_TEST_CASE(util_GetBoolArg) BOOST_CHECK(testArgs.IsArgSet({'-', opt}) || !opt); // Nothing else should be in the map - BOOST_CHECK(testArgs.GetMapArgs().size() == 6 && - testArgs.GetMapMultiArgs().size() == 6); + BOOST_CHECK(testArgs.GetOverrideArgs().size() == 6 && + testArgs.GetConfigArgs().empty()); // The -no prefix should get stripped on the way in. BOOST_CHECK(!testArgs.IsArgSet("-nob")); @@ -326,17 +334,17 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream) test_args.ReadConfigString(str_config); // expectation: a, b, ccc, d, fff, ggg, h, i end up in map - BOOST_CHECK(test_args.GetMapArgs().size() == 8); - BOOST_CHECK(test_args.GetMapMultiArgs().size() == 8); + BOOST_CHECK(test_args.GetOverrideArgs().empty()); + BOOST_CHECK(test_args.GetConfigArgs().size() == 8); - BOOST_CHECK(test_args.GetMapArgs().count("-a") - && test_args.GetMapArgs().count("-b") - && test_args.GetMapArgs().count("-ccc") - && test_args.GetMapArgs().count("-d") - && test_args.GetMapArgs().count("-fff") - && test_args.GetMapArgs().count("-ggg") - && test_args.GetMapArgs().count("-h") - && test_args.GetMapArgs().count("-i") + BOOST_CHECK(test_args.GetConfigArgs().count("-a") + && test_args.GetConfigArgs().count("-b") + && test_args.GetConfigArgs().count("-ccc") + && test_args.GetConfigArgs().count("-d") + && test_args.GetConfigArgs().count("-fff") + && test_args.GetConfigArgs().count("-ggg") + && test_args.GetConfigArgs().count("-h") + && test_args.GetConfigArgs().count("-i") ); BOOST_CHECK(test_args.IsArgSet("-a") @@ -411,16 +419,24 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream) BOOST_AUTO_TEST_CASE(util_GetArg) { TestArgsManager testArgs; - testArgs.GetMapArgs().clear(); - testArgs.GetMapArgs()["strtest1"] = "string..."; + testArgs.GetOverrideArgs().clear(); + testArgs.GetOverrideArgs()["strtest1"] = {"string..."}; // strtest2 undefined on purpose - testArgs.GetMapArgs()["inttest1"] = "12345"; - testArgs.GetMapArgs()["inttest2"] = "81985529216486895"; + testArgs.GetOverrideArgs()["inttest1"] = {"12345"}; + testArgs.GetOverrideArgs()["inttest2"] = {"81985529216486895"}; // inttest3 undefined on purpose - testArgs.GetMapArgs()["booltest1"] = ""; + testArgs.GetOverrideArgs()["booltest1"] = {""}; // booltest2 undefined on purpose - testArgs.GetMapArgs()["booltest3"] = "0"; - testArgs.GetMapArgs()["booltest4"] = "1"; + testArgs.GetOverrideArgs()["booltest3"] = {"0"}; + testArgs.GetOverrideArgs()["booltest4"] = {"1"}; + + // priorities + testArgs.GetOverrideArgs()["pritest1"] = {"a", "b"}; + testArgs.GetConfigArgs()["pritest2"] = {"a", "b"}; + testArgs.GetOverrideArgs()["pritest3"] = {"a"}; + testArgs.GetConfigArgs()["pritest3"] = {"b"}; + testArgs.GetOverrideArgs()["pritest4"] = {"a","b"}; + testArgs.GetConfigArgs()["pritest4"] = {"c","d"}; BOOST_CHECK_EQUAL(testArgs.GetArg("strtest1", "default"), "string..."); BOOST_CHECK_EQUAL(testArgs.GetArg("strtest2", "default"), "default"); @@ -431,6 +447,11 @@ BOOST_AUTO_TEST_CASE(util_GetArg) BOOST_CHECK_EQUAL(testArgs.GetBoolArg("booltest2", false), false); BOOST_CHECK_EQUAL(testArgs.GetBoolArg("booltest3", false), false); BOOST_CHECK_EQUAL(testArgs.GetBoolArg("booltest4", false), true); + + BOOST_CHECK_EQUAL(testArgs.GetArg("pritest1", "default"), "b"); + BOOST_CHECK_EQUAL(testArgs.GetArg("pritest2", "default"), "a"); + BOOST_CHECK_EQUAL(testArgs.GetArg("pritest3", "default"), "a"); + BOOST_CHECK_EQUAL(testArgs.GetArg("pritest4", "default"), "b"); } BOOST_AUTO_TEST_CASE(util_GetChainName) diff --git a/src/util.cpp b/src/util.cpp index f55c9c8c34..f8366fe694 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -455,6 +455,67 @@ static bool InterpretBool(const std::string& strValue) return (atoi(strValue) != 0); } +/** Internal helper functions for ArgsManager */ +class ArgsManagerHelper { +public: + typedef std::map> MapArgs; + + /** Find arguments in a map and add them to a vector */ + static inline void AddArgs(std::vector& res, const MapArgs& map_args, const std::string& arg) + { + auto it = map_args.find(arg); + if (it != map_args.end()) { + res.insert(res.end(), it->second.begin(), it->second.end()); + } + } + + /** Return true/false if an argument is set in a map, and also + * return the first (or last) of the possibly multiple values it has + */ + static inline std::pair GetArgHelper(const MapArgs& map_args, const std::string& arg, bool getLast = false) + { + auto it = map_args.find(arg); + + if (it == map_args.end() || it->second.empty()) { + return std::make_pair(false, std::string()); + } + + if (getLast) { + return std::make_pair(true, it->second.back()); + } else { + return std::make_pair(true, it->second.front()); + } + } + + /* Get the string value of an argument, returning a pair of a boolean + * indicating the argument was found, and the value for the argument + * if it was found (or the empty string if not found). + */ + static inline std::pair GetArg(const ArgsManager &am, const std::string& arg) + { + LOCK(am.cs_args); + std::pair found_result(false, std::string()); + + // We pass "true" to GetArgHelper in order to return the last + // argument value seen from the command line (so "bitcoind -foo=bar + // -foo=baz" gives GetArg(am,"foo")=={true,"baz"} + found_result = GetArgHelper(am.m_override_args, arg, true); + if (found_result.first) { + return found_result; + } + + // But in contrast we return the first argument seen in a config file, + // so "foo=bar \n foo=baz" in the config file gives + // GetArg(am,"foo")={true,"bar"} + found_result = GetArgHelper(am.m_config_args, arg); + if (found_result.first) { + return found_result; + } + + return found_result; + } +}; + /** * Interpret -nofoo as if the user supplied -foo=0. * @@ -485,8 +546,7 @@ void ArgsManager::InterpretNegatedOption(std::string& key, std::string& val) void ArgsManager::ParseParameters(int argc, const char* const argv[]) { LOCK(cs_args); - mapArgs.clear(); - mapMultiArgs.clear(); + m_override_args.clear(); m_negated_args.clear(); for (int i = 1; i < argc; i++) { @@ -513,23 +573,23 @@ void ArgsManager::ParseParameters(int argc, const char* const argv[]) // Transform -nofoo to -foo=0 InterpretNegatedOption(key, val); - mapArgs[key] = val; - mapMultiArgs[key].push_back(val); + m_override_args[key].push_back(val); } } std::vector ArgsManager::GetArgs(const std::string& strArg) const { + std::vector result = {}; + LOCK(cs_args); - auto it = mapMultiArgs.find(strArg); - if (it != mapMultiArgs.end()) return it->second; - return {}; + ArgsManagerHelper::AddArgs(result, m_override_args, strArg); + ArgsManagerHelper::AddArgs(result, m_config_args, strArg); + return result; } bool ArgsManager::IsArgSet(const std::string& strArg) const { - LOCK(cs_args); - return mapArgs.count(strArg); + return ArgsManagerHelper::GetArg(*this, strArg).first; } bool ArgsManager::IsArgNegated(const std::string& strArg) const @@ -540,25 +600,22 @@ bool ArgsManager::IsArgNegated(const std::string& strArg) const std::string ArgsManager::GetArg(const std::string& strArg, const std::string& strDefault) const { - LOCK(cs_args); - auto it = mapArgs.find(strArg); - if (it != mapArgs.end()) return it->second; + std::pair found_res = ArgsManagerHelper::GetArg(*this, strArg); + if (found_res.first) return found_res.second; return strDefault; } int64_t ArgsManager::GetArg(const std::string& strArg, int64_t nDefault) const { - LOCK(cs_args); - auto it = mapArgs.find(strArg); - if (it != mapArgs.end()) return atoi64(it->second); + std::pair found_res = ArgsManagerHelper::GetArg(*this, strArg); + if (found_res.first) return atoi64(found_res.second); return nDefault; } bool ArgsManager::GetBoolArg(const std::string& strArg, bool fDefault) const { - LOCK(cs_args); - auto it = mapArgs.find(strArg); - if (it != mapArgs.end()) return InterpretBool(it->second); + std::pair found_res = ArgsManagerHelper::GetArg(*this, strArg); + if (found_res.first) return InterpretBool(found_res.second); return fDefault; } @@ -581,8 +638,7 @@ bool ArgsManager::SoftSetBoolArg(const std::string& strArg, bool fValue) void ArgsManager::ForceSetArg(const std::string& strArg, const std::string& strValue) { LOCK(cs_args); - mapArgs[strArg] = strValue; - mapMultiArgs[strArg] = {strValue}; + m_override_args[strArg] = {strValue}; } bool HelpRequested(const ArgsManager& args) @@ -749,14 +805,17 @@ void ArgsManager::ReadConfigStream(std::istream& stream) std::string strKey = std::string("-") + it->string_key; std::string strValue = it->value[0]; InterpretNegatedOption(strKey, strValue); - if (mapArgs.count(strKey) == 0) - mapArgs[strKey] = strValue; - mapMultiArgs[strKey].push_back(strValue); + m_config_args[strKey].push_back(strValue); } } void ArgsManager::ReadConfigFile(const std::string& confPath) { + { + LOCK(cs_args); + m_config_args.clear(); + } + fs::ifstream stream(GetConfigFile(confPath)); // ok to not have a config file diff --git a/src/util.h b/src/util.h index 3952461e48..01ff5992fb 100644 --- a/src/util.h +++ b/src/util.h @@ -223,11 +223,12 @@ inline bool IsSwitchChar(char c) class ArgsManager { protected: - mutable CCriticalSection cs_args; - std::map mapArgs; - std::map> mapMultiArgs; - std::unordered_set m_negated_args; + friend class ArgsManagerHelper; + mutable CCriticalSection cs_args; + std::map> m_override_args; + std::map> m_config_args; + std::unordered_set m_negated_args; void ReadConfigStream(std::istream& stream); public: