diff --git a/doc/release-notes.md b/doc/release-notes.md --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -12,4 +12,6 @@ - The log timestamp format is now ISO 8601 (e.g. "2019-01-28T15:41:17Z") - Behavior change: in case of multiple values for an argument, the following rules apply: - From the command line, the *last* value takes precedence - - From the config file, the *first* value takes precedence \ No newline at end of file + - From the config file, the *first* value takes precedence + - From the config file, if an argument is negated it takes precedent over all the + previous occurences of this argument (e.g. "foo=2 \n nofoo=1" will set foo=0) \ No newline at end of file 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 @@ -176,9 +176,6 @@ 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 streamConfig(str_config); { @@ -245,7 +242,6 @@ // The -b option is flagged as negated, and nothing else is BOOST_CHECK(testArgs.IsArgNegated("-b")); - BOOST_CHECK(testArgs.GetNegatedArgs().size() == 1); BOOST_CHECK(!testArgs.IsArgNegated("-a")); // Check expected values. @@ -269,8 +265,8 @@ BOOST_CHECK(!testArgs.IsArgNegated("-foo")); BOOST_CHECK(testArgs.GetArg("-foo", "xxx") == ""); - // A double negative is a positive. - BOOST_CHECK(testArgs.IsArgNegated("-bar")); + // A double negative is a positive, and not marked as negated. + BOOST_CHECK(!testArgs.IsArgNegated("-bar")); BOOST_CHECK(testArgs.GetArg("-bar", "xxx") == "1"); // Config test @@ -279,12 +275,12 @@ testArgs.ReadConfigString(conf_test); // This was passed twice, second one overrides the negative setting, - // but not the value. + // and the value. BOOST_CHECK(!testArgs.IsArgNegated("-foo")); - BOOST_CHECK(testArgs.GetArg("-foo", "xxx") == "0"); + BOOST_CHECK(testArgs.GetArg("-foo", "xxx") == "1"); - // A double negative is a positive. - BOOST_CHECK(testArgs.IsArgNegated("-bar")); + // A double negative is a positive, and does not count as negated. + BOOST_CHECK(!testArgs.IsArgNegated("-bar")); BOOST_CHECK(testArgs.GetArg("-bar", "xxx") == "1"); // Combined test @@ -294,18 +290,15 @@ testArgs.ReadConfigString(combo_test_conf); // Command line overrides, but doesn't erase old setting - BOOST_CHECK(!testArgs.IsArgNegated("-foo")); + BOOST_CHECK(testArgs.IsArgNegated("-foo")); BOOST_CHECK(testArgs.GetArg("-foo", "xxx") == "0"); - BOOST_CHECK(testArgs.GetArgs("-foo").size() == 2 && - testArgs.GetArgs("-foo").front() == "0" && - testArgs.GetArgs("-foo").back() == "1"); + BOOST_CHECK(testArgs.GetArgs("-foo").size() == 0); // Command line overrides, but doesn't erase old setting - BOOST_CHECK(testArgs.IsArgNegated("-bar")); + BOOST_CHECK(!testArgs.IsArgNegated("-bar")); BOOST_CHECK(testArgs.GetArg("-bar", "xxx") == ""); - BOOST_CHECK(testArgs.GetArgs("-bar").size() == 2 && - testArgs.GetArgs("-bar").front() == "" && - testArgs.GetArgs("-bar").back() == "0"); + BOOST_CHECK(testArgs.GetArgs("-bar").size() == 1 && + testArgs.GetArgs("-bar").front() == ""); } BOOST_AUTO_TEST_CASE(util_ReadConfigStream) { @@ -350,10 +343,8 @@ test_args.GetArg("-d", "xxx") == "e" && test_args.GetArg("-fff", "xxx") == "0" && test_args.GetArg("-ggg", "xxx") == "1" && - // 1st value takes precedence - test_args.GetArg("-h", "xxx") == "1" && - // 1st value takes precedence - test_args.GetArg("-i", "xxx") == "0" && + test_args.GetArg("-h", "xxx") == "0" && + test_args.GetArg("-i", "xxx") == "1" && test_args.GetArg("-zzz", "xxx") == "xxx"); for (bool def : {false, true}) { @@ -363,8 +354,8 @@ !test_args.GetBoolArg("-d", def) && !test_args.GetBoolArg("-fff", def) && test_args.GetBoolArg("-ggg", def) && - test_args.GetBoolArg("-h", def) && - !test_args.GetBoolArg("-i", def) && + !test_args.GetBoolArg("-h", def) && + test_args.GetBoolArg("-i", def) && test_args.GetBoolArg("-zzz", def) == def); } @@ -375,19 +366,15 @@ BOOST_CHECK(test_args.GetArgs("-ccc").size() == 2 && test_args.GetArgs("-ccc").front() == "argument" && test_args.GetArgs("-ccc").back() == "multiple"); - BOOST_CHECK(test_args.GetArgs("-fff").size() == 1 && - test_args.GetArgs("-fff").front() == "0"); + BOOST_CHECK(test_args.GetArgs("-fff").size() == 0); BOOST_CHECK(test_args.GetArgs("-nofff").size() == 0); BOOST_CHECK(test_args.GetArgs("-ggg").size() == 1 && test_args.GetArgs("-ggg").front() == "1"); BOOST_CHECK(test_args.GetArgs("-noggg").size() == 0); - BOOST_CHECK(test_args.GetArgs("-h").size() == 2 && - test_args.GetArgs("-h").front() == "1" && - test_args.GetArgs("-h").back() == "0"); + BOOST_CHECK(test_args.GetArgs("-h").size() == 0); BOOST_CHECK(test_args.GetArgs("-noh").size() == 0); - BOOST_CHECK(test_args.GetArgs("-i").size() == 2 && - test_args.GetArgs("-i").front() == "0" && - test_args.GetArgs("-i").back() == "1"); + BOOST_CHECK(test_args.GetArgs("-i").size() == 1 && + test_args.GetArgs("-i").front() == "1"); BOOST_CHECK(test_args.GetArgs("-noi").size() == 0); BOOST_CHECK(test_args.GetArgs("-zzz").size() == 0); @@ -396,8 +383,7 @@ BOOST_CHECK(!test_args.IsArgNegated("-ccc")); BOOST_CHECK(!test_args.IsArgNegated("-d")); BOOST_CHECK(test_args.IsArgNegated("-fff")); - // IsArgNegated==true when noggg=0 - BOOST_CHECK(test_args.IsArgNegated("-ggg")); + BOOST_CHECK(!test_args.IsArgNegated("-ggg")); // last setting takes precedence BOOST_CHECK(test_args.IsArgNegated("-h")); // last setting takes precedence diff --git a/src/util.h b/src/util.h --- a/src/util.h +++ b/src/util.h @@ -100,7 +100,6 @@ 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); @@ -196,10 +195,6 @@ // Remove an arg setting, used only in testing void ClearArg(const std::string &strArg); - -private: - // Munge -nofoo into -foo=0 and track the value as negated. - void InterpretNegatedOption(std::string &key, std::string &val); }; extern ArgsManager gArgs; diff --git a/src/util.cpp b/src/util.cpp --- a/src/util.cpp +++ b/src/util.cpp @@ -236,35 +236,43 @@ /** * Interpret -nofoo as if the user supplied -foo=0. * - * This method also tracks when the -no form was supplied, and treats "-foo" as - * a negated option when this happens. This can be later checked using the - * IsArgNegated() method. One use case for this is to have a way to disable - * options that are not normally boolean (e.g. using -nodebuglogfile to request - * that debug log output is not sent to any file at all). + * This method also tracks when the -no form was supplied, and if so, checks + * whether there was a double-negative (-nofoo=0 -> -foo=1). + * + * If there was not a double negative, it removes the "no" from the key, and + * returns true, indicating the caller should clear the args vector to indicate + * a negated option. + * + * If there was a double negative, it removes "no" from the key, sets the value + * to "1" and returns false. + * + * If there was no "no", it leaves key and value untouched and returns false. + * + * Where an option was negated can be later checked using the IsArgNegated() + * method. One use case for this is to have a way to disable options that are + * not normally boolean (e.g. using -nodebuglogfile to request that debug log + * output is not sent to any file at all). */ -void ArgsManager::InterpretNegatedOption(std::string &key, std::string &val) { +static bool InterpretNegatedOption(std::string &key, std::string &val) { if (key.substr(0, 3) == "-no") { bool bool_val = InterpretBool(val); + key.erase(1, 2); if (!bool_val) { // Double negatives like -nofoo=0 are supported (but discouraged) LogPrintf( "Warning: parsed potentially confusing double-negative %s=%s\n", key, val); + val = "1"; + } else { + return true; } - key.erase(1, 2); - m_negated_args.insert(key); - val = bool_val ? "0" : "1"; - } else { - // In an invocation like "bitcoind -nofoo -foo" we want to unmark -foo - // as negated when we see the second option. - m_negated_args.erase(key); } + return false; } void ArgsManager::ParseParameters(int argc, const char *const argv[]) { LOCK(cs_args); m_override_args.clear(); - m_negated_args.clear(); for (int i = 1; i < argc; i++) { std::string key(argv[i]); @@ -290,15 +298,21 @@ key.erase(0, 1); } - // Transform -nofoo to -foo=0 - InterpretNegatedOption(key, val); - - m_override_args[key].push_back(val); + // Check for -nofoo + if (InterpretNegatedOption(key, val)) { + m_override_args[key].clear(); + } else { + m_override_args[key].push_back(val); + } } } std::vector ArgsManager::GetArgs(const std::string &strArg) const { std::vector result = {}; + // special case + if (IsArgNegated(strArg)) { + return result; + } LOCK(cs_args); ArgsManagerHelper::AddArgs(result, m_override_args, strArg); @@ -307,16 +321,34 @@ } bool ArgsManager::IsArgSet(const std::string &strArg) const { + // special case + if (IsArgNegated(strArg)) { + return true; + } return ArgsManagerHelper::GetArg(*this, strArg).first; } bool ArgsManager::IsArgNegated(const std::string &strArg) const { LOCK(cs_args); - return m_negated_args.find(strArg) != m_negated_args.end(); + + const auto &ov = m_override_args.find(strArg); + if (ov != m_override_args.end()) { + return ov->second.empty(); + } + + const auto &cf = m_config_args.find(strArg); + if (cf != m_config_args.end()) { + return cf->second.empty(); + } + + return false; } std::string ArgsManager::GetArg(const std::string &strArg, const std::string &strDefault) const { + if (IsArgNegated(strArg)) { + return "0"; + } std::pair found_res = ArgsManagerHelper::GetArg(*this, strArg); if (found_res.first) { @@ -326,6 +358,9 @@ } int64_t ArgsManager::GetArg(const std::string &strArg, int64_t nDefault) const { + if (IsArgNegated(strArg)) { + return 0; + } std::pair found_res = ArgsManagerHelper::GetArg(*this, strArg); if (found_res.first) { @@ -335,6 +370,9 @@ } bool ArgsManager::GetBoolArg(const std::string &strArg, bool fDefault) const { + if (IsArgNegated(strArg)) { + return false; + } std::pair found_res = ArgsManagerHelper::GetArg(*this, strArg); if (found_res.first) { @@ -517,12 +555,13 @@ it(stream, setOptions), end; it != end; ++it) { - // Don't overwrite existing settings so command line settings override - // bitcoin.conf std::string strKey = std::string("-") + it->string_key; std::string strValue = it->value[0]; - InterpretNegatedOption(strKey, strValue); - m_config_args[strKey].push_back(strValue); + if (InterpretNegatedOption(strKey, strValue)) { + m_config_args[strKey].clear(); + } else { + m_config_args[strKey].push_back(strValue); + } } }