From 0e01d5b5f38104f1db3c9a1e2aca87c209c35a85 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sun, 17 Nov 2024 15:11:52 +0000 Subject: [PATCH] partial bitcoin#22766: Clarify and disable unused ArgsManager flags excludes: - c5d7e34bd9a4ad752c5ec88032420e2e90ab17ab --- src/util/system.cpp | 104 +++++++++++++++++++++++--------------------- src/util/system.h | 16 ++++--- 2 files changed, 65 insertions(+), 55 deletions(-) diff --git a/src/util/system.cpp b/src/util/system.cpp index 6834189aa6..112dbb9e09 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -79,6 +79,7 @@ #include #include #include +#include #include #include #include @@ -205,39 +206,60 @@ static std::string SettingName(const std::string& arg) return arg.size() > 0 && arg[0] == '-' ? arg.substr(1) : arg; } +struct KeyInfo { + std::string name; + std::string section; + bool negated{false}; +}; + /** - * Interpret -nofoo as if the user supplied -foo=0. + * Parse "name", "section.name", "noname", "section.noname" settings keys. * - * 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 false. - * - * If there was a double negative, it removes "no" from the key, and - * returns true. - * - * If there was no "no", it returns the string value untouched. - * - * Where an option was negated can be later checked using the + * @note 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). */ - -static util::SettingsValue InterpretOption(std::string& section, std::string& key, const std::string& value) +KeyInfo InterpretKey(std::string key) { + KeyInfo result; // Split section name from key name for keys like "testnet.foo" or "regtest.bar" size_t option_index = key.find('.'); if (option_index != std::string::npos) { - section = key.substr(0, option_index); + result.section = key.substr(0, option_index); key.erase(0, option_index + 1); } if (key.substr(0, 2) == "no") { key.erase(0, 2); + result.negated = true; + } + result.name = key; + return result; +} + +/** + * Interpret settings value based on registered flags. + * + * @param[in] key key information to know if key was negated + * @param[in] value string value of setting to be parsed + * @param[in] flags ArgsManager registered argument flags + * @param[out] error Error description if settings value is not valid + * + * @return parsed settings value if it is valid, otherwise nullopt accompanied + * by a descriptive error string + */ +static std::optional InterpretValue(const KeyInfo& key, const std::string& value, + unsigned int flags, std::string& error) +{ + // Return negated settings as false values. + if (key.negated) { + if (flags & ArgsManager::DISALLOW_NEGATION) { + error = strprintf("Negating of -%s is meaningless and therefore forbidden", key.name); + return std::nullopt; + } // Double negatives like -nofoo=0 are supported (but discouraged) if (!InterpretBool(value)) { - LogPrintf("Warning: parsed potentially confusing double-negative -%s=%s\n", key, value); + LogPrintf("Warning: parsed potentially confusing double-negative -%s=%s\n", key.name, value); return true; } return false; @@ -245,22 +267,6 @@ static util::SettingsValue InterpretOption(std::string& section, std::string& ke return value; } -/** - * Check settings value validity according to flags. - * - * TODO: Add more meaningful error checks here in the future - * See "here's how the flags are meant to behave" in - * https://github.com/bitcoin/bitcoin/pull/16097#issuecomment-514627823 - */ -static bool CheckValid(const std::string& key, const util::SettingsValue& val, unsigned int flags, std::string& error) -{ - if (val.isBool() && !(flags & ArgsManager::ALLOW_BOOL)) { - error = strprintf("Negating of -%s is meaningless and therefore forbidden", key); - return false; - } - return true; -} - // 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. @@ -366,21 +372,21 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin // Transform -foo to foo key.erase(0, 1); - std::string section; - util::SettingsValue value = InterpretOption(section, key, val); - std::optional flags = GetArgFlags('-' + key); + KeyInfo keyinfo = InterpretKey(key); + std::optional flags = GetArgFlags('-' + keyinfo.name); // Unknown command line options and command line options with dot - // characters (which are returned from InterpretOption with nonempty + // characters (which are returned from InterpretKey with nonempty // section strings) are not valid. - if (!flags || !section.empty()) { + if (!flags || !keyinfo.section.empty()) { error = strprintf("Invalid parameter %s", argv[i]); return false; } - if (!CheckValid(key, value, *flags, error)) return false; + std::optional value = InterpretValue(keyinfo, val, *flags, error); + if (!value) return false; - m_settings.command_line_options[key].push_back(value); + m_settings.command_line_options[keyinfo.name].push_back(*value); } // we do not allow -includeconf from command line, only -noincludeconf @@ -580,10 +586,8 @@ bool ArgsManager::ReadSettingsFile(std::vector* errors) return false; } for (const auto& setting : m_settings.rw_settings) { - std::string section; - std::string key = setting.first; - (void)InterpretOption(section, key, /* value */ {}); // Split setting key into section and argname - if (!GetArgFlags('-' + key)) { + KeyInfo key = InterpretKey(setting.first); // Split setting key into section and argname + if (!GetArgFlags('-' + key.name)) { LogPrintf("Ignoring unknown rw_settings value %s\n", setting.first); } } @@ -680,6 +684,7 @@ void ArgsManager::AddArg(const std::string& name, const std::string& help, unsig LOCK(cs_args); std::map& arg_map = m_available_args[cat]; + if ((flags & (ALLOW_ANY | ALLOW_BOOL)) == 0) flags |= DISALLOW_NEGATION; // Temporary, removed in next scripted-diff auto ret = arg_map.emplace(arg_name, Arg{name.substr(eq_index, name.size() - eq_index), help, flags}); assert(ret.second); // Make sure an insertion actually happened @@ -926,15 +931,14 @@ bool ArgsManager::ReadConfigStream(std::istream& stream, const std::string& file return false; } for (const std::pair& option : options) { - std::string section; - std::string key = option.first; - util::SettingsValue value = InterpretOption(section, key, option.second); - std::optional flags = GetArgFlags('-' + key); + KeyInfo key = InterpretKey(option.first); + std::optional flags = GetArgFlags('-' + key.name); if (flags) { - if (!CheckValid(key, value, *flags, error)) { + std::optional value = InterpretValue(key, option.second, *flags, error); + if (!value) { return false; } - m_settings.ro_config[section][key].push_back(value); + m_settings.ro_config[key.section][key.name].push_back(*value); } else { if (ignore_invalid_keys) { LogPrintf("Ignoring unknown configuration value %s\n", option.first); diff --git a/src/util/system.h b/src/util/system.h index b99a5f9a1e..376b6ca90b 100644 --- a/src/util/system.h +++ b/src/util/system.h @@ -178,12 +178,18 @@ struct SectionInfo class ArgsManager { public: + /** + * Flags controlling how config and command line arguments are validated and + * interpreted. + */ enum Flags : uint32_t { - // Boolean options can accept negation syntax -noOPTION or -noOPTION=1 - ALLOW_BOOL = 0x01, - ALLOW_INT = 0x02, - ALLOW_STRING = 0x04, - ALLOW_ANY = ALLOW_BOOL | ALLOW_INT | ALLOW_STRING, + ALLOW_ANY = 0x01, //!< disable validation + ALLOW_BOOL = 0x02, //!< unimplemented, draft implementation in #16545 + ALLOW_INT = 0x04, //!< unimplemented, draft implementation in #16545 + ALLOW_STRING = 0x08, //!< unimplemented, draft implementation in #16545 + ALLOW_LIST = 0x10, //!< unimplemented, draft implementation in #16545 + DISALLOW_NEGATION = 0x20, //!< disallow -nofoo syntax + DEBUG_ONLY = 0x100, /* Some options would cause cross-contamination if values for * mainnet were used while running on regtest/testnet (or vice-versa).