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

Warn unrecognised sections in the config file #14708

Merged
merged 1 commit into from Nov 21, 2018

Conversation

Projects
None yet
6 participants
@AkioNak
Copy link
Contributor

AkioNak commented Nov 12, 2018

This PR intends to resolve #14702.

In the config file, sections are specified by square bracket pair "[]"$,
or included in the option name itself which separated by a period"(.)".

Typicaly, [testnet] is not a correct section name and specified options
in that section are ignored but user cannot recognize what is happen.

So, add some log-warning messages if unrecognized section names are
present in the config file after checking section only args.

note: Currentry, followings are out of scope of this PR.

  1. Empty section name or option name can describe.
    e.g. [] , .a=b, =c
  2. Multiple period characters can exist in the section name and option name.
    e.g. [c.d.e], [..], f.g.h.i=j, ..=k
@promag
Copy link
Member

promag left a comment

Tested ACK but I'd like to get feedback on my comments, looks like it has unnecessary changes.

Show resolved Hide resolved src/util/system.cpp Outdated
Show resolved Hide resolved src/util/system.cpp Outdated
Show resolved Hide resolved src/util/system.h Outdated
Show resolved Hide resolved src/util/system.h Outdated
Show resolved Hide resolved src/util/system.cpp Outdated
@AkioNak

This comment has been minimized.

Copy link
Contributor

AkioNak commented Nov 12, 2018

@promag Thank you for your reviewing. I will address what you pointed out soon.

@AkioNak AkioNak force-pushed the AkioNak:warn_unrecognized_section branch from f14ddd2 to 7b8728a Nov 13, 2018

@AkioNak

This comment has been minimized.

Copy link
Contributor

AkioNak commented Nov 13, 2018

@promag I've fixed what you pointed out. Please re-review :-)

@promag
Copy link
Member

promag left a comment

Thanks, looks good to me, just adding 2 small comments for your consideration.

Show resolved Hide resolved src/util/system.cpp Outdated
Show resolved Hide resolved src/util/system.cpp Outdated
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Nov 13, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14045 (WIP: refactor: Fix the chainparamsbase -> util circular dependency by Empact)

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.

@MarcoFalke
Copy link
Member

MarcoFalke left a comment

utACK 23e3619 (Could squash commits?)

@@ -170,6 +171,11 @@ class ArgsManager
*/
void WarnForSectionOnlyArgs();

This comment has been minimized.

@MarcoFalke

MarcoFalke Nov 15, 2018

Member

For completeness, could make this const as well?

This comment has been minimized.

@AkioNak

AkioNak Nov 16, 2018

Contributor

sure.

This comment has been minimized.

@AkioNak

AkioNak Nov 16, 2018

Contributor

done.

