-
Notifications
You must be signed in to change notification settings - Fork 37.7k
util: Make default arg values more specific #19471
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Updated f0add14 -> 6a5f5d8 (pr19471.01 -> pr19471.02, diff):
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
Rebased 6a5f5d8 -> c103052 (pr19471.02 -> pr19471.04) due to the conflicts with #15935 and #18923. |
re-run ci |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK 1a02a03. No changes, just rebase and a few conflict resolutions
Rebased 1a02a03 -> d796dd0 (pr19471.11 -> pr19471.12) due to the conflict with #21007. |
Rebased d796dd0 -> 2ce271c (pr19471.12 -> pr19471.13) on top of the #21447. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK 2ce271c. Just rebased after conflict
src/qt/bitcoin.cpp
Outdated
// Default printtoconsole to false for the GUI. GUI programs should not | ||
// print to the console unnecessarily. | ||
gArgs.SoftSetBoolArg("-printtoconsole", false); | ||
gArgs.SoftSetBoolArg("-printtoconsole", SERVER_ARGS_OPTIONS.printtoconsole_default); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the point of overwriting the arg here, but keeping the default value as !args.GetBoolArg("-daemon", false)
where it is parsed? It would be cleaner to not do any overwriting here and simply use printtoconsole_default as default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides
Line 894 in 2ce271c
LogInstance().m_print_to_console = args.GetBoolArg("-printtoconsole", !args.GetBoolArg("-daemon", false)); |
bitcoin/src/bitcoin-wallet.cpp
Line 67 in 2ce271c
LogInstance().m_print_to_console = args.GetBoolArg("-printtoconsole", args.GetBoolArg("-debug", false)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bitcoin-wallet is a completely separate binary that isn't even modified in this PR. I fail to see how this is relevant to the discussion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be cleaner to not do any overwriting here and simply use printtoconsole_default as default value.
Yes, you right. But at the parsing place the SERVER_ARGS_OPTIONS
is out of the scope. It is possible to bring it into the scope by adding a data member to the ArgsManager
but it will make a diff bigger. Is it worth it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why add a data member to ArgsManager? Just pass it into InitLogging
by reference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pass it into
InitLogging
by reference?
What about interfaces::Node::initLogging()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the point of overwriting the arg here, but keeping the default value as
!args.GetBoolArg("-daemon", false)
where it is parsed? It would be cleaner to not do any overwriting here and simply use printtoconsole_default as default value.
It will change the current behavior, as for bitcoind
the printtoconsole_default
must be disabled when the -daemon
option is set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Updated.
A struct is used instead of bit flags as args could have non-boolean types.
This change makes -help command print correct default values for some options.
Updated 2ce271c -> e719835 (pr19471.13 -> pr19471.14, diff):
|
Updated e719835 -> ccc0388 (pr19471.14 -> pr19471.15, diff):
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
Sorry, I think I created a lot of conflicts with here with #19160 and #21732. But I think this can be rebased as a change like the diff below. In the diff I consolidated the I think this all could be cleaner after the change in #10102 adding separate BitcoinNodeInit / BitcoinWalletInit / BitcoinGuiInit / BitcoinQtInit / BitcoindInit classes, since it will be easier to add custom hooks for each executable. I will try to split that change into a smaller PR, but there is no need to delay better behavior implemented in this PR waiting for IPC stuff. diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
index cf9e4fad44d..7e44ca7517d 100644
--- a/src/bitcoind.cpp
+++ b/src/bitcoind.cpp
@@ -170,10 +170,8 @@ static bool AppInit(NodeContext& node, int argc, char* argv[])
return false;
}
- // -server defaults to true for bitcoind but not for the GUI so do this here
- args.SoftSetBoolArg("-server", true);
// Set this early so that parameter interactions go to console
- InitLogging(args);
+ InitLogging(args, node.is_gui);
InitParameterInteraction(args);
if (!AppInitBasicSetup(args)) {
// InitError will have been called with detailed error, which ends up on console
diff --git a/src/init.cpp b/src/init.cpp
index 24d67f48dc1..09faa05f28c 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -347,7 +347,7 @@ void SetupServerArgs(NodeContext& node)
SetupHelpOptions(argsman);
argsman.AddArg("-help-debug", "Print help message with debugging options and exit", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); // server-only for now
- init::AddLoggingArgs(argsman);
+ init::AddLoggingArgs(argsman, /* can_daemonize= */ !node.is_gui, /* default_printtoconsole= */ !node.is_gui);
const auto defaultBaseParams = CreateBaseChainParams(CBaseChainParams::MAIN);
const auto testnetBaseParams = CreateBaseChainParams(CBaseChainParams::TESTNET);
@@ -545,11 +545,16 @@ void SetupServerArgs(NodeContext& node)
argsman.AddArg("-rpcwhitelist=<whitelist>", "Set a whitelist to filter incoming RPC calls for a specific user. The field <whitelist> comes in the format: <USERNAME>:<rpc 1>,<rpc 2>,...,<rpc n>. If multiple whitelists are set for a given user, they are set-intersected. See -rpcwhitelistdefault documentation for information on default whitelist behavior.", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
argsman.AddArg("-rpcwhitelistdefault", "Sets default behavior for rpc whitelisting. Unless rpcwhitelistdefault is set to 0, if any -rpcwhitelist is set, the rpc server acts as if all rpc users are subject to empty-unless-otherwise-specified whitelists. If rpcwhitelistdefault is set to 1 and no -rpcwhitelist is set, rpc server acts as if all rpc users are subject to empty whitelists.", ArgsManager::ALLOW_BOOL, OptionsCategory::RPC);
argsman.AddArg("-rpcworkqueue=<n>", strprintf("Set the depth of the work queue to service RPC calls (default: %d)", DEFAULT_HTTP_WORKQUEUE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::RPC);
- argsman.AddArg("-server", "Accept command line and JSON-RPC commands", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
+ argsman.AddArg("-server", strprintf("Accept command line and JSON-RPC commands (default: %u)", !node.is_gui), ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
#if HAVE_DECL_FORK
+ if (!node.is_gui) {
argsman.AddArg("-daemon", strprintf("Run in the background as a daemon and accept commands (default: %d)", DEFAULT_DAEMON), ArgsManager::ALLOW_BOOL, OptionsCategory::OPTIONS);
argsman.AddArg("-daemonwait", strprintf("Wait for initialization to be finished before exiting. This implies -daemon (default: %d)", DEFAULT_DAEMONWAIT), ArgsManager::ALLOW_BOOL, OptionsCategory::OPTIONS);
+ } else {
+ hidden_args.emplace_back("-daemon");
+ hidden_args.emplace_back("-daemonwait");
+ }
#else
hidden_args.emplace_back("-daemon");
hidden_args.emplace_back("-daemonwait");
@@ -740,9 +745,10 @@ void InitParameterInteraction(ArgsManager& args)
* Note that this is called very early in the process lifetime, so you should be
* careful about what global state you rely on here.
*/
-void InitLogging(const ArgsManager& args)
+void InitLogging(const ArgsManager& args, bool is_gui)
{
- init::SetLoggingOptions(args);
+ init::SetLoggingOptions(args, /* is_daemon= */ args.GetBoolArg("-daemon", DEFAULT_DAEMON) || args.GetBoolArg("-daemonwait", DEFAULT_DAEMONWAIT), /* default_printtoconsole= */ !is_gui);
+
init::LogPackageVersion();
}
@@ -1169,7 +1175,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
* that the server is there and will be ready later). Warmup mode will
* be disabled when initialisation is finished.
*/
- if (args.GetBoolArg("-server", false)) {
+ if (args.GetBoolArg("-server", !node.is_gui)) {
uiInterface.InitMessage_connect(SetRPCWarmupStatus);
if (!AppInitServers(node))
return InitError(_("Unable to start HTTP server. See debug log for details."));
diff --git a/src/init.h b/src/init.h
index 328eda9c7e8..b0c953ab8a3 100644
--- a/src/init.h
+++ b/src/init.h
@@ -28,7 +28,7 @@ class thread_group;
void Interrupt(NodeContext& node);
void Shutdown(NodeContext& node);
//!Initialize the logging infrastructure
-void InitLogging(const ArgsManager& args);
+void InitLogging(const ArgsManager& args, bool is_gui);
//!Parameter interaction: change current parameters depending on various rules
void InitParameterInteraction(ArgsManager& args);
diff --git a/src/init/common.cpp b/src/init/common.cpp
index 79e0c9da782..8ba2262b4f3 100644
--- a/src/init/common.cpp
+++ b/src/init/common.cpp
@@ -58,7 +58,7 @@ bool SanityChecks()
return true;
}
-void AddLoggingArgs(ArgsManager& argsman)
+void AddLoggingArgs(ArgsManager& argsman, bool can_daemonize, bool default_printtoconsole)
{
argsman.AddArg("-debuglogfile=<file>", strprintf("Specify location of debug log file. Relative paths will be prefixed by a net-specific datadir location. (-nodebuglogfile to disable; default: %s)", DEFAULT_DEBUGLOGFILE), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
argsman.AddArg("-debug=<category>", "Output debugging information (default: -nodebug, supplying <category> is optional). "
@@ -74,15 +74,19 @@ void AddLoggingArgs(ArgsManager& argsman)
#endif
argsman.AddArg("-logsourcelocations", strprintf("Prepend debug output with name of the originating source location (source file, line number and function name) (default: %u)", DEFAULT_LOGSOURCELOCATIONS), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
argsman.AddArg("-logtimemicros", strprintf("Add microsecond precision to debug timestamps (default: %u)", DEFAULT_LOGTIMEMICROS), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
- argsman.AddArg("-printtoconsole", "Send trace/debug info to console (default: 1 when no -daemon. To disable logging to file, set -nodebuglogfile)", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
+ argsman.AddArg("-printtoconsole", strprintf("Send trace/debug info to console (default: %u).%s To disable logging to file, set -nodebuglogfile", default_printtoconsole, can_daemonize ? " Disabled when -daemon=1." : ""), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
argsman.AddArg("-shrinkdebugfile", "Shrink debug.log file on client startup (default: 1 when no -debug)", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
}
-void SetLoggingOptions(const ArgsManager& args)
+void SetLoggingOptions(const ArgsManager& args, bool is_daemon, bool default_printtoconsole)
{
LogInstance().m_print_to_file = !args.IsArgNegated("-debuglogfile");
LogInstance().m_file_path = AbsPathForConfigVal(args.GetArg("-debuglogfile", DEFAULT_DEBUGLOGFILE));
- LogInstance().m_print_to_console = args.GetBoolArg("-printtoconsole", !args.GetBoolArg("-daemon", false));
+ if (is_daemon) {
+ LogInstance().m_print_to_console = false;
+ } else {
+ LogInstance().m_print_to_console = args.GetBoolArg("-printtoconsole", default_printtoconsole);
+ }
LogInstance().m_log_timestamps = args.GetBoolArg("-logtimestamps", DEFAULT_LOGTIMESTAMPS);
LogInstance().m_log_time_micros = args.GetBoolArg("-logtimemicros", DEFAULT_LOGTIMEMICROS);
#ifdef HAVE_THREAD_LOCAL
diff --git a/src/init/common.h b/src/init/common.h
index fc4bc1b2800..6efa7917093 100644
--- a/src/init/common.h
+++ b/src/init/common.h
@@ -18,8 +18,8 @@ void UnsetGlobals();
* necessary library support.
*/
bool SanityChecks();
-void AddLoggingArgs(ArgsManager& args);
-void SetLoggingOptions(const ArgsManager& args);
+void AddLoggingArgs(ArgsManager& argsman, bool can_daemonize, bool default_printtoconsole);
+void SetLoggingOptions(const ArgsManager& args, bool is_daemon, bool default_printtoconsole);
void SetLoggingCategories(const ArgsManager& args);
bool StartLogging(const ArgsManager& args);
void LogPackageVersion();
diff --git a/src/node/context.h b/src/node/context.h
index 06adb33a80e..1bdaaddb6b7 100644
--- a/src/node/context.h
+++ b/src/node/context.h
@@ -39,6 +39,9 @@ class WalletClient;
struct NodeContext {
//! Init interface for initializing current process and connecting to other processes.
interfaces::Init* init{nullptr};
+ //! Bool indicating if this is a GUI process that does not support
+ //! daemonization, and should also disable the RPC server by default.
+ bool is_gui{false};
std::unique_ptr<CAddrMan> addrman;
std::unique_ptr<CConnman> connman;
std::unique_ptr<CTxMemPool> mempool;
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
index 8befbf5e307..8352936c182 100644
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -71,7 +71,7 @@ private:
ChainstateManager& chainman() { return *Assert(m_context->chainman); }
public:
explicit NodeImpl(NodeContext* context) { setContext(context); }
- void initLogging() override { InitLogging(*Assert(m_context->args)); }
+ void initLogging() override { InitLogging(*Assert(m_context->args), m_context->is_gui); }
void initParameterInteraction() override { InitParameterInteraction(*Assert(m_context->args)); }
bilingual_str getWarnings() override { return GetWarnings(true); }
uint32_t getLogCategories() override { return LogInstance().GetCategoryMask(); }
@@ -93,7 +93,7 @@ public:
{
StartShutdown();
// Stop RPC for clean shutdown if any of waitfor* commands is executed.
- if (gArgs.GetBoolArg("-server", false)) {
+ if (gArgs.GetBoolArg("-server", !m_context->is_gui)) {
InterruptRPC();
StopRPC();
}
diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
index de71b7dea7d..33a9c24d281 100644
--- a/src/qt/bitcoin.cpp
+++ b/src/qt/bitcoin.cpp
@@ -311,11 +311,7 @@ void BitcoinApplication::startThread()
void BitcoinApplication::parameterSetup()
{
- // Default printtoconsole to false for the GUI. GUI programs should not
- // print to the console unnecessarily.
- gArgs.SoftSetBoolArg("-printtoconsole", false);
-
- InitLogging(gArgs);
+ InitLogging(gArgs, /* is_gui= */ true);
InitParameterInteraction(gArgs);
}
@@ -464,6 +460,7 @@ int GuiMain(int argc, char* argv[])
util::ThreadSetInternalName("main");
NodeContext node_context;
+ node_context.is_gui = true;
std::unique_ptr<interfaces::Node> node = interfaces::MakeNode(&node_context);
// Subscribe to global signals from core
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index ffc51151458..42793c1abf6 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -101,7 +101,7 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::ve
SelectParams(chainName);
SeedInsecureRand();
if (G_TEST_LOG_FUN) LogInstance().PushBackCallback(G_TEST_LOG_FUN);
- InitLogging(*m_node.args);
+ InitLogging(*m_node.args, m_node.is_gui);
AppInitParameterInteraction(*m_node.args);
LogInstance().StartLogging();
SHA256AutoDetect(); |
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
1 similar comment
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
I won't be able to focus on this stuff in the near future. So closing up for grabs. |
This PR:
-daemon
option forbitcoin-qt
-printtoconsole
option forbitcoin-qt
-server
optionOn master (abdfd2d):
With this PR: