Skip to content

Commit 72a6f56

Browse files
FuzzbawlsFuzzbawls
committed
Track negated arguments in the argument paser.
This commit adds tracking for negated arguments. This change will be used in a future commit that allows disabling the debug.log file using -nodebuglogfile.
1 parent 3142e1b commit 72a6f56

File tree

3 files changed

+106
-35
lines changed

3 files changed

+106
-35
lines changed

‎src/test/util_tests.cpp‎

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -94,17 +94,11 @@ BOOST_AUTO_TEST_CASE(util_DateTimeStrFormat)
9494
BOOST_CHECK_EQUAL(DateTimeStrFormat("%a, %d %b %Y %H:%M:%S +0000", 1317425777), "Fri, 30 Sep 2011 23:36:17 +0000");
9595
}
9696

97-
class TestArgsManager : public ArgsManager
97+
struct TestArgsManager : public ArgsManager
9898
{
99-
public:
100-
std::map<std::string, std::string>& GetMapArgs()
101-
{
102-
return mapArgs;
103-
};
104-
const std::map<std::string, std::vector<std::string> >& GetMapMultiArgs()
105-
{
106-
return mapMultiArgs;
107-
};
99+
std::map<std::string, std::string>& GetMapArgs() { return mapArgs; }
100+
const std::map<std::string, std::vector<std::string> >& GetMapMultiArgs() { return mapMultiArgs; }
101+
const std::unordered_set<std::string>& GetNegatedArgs() { return m_negated_args; }
108102
};
109103

110104
BOOST_AUTO_TEST_CASE(util_ParseParameters)
@@ -150,6 +144,11 @@ BOOST_AUTO_TEST_CASE(util_GetBoolArg)
150144
// The -no prefix should get stripped on the way in.
151145
BOOST_CHECK(!testArgs.IsArgSet("-nob"));
152146

147+
// The -b option is flagged as negated, and nothing else is
148+
BOOST_CHECK(testArgs.IsArgNegated("-b"));
149+
BOOST_CHECK(testArgs.GetNegatedArgs().size() == 1);
150+
BOOST_CHECK(!testArgs.IsArgNegated("-a"));
151+
153152
// Check expected values.
154153
BOOST_CHECK(testArgs.GetBoolArg("-a", false) == true);
155154
BOOST_CHECK(testArgs.GetBoolArg("-b", true) == false);
@@ -159,6 +158,22 @@ BOOST_AUTO_TEST_CASE(util_GetBoolArg)
159158
BOOST_CHECK(testArgs.GetBoolArg("-f", true) == false);
160159
}
161160

161+
BOOST_AUTO_TEST_CASE(util_GetBoolArgEdgeCases)
162+
{
163+
// Test some awful edge cases that hopefully no user will ever exercise.
164+
TestArgsManager testArgs;
165+
const char *argv_test[] = {"ignored", "-nofoo", "-foo", "-nobar=0"};
166+
testArgs.ParseParameters(4, (char**)argv_test);
167+
168+
// This was passed twice, second one overrides the negative setting.
169+
BOOST_CHECK(!testArgs.IsArgNegated("-foo"));
170+
BOOST_CHECK(testArgs.GetBoolArg("-foo", false) == true);
171+
172+
// A double negative is a positive.
173+
BOOST_CHECK(testArgs.IsArgNegated("-bar"));
174+
BOOST_CHECK(testArgs.GetBoolArg("-bar", false) == true);
175+
}
176+
162177
BOOST_AUTO_TEST_CASE(util_GetArg)
163178
{
164179
TestArgsManager testArgs;

‎src/util.cpp‎

Lines changed: 65 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -74,9 +74,7 @@
7474
#include <sys/prctl.h>
7575
#endif
7676

77-
#include <boost/algorithm/string/case_conv.hpp> // for to_lower()
7877
#include <boost/algorithm/string/join.hpp>
79-
#include <boost/algorithm/string/predicate.hpp> // for startswith() and endswith()
8078
#include <boost/program_options/detail/config_file.hpp>
8179
#include <boost/program_options/parsers.hpp>
8280
#include <boost/thread.hpp>
@@ -160,20 +158,54 @@ class CInit
160158
} instance_of_cinit;
161159

162160

163-
/** Interpret string as boolean, for argument parsing */
161+
/**
162+
* Interpret a string argument as a boolean.
163+
*
164+
* The definition of atoi() requires that non-numeric string values like "foo",
165+
* return 0. This means that if a user unintentionally supplies a non-integer
166+
* argument here, the return value is always false. This means that -foo=false
167+
* does what the user probably expects, but -foo=true is well defined but does
168+
* not do what they probably expected.
169+
*
170+
* The return value of atoi() is undefined when given input not representable as
171+
* an int. On most systems this means string value between "-2147483648" and
172+
* "2147483647" are well defined (this method will return true). Setting
173+
* -txindex=2147483648 on most systems, however, is probably undefined.
174+
*
175+
* For a more extensive discussion of this topic (and a wide range of opinions
176+
* on the Right Way to change this code), see upstream PR12713.
177+
*/
164178
static bool InterpretBool(const std::string& strValue)
165179
{
166180
if (strValue.empty())
167181
return true;
168182
return (atoi(strValue) != 0);
169183
}
170184