G_CONFIG_SECTIONS.begin(), G_CONFIG_SECTIONS.end(),
std::inserter(diff, diff.end()));
for (const auto& name : diff) {
LogPrintf("Warning: Section [%s] is not recognized.\n", name);

This comment has been minimized.

@MarcoFalke

MarcoFalke Nov 15, 2018

Member

Hmm. This only ends up in the debug log. However, I think that warnings could as well be printed out. (See how InitError() does it.)

Not sure if that suggestion makes sense, but if applied you could probably also write a test case like this:

diff --git a/test/functional/feature_config_args.py b/test/functional/feature_config_args.py
index 88a9aadc7b..55ef46d253 100755
--- a/test/functional/feature_config_args.py
+++ b/test/functional/feature_config_args.py
@@ -33,6 +33,11 @@ class ConfArgsTest(BitcoinTestFramework):
             conf.write('server=1\nrpcuser=someuser\nrpcpassword=some#pass')
         self.nodes[0].assert_start_raises_init_error(expected_msg='Error reading configuration file: parse error on line 3, using # in rpcpassword can be ambiguous and should be avoided')
 
+        with open(inc_conf_file_path, 'w', encoding='utf-8') as conf:
+            conf.write('testneettt.datadir=1')
+        self.restart_node(0)
+        self.nodes[0].stop_node(expected_stderr='Warning: Section [testneettt] is not recognized.')
+
         with open(inc_conf_file_path, 'w', encoding='utf-8') as conf:
             conf.write('')  # clear
 

This comment has been minimized.

@AkioNak

AkioNak Nov 16, 2018

Contributor

@MarcoFalke Thanks for your suggestion. I think it will be more useful. I'll try it.

This comment has been minimized.

@AkioNak

AkioNak Nov 16, 2018

Contributor

done.

This comment has been minimized.

@AkioNak

AkioNak Nov 16, 2018

Contributor

I could not write a test case for WarnForSectionOnlyArgs().
Because I could not found simple way that how to produce the situation that WarnForSectionOnlyArgs() warn and to continue to run through the test.

@AkioNak AkioNak force-pushed the AkioNak:warn_unrecognized_section branch from 23e3619 to 7009611 Nov 16, 2018

@AkioNak AkioNak force-pushed the AkioNak:warn_unrecognized_section branch from 7009611 to f2092bf Nov 16, 2018

@DrahtBot DrahtBot removed the Needs rebase label Nov 16, 2018

@@ -398,7 +399,30 @@ void ArgsManager::WarnForSectionOnlyArgs()
if (!found_result.first) continue;

// otherwise, issue a warning
LogPrintf("Warning: Config setting for %s only applied on %s network when in [%s] section.\n", arg, m_network, m_network);
const std::string msg = strprintf("Config setting for %s only applied on %s network when in [%s] section.", arg, m_network, m_network);
uiInterface.ThreadSafeMessageBox(msg, "", CClientUIInterface::MSG_WARNING);

This comment has been minimized.

@MarcoFalke

MarcoFalke Nov 16, 2018

Member

Putting this here smells a bit. You could either move ui_interface to the libbitcoin_util or pass the warnings up to the caller?

This comment has been minimized.

@MarcoFalke

MarcoFalke Nov 16, 2018

Member

Probably want to pass it up as per travis:

A new circular dependency in the form of "ui_interface -> util/system -> ui_interface" appears to have been introduced.

This comment has been minimized.

@AkioNak

AkioNak Nov 19, 2018

Contributor

Addressed. Please re-review.

@AkioNak AkioNak force-pushed the AkioNak:warn_unrecognized_section branch from f2092bf to ecfc35c Nov 19, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 19, 2018

utACK ecfc35c. Thanks for fixing up all the feedback!

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 19, 2018

Appveyor fails with AssertionError: Unexpected stderr Warning: Section [regtest] is not recognized. != .

@MarcoFalke
Copy link
Member

MarcoFalke left a comment

I haven't looked at the appveyor failure but it could be caused by static initialization issues based on ordering (a global is initialized on another globals value, but before the other global was initialized). So maybe G_CONFIG_SECTIONS is just the set with the empty string in it on appveyor?

Could at least rule that out by trying the refactoring I suggested in this review.

Show resolved Hide resolved src/util/system.cpp Outdated

@AkioNak AkioNak force-pushed the AkioNak:warn_unrecognized_section branch from ecfc35c to a04e3bd Nov 20, 2018

@AkioNak AkioNak force-pushed the AkioNak:warn_unrecognized_section branch from a04e3bd to f38903c Nov 20, 2018

Warn unrecognized sections in the config file
In the config file, sections are specified by square bracket pair "[]"$,
or included in the option name itself which separated by a period"(.)".

Typicaly, [testnet] is not a correct section name and specified options
in that section are ignored but user cannot recognize what is happen.

So, add some log/stderr-warning messages if unrecognized section names
are present in the config file after checking section only args.

@AkioNak AkioNak force-pushed the AkioNak:warn_unrecognized_section branch from f38903c to 3fb09b9 Nov 20, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 20, 2018

re-utACK 3fb09b9

Only change was the suggested refactoring to fix the appveyor build.

@MarcoFalke MarcoFalke added this to the 0.18.0 milestone Nov 20, 2018

@laanwj laanwj merged commit 3fb09b9 into bitcoin:master Nov 21, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Nov 21, 2018

Merge #14708: Warn unrecognised sections in the config file
3fb09b9 Warn unrecognized sections in the config file (Akio Nakamura)

Pull request description:

  This PR intends to resolve #14702.

  In the config file, sections are specified by square bracket pair "[]"$,
  or included in the option name itself which separated by a period"(.)".

  Typicaly, [testnet] is not a correct section name and specified options
  in that section are ignored but user cannot recognize what is happen.

  So, add some log-warning messages if unrecognized section names are
  present in the config file after checking section only args.

  note: Currentry, followings are out of scope of this PR.
  1) Empty section name or option name can describe.
  e.g. [] , .a=b, =c
  2) Multiple period characters can exist in the section name and option name.
  e.g. [c.d.e], [..], f.g.h.i=j, ..=k

Tree-SHA512: 2cea02a0525feb40320613989a75cd7b7b1bd12158d5e6f3174ca77e6a25bb84425dd8812f62483df9fc482045c7b5402d69bc714430518b1847d055a2dc304b
@promag

This comment has been minimized.

Copy link
Member

promag commented Nov 21, 2018

utACK 3fb09b9.

@AkioNak AkioNak deleted the AkioNak:warn_unrecognized_section branch Dec 6, 2018

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Dec 6, 2018

Merge bitcoin#14783: gui: Fix boost::signals2::no_slots_error in earl…
…y calls to InitWarning

6bbdb20 squashme: connect thru node interface (João Barbosa)
a0f8df3 qt: Call noui_connect to prevent boost::signals2::no_slots_error in early calls to InitWarning (João Barbosa)

Pull request description:

  Adding the following to `bitcoin.conf`
  ```
  [xxx]
  disablewallet=1
  ```
  And running `bitcoin-qt` gives:
  ```
  libc++abi.dylib: terminating with uncaught exception of type boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<boost::signals2::no_slots_error> >: boost::signals2::no_slots_error
  ```

  Fixes regression in bitcoin#14708.

Tree-SHA512: 7c158376fad6ebcd80fc0dbe549d5b6e893fb82e7dc1e455825633d7f91b14dc34493487cab7642152e88f9eaf99bfa91988972d600e9fb289cf26afd64aff8a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment