Produce error on unrecognized command-line args #215
Conversation
Output an error message and exit the program if there are any unrecognized command-line args or any unrecognized parameters in the config file.
|
Awesome! I'm travelling for the rest of the week, so please be patient with this one. |
|
While I was away the teamcity CI was turned off, I just started it up and this PR got an all green. |
| @@ -311,6 +311,7 @@ libbitcoin_util_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) | |||
| libbitcoin_util_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) | |||
| libbitcoin_util_a_SOURCES = \ | |||
| support/pagelocker.cpp \ | |||
| allowed_args.cpp \ | |||
There was a problem hiding this comment.
To allow gitian to build, the header file must also be added to the Makefile.am.
Please add it to the BITCOIN_CORE_H variable (approx line 70)
| @@ -0,0 +1,247 @@ | |||
| // Copyright (c) 2017 The Bitcoin Classic developers | |||
| // Distributed under the MIT software license, see the accompanying | |||
There was a problem hiding this comment.
I have not documented this well, but please be aware that you are allowed to use GPLv3 for new files. We have been shipping static-linked to Qt for so long that there effectively no change and many prefer that one. See networkmanager/* for any example headers.
There was a problem hiding this comment.
I'm not an expert here, but I generally prefer the MIT/BSD style license, because it's the most permissive. I thought the rest of the codebase was MIT licensed? I do see that some of the new files you've added are GPLv3, but I don't understand how that's compatible with the rest of the MIT licensed codebase. I'd love to read more of an explanation though!
There was a problem hiding this comment.
You can use MIT just fine, no worries. Its a personal preference and I wanted to make sure you knew you had a choice.
The licenses are compatible, as they should be because when we link to the static build of Qt we link bitcoin classic to the LGPLv3 licensed Qt libraries. Static linking means its to be interpreted as the GPLv3.
The FSF website also has a page on licenses compatible with the GPL.
| @@ -0,0 +1,247 @@ | |||
| // Copyright (c) 2017 The Bitcoin Classic developers | |||
There was a problem hiding this comment.
Please assign copyright to yourself. Assigning it to "The Bitcoin Classic developers" is not enforceable in most places as it is not a "legal entity".
There was a problem hiding this comment.
Makes sense. I have updated the header to assign myself the copyright
| @@ -371,6 +371,7 @@ void ParseParameters(int argc, const char* const argv[]) | |||
| if (str.length() > 1 && str[1] == '-') | |||
| str = str.substr(1); | |||
| InterpretNegativeSetting(str, strValue); | |||
| checkArgFunc(str.substr(1)); | |||
There was a problem hiding this comment.
I'm thinking a str.length() (or empty()) may be good here.
There was a problem hiding this comment.
I don't think that check is necessary here, because a few lines above is:
if (str[0] != '-')
break;
So, at this point, we know the string is at least length 1.
Also add allowed_args.h to Makefile.
Partially resolves #172
Output an error message and exit the program if there are any unrecognized command-line args or any unrecognized parameters in the config file.
A new class is created:
AllowedArgs. This class contains sets that define which arguments are allowed for each executable. It also defines a function for each of the executable types. These functions are passed in as a new required argument to theParseParametersmethod in util.cpp.All tests are updated to use allowed arguments. A new test is created which exercises parsing an invalid argument.
Several custom error messages for invalid args are removed, since the checking is now done earlier in the program execution.
In order to determine the set of allowed arguments, I primarily looked at the help documentation in the following locations:
bitcoind: init.cppbitcoin-qt: qt/utilitydialog.cpp (combined with the bitcoind args)bitcoin-cli: bitcoin-cli.cpp.bitcoin-tx: bitcoin-tx.cpp.In addition, I grepped through the codebase using the following searches:
grep -r --exclude-dir=test -E "Get(Bool)?Arg\(\"\S*" .grep -r --exclude-dir=test -E "map(Mutli)?Args.*?\"" .Using these searches, I found 2 additional allowed arguments: "excessiveblocksize", and "use-thinblocks". I added help documentation to init.cpp for "use-thinblocks".
This change also fixes 2 related issues that came up:
bitcoin-tx. This standardizes the set of help arguments across all executables.BitcoinApplicationbefore parsing the command-line options. This allows for any command-line parsing error messages to be shown in a gui window. This also fixes a bug that prevented the 'uiplatform' argument from taking effect if it was specified in the config file.This change only checks for unrecognized args, it does not do any type checking on the values of the arguments. This is already quite large, so I wanted to limit the scope somewhat. However, it is designed such that adding support for type checking should be do-able without a major redesign - most of the future changes will simply need to be made to the
AllowedArgsclass.