171-
/** Turn -noX into -X=0 */
172-
static void InterpretNegativeSetting(std::string& strKey, std::string& strValue)
185+
/**
186+
* Interpret -nofoo as if the user supplied -foo=0.
187+
*
188+
* This method also tracks when the -no form was supplied, and treats "-foo" as
189+
* a negated option when this happens. This can be later checked using the
190+
* IsArgNegated() method. One use case for this is to have a way to disable
191+
* options that are not normally boolean (e.g. using -nodebuglogfile to request
192+
* that debug log output is not sent to any file at all).
193+
*/
194+
void ArgsManager::InterpretNegatedOption(std::string& key, std::string& val)
173195
{
174-
if (strKey.length()>3 && strKey[0]=='-' && strKey[1]=='n' && strKey[2]=='o') {
175-
strKey = "-" + strKey.substr(3);
176-
strValue = InterpretBool(strValue) ? "0" : "1";
196+
if (key.substr(0, 3) == "-no") {
197+
bool bool_val = InterpretBool(val);
198+
if (!bool_val ) {
199+
// Double negatives like -nofoo=0 are supported (but discouraged)
200+
LogPrintf("Warning: parsed potentially confusing double-negative %s=%s\n", key, val);
201+
}
202+
key.erase(1, 2);
203+
m_negated_args.insert(key);
204+
val = bool_val ? "0" : "1";
205+
} else {
206+
// In an invocation like "bitcoind -nofoo -foo" we want to unmark -foo
207+
// as negated when we see the second option.
208+
m_negated_args.erase(key);
177209
}
178210
}
179211

@@ -182,32 +214,34 @@ void ArgsManager::ParseParameters(int argc, const char* const argv[])
182214
LOCK(cs_args);
183215
mapArgs.clear();
184216
mapMultiArgs.clear();
217+
m_negated_args.clear();
185218

186219
for (int i = 1; i < argc; i++) {
187-
std::string str(argv[i]);
188-
std::string strValue;
189-
size_t is_index = str.find('=');
220+
std::string key(argv[i]);
221+
std::string val;
222+
size_t is_index = key.find('=');
190223
if (is_index != std::string::npos) {
191-
strValue = str.substr(is_index + 1);
192-
str = str.substr(0, is_index);
224+
val = key.substr(is_index + 1);
225+
key.erase(is_index);
193226
}
194227
#ifdef WIN32
195-
boost::to_lower(str);
196-
if (boost::algorithm::starts_with(str, "/"))
197-
str = "-" + str.substr(1);
228+
std::transform(key.begin(), key.end(), key.begin(), ::tolower);
229+
if (key[0] == '/')
230+
key[0] = '-';
198231
#endif
199232

200-
if (str[0] != '-')
233+
if (key[0] != '-')
201234
break;
202235

203-
// Interpret --foo as -foo.
204-
// If both --foo and -foo are set, the last takes effect.
205-
if (str.length() > 1 && str[1] == '-')
206-
str = str.substr(1);
207-
InterpretNegativeSetting(str, strValue);
236+
// Transform --foo to -foo
237+
if (key.length() > 1 && key[1] == '-')
238+
key.erase(0, 1);
208239

209-
mapArgs[str] = strValue;
210-
mapMultiArgs[str].push_back(strValue);
240+
// Transform -nofoo to -foo=0
241+
InterpretNegatedOption(key, val);
242+
243+
mapArgs[key] = val;
244+
mapMultiArgs[key].push_back(val);
211245
}
212246
}
213247

@@ -225,6 +259,12 @@ bool ArgsManager::IsArgSet(const std::string& strArg) const
225259
return mapArgs.count(strArg);
226260
}
227261

262+
bool ArgsManager::IsArgNegated(const std::string& strArg) const
263+
{
264+
LOCK(cs_args);
265+
return m_negated_args.find(strArg) != m_negated_args.end();
266+
}
267+
228268
std::string ArgsManager::GetArg(const std::string& strArg, const std::string& strDefault) const
229269
{
230270
LOCK(cs_args);
@@ -507,7 +547,7 @@ void ArgsManager::ReadConfigFile()
507547
// Don't overwrite existing settings so command line settings override pivx.conf
508548
std::string strKey = std::string("-") + it->string_key;
509549
std::string strValue = it->value[0];
510-
InterpretNegativeSetting(strKey, strValue);
550+
InterpretNegatedOption(strKey, strValue);
511551
if (mapArgs.count(strKey) == 0)
512552
mapArgs[strKey] = strValue;
513553
mapMultiArgs[strKey].push_back(strValue);

‎src/util.h‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
#include <map>
3030
#include <stdint.h>
3131
#include <string>
32+
#include <unordered_set>
3233
#include <vector>
3334

3435
#include <boost/thread/exceptions.hpp>
@@ -108,6 +109,7 @@ class ArgsManager
108109
mutable RecursiveMutex cs_args;
109110
std::map<std::string, std::string> mapArgs;
110111
std::map<std::string, std::vector<std::string>> mapMultiArgs;
112+
std::unordered_set<std::string> m_negated_args;
111113

112114
public:
113115
void ParseParameters(int argc, const char* const argv[]);
@@ -129,6 +131,15 @@ class ArgsManager
129131
*/
130132
bool IsArgSet(const std::string& strArg) const;
131133

134+
/**
135+
* Return true if the argument was originally passed as a negated option,
136+
* i.e. -nofoo.
137+
*
138+
* @param strArg Argument to get (e.g. "-foo")
139+
* @return true if the argument was passed negated
140+
*/
141+
bool IsArgNegated(const std::string& strArg) const;
142+
132143
/**
133144
* Return string argument or default value
134145
*
@@ -176,6 +187,11 @@ class ArgsManager
176187

177188
// Forces a arg setting, used only in testing
178189
void ForceSetArg(const std::string& strArg, const std::string& strValue);
190+
191+
private:
192+
193+
// Munge -nofoo into -foo=0 and track the value as negated.
194+
void InterpretNegatedOption(std::string &key, std::string &val);
179195
};
180196

181197
extern ArgsManager gArgs;

0 commit comments

Comments
 (0)