diff --git a/src/test/getarg_tests.cpp b/src/test/getarg_tests.cpp --- a/src/test/getarg_tests.cpp +++ b/src/test/getarg_tests.cpp @@ -48,7 +48,7 @@ BOOST_AUTO_TEST_CASE(boolarg) { ArgsManager am; - const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_BOOL); + const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_ANY); SetupArgs(am, {foo}); ResetArgs(am, "-foo"); BOOST_CHECK(am.GetBoolArg("-foo", false)); @@ -104,8 +104,8 @@ BOOST_AUTO_TEST_CASE(stringarg) { ArgsManager am; - const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_STRING); - const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_STRING); + const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_ANY); + const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_ANY); SetupArgs(am, {foo, bar}); ResetArgs(am, ""); BOOST_CHECK_EQUAL(am.GetArg("-foo", ""), ""); @@ -130,8 +130,8 @@ BOOST_AUTO_TEST_CASE(intarg) { ArgsManager am; - const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_INT); - const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_INT); + const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_ANY); + const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_ANY); SetupArgs(am, {foo, bar}); ResetArgs(am, ""); BOOST_CHECK_EQUAL(am.GetArg("-foo", 11), 11); @@ -165,8 +165,8 @@ BOOST_AUTO_TEST_CASE(boolargno) { ArgsManager am; - const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_BOOL); - const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_BOOL); + const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_ANY); + const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_ANY); SetupArgs(am, {foo, bar}); ResetArgs(am, "-nofoo"); BOOST_CHECK(!am.GetBoolArg("-foo", true)); diff --git a/src/test/settings_tests.cpp b/src/test/settings_tests.cpp --- a/src/test/settings_tests.cpp +++ b/src/test/settings_tests.cpp @@ -49,6 +49,22 @@ CheckValues(settings2, R"("val2")", R"(["val2","val3"])"); } +// Confirm that a high priority setting overrides a lower priority setting even +// if the high priority setting is null. This behavior is useful for a high +// priority setting source to be able to effectively reset any setting back to +// its default value. +BOOST_AUTO_TEST_CASE(NullOverride) { + util::Settings settings; + settings.command_line_options["name"].push_back("value"); + BOOST_CHECK_EQUAL( + R"("value")", + GetSetting(settings, "section", "name", false, false).write().c_str()); + settings.forced_settings["name"] = {}; + BOOST_CHECK_EQUAL( + R"(null)", + GetSetting(settings, "section", "name", false, false).write().c_str()); +} + // Test different ways settings can be merged, and verify results. This test can // be used to confirm that updates to settings code don't change behavior // unintentionally. diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include #include @@ -194,11 +195,159 @@ } } using ArgsManager::cs_args; + using ArgsManager::GetSetting; + using ArgsManager::GetSettingsList; using ArgsManager::m_network; using ArgsManager::m_settings; using ArgsManager::ReadConfigStream; }; +//! Test GetSetting and GetArg type coercion, negation, and default value +//! handling. +class CheckValueTest : public TestChain100Setup { +public: + struct Expect { + util::SettingsValue setting; + bool default_string = false; + bool default_int = false; + bool default_bool = false; + const char *string_value = nullptr; + Optional int_value; + Optional bool_value; + Optional> list_value; + const char *error = nullptr; + + Expect(util::SettingsValue s) : setting(std::move(s)) {} + Expect &DefaultString() { + default_string = true; + return *this; + } + Expect &DefaultInt() { + default_int = true; + return *this; + } + Expect &DefaultBool() { + default_bool = true; + return *this; + } + Expect &String(const char *s) { + string_value = s; + return *this; + } + Expect &Int(int64_t i) { + int_value = i; + return *this; + } + Expect &Bool(bool b) { + bool_value = b; + return *this; + } + Expect &List(std::vector m) { + list_value = std::move(m); + return *this; + } + Expect &Error(const char *e) { + error = e; + return *this; + } + }; + + void CheckValue(unsigned int flags, const char *arg, const Expect &expect) { + TestArgsManager test; + test.SetupArgs({{"-value", flags}}); + const char *argv[] = {"ignored", arg}; + std::string error; + bool success = test.ParseParameters(arg ? 2 : 1, (char **)argv, error); + + BOOST_CHECK_EQUAL(test.GetSetting("-value").write(), + expect.setting.write()); + auto settings_list = test.GetSettingsList("-value"); + if (expect.setting.isNull() || expect.setting.isFalse()) { + BOOST_CHECK_EQUAL(settings_list.size(), 0); + } else { + BOOST_CHECK_EQUAL(settings_list.size(), 1); + BOOST_CHECK_EQUAL(settings_list[0].write(), expect.setting.write()); + } + + if (expect.error) { + BOOST_CHECK(!success); + BOOST_CHECK_NE(error.find(expect.error), std::string::npos); + } else { + BOOST_CHECK(success); + BOOST_CHECK_EQUAL(error, ""); + } + + if (expect.default_string) { + BOOST_CHECK_EQUAL(test.GetArg("-value", "zzzzz"), "zzzzz"); + } else if (expect.string_value) { + BOOST_CHECK_EQUAL(test.GetArg("-value", "zzzzz"), + expect.string_value); + } else { + BOOST_CHECK(!success); + } + + if (expect.default_int) { + BOOST_CHECK_EQUAL(test.GetArg("-value", 99999), 99999); + } else if (expect.int_value) { + BOOST_CHECK_EQUAL(test.GetArg("-value", 99999), *expect.int_value); + } else { + BOOST_CHECK(!success); + } + + if (expect.default_bool) { + BOOST_CHECK_EQUAL(test.GetBoolArg("-value", false), false); + BOOST_CHECK_EQUAL(test.GetBoolArg("-value", true), true); + } else if (expect.bool_value) { + BOOST_CHECK_EQUAL(test.GetBoolArg("-value", false), + *expect.bool_value); + BOOST_CHECK_EQUAL(test.GetBoolArg("-value", true), + *expect.bool_value); + } else { + BOOST_CHECK(!success); + } + + if (expect.list_value) { + auto l = test.GetArgs("-value"); + BOOST_CHECK_EQUAL_COLLECTIONS(l.begin(), l.end(), + expect.list_value->begin(), + expect.list_value->end()); + } else { + BOOST_CHECK(!success); + } + } +}; + +BOOST_FIXTURE_TEST_CASE(util_CheckValue, CheckValueTest) { + using M = ArgsManager; + + CheckValue(M::ALLOW_ANY, nullptr, + Expect{{}}.DefaultString().DefaultInt().DefaultBool().List({})); + CheckValue(M::ALLOW_ANY, "-novalue", + Expect{false}.String("0").Int(0).Bool(false).List({})); + CheckValue(M::ALLOW_ANY, "-novalue=", + Expect{false}.String("0").Int(0).Bool(false).List({})); + CheckValue(M::ALLOW_ANY, "-novalue=0", + Expect{true}.String("1").Int(1).Bool(true).List({"1"})); + CheckValue(M::ALLOW_ANY, "-novalue=1", + Expect{false}.String("0").Int(0).Bool(false).List({})); + CheckValue(M::ALLOW_ANY, "-novalue=2", + Expect{false}.String("0").Int(0).Bool(false).List({})); + CheckValue(M::ALLOW_ANY, "-novalue=abc", + Expect{true}.String("1").Int(1).Bool(true).List({"1"})); + CheckValue(M::ALLOW_ANY, "-value", + Expect{""}.String("").Int(0).Bool(true).List({""})); + CheckValue(M::ALLOW_ANY, + "-value=", Expect{""}.String("").Int(0).Bool(true).List({""})); + CheckValue(M::ALLOW_ANY, "-value=0", + Expect{"0"}.String("0").Int(0).Bool(false).List({"0"})); + CheckValue(M::ALLOW_ANY, "-value=1", + Expect{"1"}.String("1").Int(1).Bool(true).List({"1"})); + CheckValue(M::ALLOW_ANY, "-value=2", + Expect{"2"}.String("2").Int(2).Bool(true).List({"2"})); + CheckValue(M::ALLOW_ANY, "-value=abc", + Expect{"abc"}.String("abc").Int(0).Bool(false).List({"abc"})); +} + BOOST_AUTO_TEST_CASE(util_ParseParameters) { TestArgsManager testArgs; const auto a = std::make_pair("-a", ArgsManager::ALLOW_ANY); @@ -291,12 +440,12 @@ BOOST_AUTO_TEST_CASE(util_GetBoolArg) { TestArgsManager testArgs; - const auto a = std::make_pair("-a", ArgsManager::ALLOW_BOOL); - const auto b = std::make_pair("-b", ArgsManager::ALLOW_BOOL); - const auto c = std::make_pair("-c", ArgsManager::ALLOW_BOOL); - const auto d = std::make_pair("-d", ArgsManager::ALLOW_BOOL); - const auto e = std::make_pair("-e", ArgsManager::ALLOW_BOOL); - const auto f = std::make_pair("-f", ArgsManager::ALLOW_BOOL); + const auto a = std::make_pair("-a", ArgsManager::ALLOW_ANY); + const auto b = std::make_pair("-b", ArgsManager::ALLOW_ANY); + const auto c = std::make_pair("-c", ArgsManager::ALLOW_ANY); + const auto d = std::make_pair("-d", ArgsManager::ALLOW_ANY); + const auto e = std::make_pair("-e", ArgsManager::ALLOW_ANY); + const auto f = std::make_pair("-f", ArgsManager::ALLOW_ANY); const char *argv_test[] = {"ignored", "-a", "-nob", "-c=0", "-d=1", "-e=false", "-f=true"}; @@ -335,8 +484,8 @@ TestArgsManager testArgs; // Params test - const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_BOOL); - const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_BOOL); + const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_ANY); + const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_ANY); const char *argv_test[] = {"ignored", "-nofoo", "-foo", "-nobar=0"}; testArgs.SetupArgs({foo, bar}); std::string error; @@ -406,16 +555,16 @@ TestArgsManager test_args; LOCK(test_args.cs_args); - const auto a = std::make_pair("-a", ArgsManager::ALLOW_BOOL); - const auto b = std::make_pair("-b", ArgsManager::ALLOW_BOOL); - const auto ccc = std::make_pair("-ccc", ArgsManager::ALLOW_STRING); - const auto d = std::make_pair("-d", ArgsManager::ALLOW_STRING); + const auto a = std::make_pair("-a", ArgsManager::ALLOW_ANY); + const auto b = std::make_pair("-b", ArgsManager::ALLOW_ANY); + const auto ccc = std::make_pair("-ccc", ArgsManager::ALLOW_ANY); + const auto d = std::make_pair("-d", ArgsManager::ALLOW_ANY); const auto e = std::make_pair("-e", ArgsManager::ALLOW_ANY); - const auto fff = std::make_pair("-fff", ArgsManager::ALLOW_BOOL); - const auto ggg = std::make_pair("-ggg", ArgsManager::ALLOW_BOOL); - const auto h = std::make_pair("-h", ArgsManager::ALLOW_BOOL); - const auto i = std::make_pair("-i", ArgsManager::ALLOW_BOOL); - const auto iii = std::make_pair("-iii", ArgsManager::ALLOW_INT); + const auto fff = std::make_pair("-fff", ArgsManager::ALLOW_ANY); + const auto ggg = std::make_pair("-ggg", ArgsManager::ALLOW_ANY); + const auto h = std::make_pair("-h", ArgsManager::ALLOW_ANY); + const auto i = std::make_pair("-i", ArgsManager::ALLOW_ANY); + const auto iii = std::make_pair("-iii", ArgsManager::ALLOW_ANY); test_args.SetupArgs({a, b, ccc, d, e, fff, ggg, h, i, iii}); test_args.ReadConfigString(str_config); @@ -695,8 +844,8 @@ BOOST_AUTO_TEST_CASE(util_GetChainName) { TestArgsManager test_args; - const auto testnet = std::make_pair("-testnet", ArgsManager::ALLOW_BOOL); - const auto regtest = std::make_pair("-regtest", ArgsManager::ALLOW_BOOL); + const auto testnet = std::make_pair("-testnet", ArgsManager::ALLOW_ANY); + const auto regtest = std::make_pair("-regtest", ArgsManager::ALLOW_ANY); test_args.SetupArgs({testnet, regtest}); const char *argv_testnet[] = {"cmd", "-testnet"}; diff --git a/src/util/settings.cpp b/src/util/settings.cpp --- a/src/util/settings.cpp +++ b/src/util/settings.cpp @@ -57,6 +57,8 @@ bool ignore_default_section_config, bool get_chain_name) { SettingsValue result; + // Done merging any more settings sources. + bool done = false; MergeSettings( settings, section, name, [&](SettingsSpan span, Source source) { // Weird behavior preserved for backwards compatibility: Apply @@ -84,6 +86,10 @@ // ignored. const bool skip_negated_command_line = get_chain_name; + if (done) { + return; + } + // Ignore settings in default config section if requested. if (ignore_default_section_config && source == Source::CONFIG_FILE_DEFAULT_SECTION && @@ -96,15 +102,12 @@ return; } - // Stick with highest priority value, keeping result if already set. - if (!result.isNull()) { - return; - } - if (!span.empty()) { result = reverse_precedence ? span.begin()[0] : span.end()[-1]; + done = true; } else if (span.last_negated()) { result = false; + done = true; } }); return result; @@ -115,7 +118,8 @@ const std::string &name, bool ignore_default_section_config) { std::vector result; - bool result_complete = false; + // Done merging any more settings sources. + bool done = false; bool prev_negated_empty = false; MergeSettings( settings, section, name, [&](SettingsSpan span, Source source) { @@ -139,7 +143,7 @@ // Add new settings to the result if isn't already complete, or if // the values are zombies. - if (!result_complete || add_zombie_config_values) { + if (!done || add_zombie_config_values) { for (const auto &value : span) { if (value.isArray()) { result.insert(result.end(), value.getValues().begin(), @@ -150,10 +154,9 @@ } } - // If a setting was negated, or if a setting was forced, set - // result_complete to true to ignore any later lower priority - // settings. - result_complete |= span.negated() > 0 || source == Source::FORCED; + // If a setting was negated, or if a setting was forced, set done to + // true to ignore any later lower priority settings. + done |= span.negated() > 0 || source == Source::FORCED; // Update the negated and empty state used for the zombie values // check. diff --git a/src/util/system.h b/src/util/system.h --- a/src/util/system.h +++ b/src/util/system.h @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -145,7 +146,6 @@ class ArgsManager { public: enum Flags { - NONE = 0x00, // Boolean options can accept negation syntax -noOPTION or -noOPTION=1 ALLOW_BOOL = 0x01, ALLOW_INT = 0x02, @@ -161,8 +161,6 @@ }; protected: - friend class ArgsManagerHelper; - struct Arg { std::string m_help_param; std::string m_help_text; @@ -182,6 +180,29 @@ std::string &error, bool ignore_invalid_keys = false); + /** + * Returns true if settings values from the default section should be used, + * depending on the current network and whether the setting is + * network-specific. + */ + bool UseDefaultSection(const std::string &arg) const + EXCLUSIVE_LOCKS_REQUIRED(cs_args); + + /** + * Get setting value. + * + * Result will be null if setting was unset, true if "-setting" argument was + * passed false if "-nosetting" argument was passed, and a string if a + * "-setting=value" argument was passed. + */ + util::SettingsValue GetSetting(const std::string &arg) const; + + /** + * Get list of setting values. + */ + std::vector + GetSettingsList(const std::string &arg) const; + public: ArgsManager(); @@ -325,9 +346,9 @@ /** * Return Flags for known arg. - * Return ArgsManager::NONE for unknown arg. + * Return nullopt for unknown arg. */ - unsigned int FlagsOfKnownArg(const std::string &key) const; + Optional GetArgFlags(const std::string &name) const; }; extern ArgsManager gArgs; diff --git a/src/util/system.cpp b/src/util/system.cpp --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -187,27 +187,6 @@ return arg.size() > 0 && arg[0] == '-' ? arg.substr(1) : arg; } -/** Internal helper functions for ArgsManager */ -class ArgsManagerHelper { -public: - /** Determine whether to use config settings in the default section, - * See also comments around ArgsManager::ArgsManager() below. */ - static inline bool UseDefaultSection(const ArgsManager &am, - const std::string &arg) - EXCLUSIVE_LOCKS_REQUIRED(am.cs_args) { - return (am.m_network == CBaseChainParams::MAIN || - am.m_network_only_args.count(arg) == 0); - } - - static util::SettingsValue Get(const ArgsManager &am, - const std::string &arg) { - LOCK(am.cs_args); - return GetSetting(am.m_settings, am.m_network, SettingName(arg), - !UseDefaultSection(am, arg), - /* get_chain_name= */ false); - } -}; - /** * Interpret -nofoo as if the user supplied -foo=0. * @@ -361,9 +340,9 @@ key.erase(0, 1); std::string section; util::SettingsValue value = InterpretOption(section, key, val); - const unsigned int flags = FlagsOfKnownArg(key); + Optional flags = GetArgFlags('-' + key); if (flags) { - if (!CheckValid(key, value, flags, error)) { + if (!CheckValid(key, value, *flags, error)) { return false; } // Weird behavior preserved for backwards compatibility: command @@ -379,7 +358,7 @@ } } - // we do not allow -includeconf from command line, so we clear it here + // we do not allow -includeconf from command line bool success = true; if (auto *includes = util::FindKey(m_settings.command_line_options, "includeconf")) { @@ -393,25 +372,20 @@ return success; } -unsigned int ArgsManager::FlagsOfKnownArg(const std::string &key) const { +Optional ArgsManager::GetArgFlags(const std::string &name) const { LOCK(cs_args); for (const auto &arg_map : m_available_args) { - const auto search = arg_map.second.find('-' + key); + const auto search = arg_map.second.find(name); if (search != arg_map.second.end()) { return search->second.m_flags; } } - return ArgsManager::NONE; + return nullopt; } std::vector ArgsManager::GetArgs(const std::string &strArg) const { - LOCK(cs_args); - bool ignore_default_section_config = - !ArgsManagerHelper::UseDefaultSection(*this, strArg); std::vector result; - for (const util::SettingsValue &value : - util::GetSettingsList(m_settings, m_network, SettingName(strArg), - ignore_default_section_config)) { + for (const util::SettingsValue &value : GetSettingsList(strArg)) { result.push_back( value.isFalse() ? "0" : value.isTrue() ? "1" : value.get_str()); } @@ -419,23 +393,23 @@ } bool ArgsManager::IsArgSet(const std::string &strArg) const { - return !ArgsManagerHelper::Get(*this, strArg).isNull(); + return !GetSetting(strArg).isNull(); } bool ArgsManager::IsArgNegated(const std::string &strArg) const { - return ArgsManagerHelper::Get(*this, strArg).isFalse(); + return GetSetting(strArg).isFalse(); } std::string ArgsManager::GetArg(const std::string &strArg, const std::string &strDefault) const { - const util::SettingsValue value = ArgsManagerHelper::Get(*this, strArg); + const util::SettingsValue value = GetSetting(strArg); return value.isNull() ? strDefault : value.isFalse() ? "0" : value.isTrue() ? "1" : value.get_str(); } int64_t ArgsManager::GetArg(const std::string &strArg, int64_t nDefault) const { - const util::SettingsValue value = ArgsManagerHelper::Get(*this, strArg); + const util::SettingsValue value = GetSetting(strArg); return value.isNull() ? nDefault : value.isFalse() @@ -446,7 +420,7 @@ } bool ArgsManager::GetBoolArg(const std::string &strArg, bool fDefault) const { - const util::SettingsValue value = ArgsManagerHelper::Get(*this, strArg); + const util::SettingsValue value = GetSetting(strArg); return value.isNull() ? fDefault : value.isBool() ? value.get_bool() : InterpretBool(value.get_str()); @@ -842,9 +816,9 @@ std::string key = option.first; util::SettingsValue value = InterpretOption(section, key, option.second); - const unsigned int flags = FlagsOfKnownArg(key); + Optional flags = GetArgFlags('-' + key); if (flags) { - if (!CheckValid(key, value, flags, error)) { + if (!CheckValid(key, value, *flags, error)) { return false; } m_settings.ro_config[section][key].push_back(value); @@ -879,8 +853,8 @@ return false; } // `-includeconf` cannot be included in the command line arguments - // except as `-noincludeconf` (which indicates that no conf file should - // be used). + // except as `-noincludeconf` (which indicates that no included conf + // file should be used). bool use_conf_file{true}; { LOCK(cs_args); @@ -969,9 +943,9 @@ auto get_net = [&](const std::string &arg) { LOCK(cs_args); util::SettingsValue value = - GetSetting(m_settings, /* section= */ "", SettingName(arg), - /* ignore_default_section_config= */ false, - /* get_chain_name= */ true); + util::GetSetting(m_settings, /* section= */ "", SettingName(arg), + /* ignore_default_section_config= */ false, + /* get_chain_name= */ true); return value.isNull() ? false : value.isBool() ? value.get_bool() : InterpretBool(value.get_str()); @@ -994,6 +968,25 @@ return GetArg("-chain", CBaseChainParams::MAIN); } +bool ArgsManager::UseDefaultSection(const std::string &arg) const { + return m_network == CBaseChainParams::MAIN || + m_network_only_args.count(arg) == 0; +} + +util::SettingsValue ArgsManager::GetSetting(const std::string &arg) const { + LOCK(cs_args); + return util::GetSetting(m_settings, m_network, SettingName(arg), + !UseDefaultSection(arg), + /* get_chain_name= */ false); +} + +std::vector +ArgsManager::GetSettingsList(const std::string &arg) const { + LOCK(cs_args); + return util::GetSettingsList(m_settings, m_network, SettingName(arg), + !UseDefaultSection(arg)); +} + bool RenameOver(fs::path src, fs::path dest) { #ifdef WIN32 return MoveFileExA(src.string().c_str(), dest.string().c_str(),