Warn unrecognised sections in the config file #14708

pull AkioNak wants to merge 1 commits into bitcoin:master from AkioNak:warn_unrecognized_section changing 4 files +56 −9
  1. AkioNak commented at 6:19 AM on November 12, 2018: contributor

    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
  2. fanquake added the label Utils/log/libs on Nov 12, 2018
  3. in src/util/system.cpp:419 in 733a8d4e84 outdated 
     414 | +    std::set<std::string> diff;
 415 | +
 416 | +    LOCK(cs_args);
 417 | +    for (const auto& arg: m_config_args) {
 418 | +        if ((pos = arg.first.rfind('.')) != std::string::npos) {
 419 | +            m_config_sections.insert(arg.first.substr(1, pos - 1));
    promag commented at 10:48 AM on November 12, 2018:

    m_config_sections is already populated in GetConfigOptions, why do it here?

    AkioNak commented at 3:26 PM on November 12, 2018:

    @promag In the config file, the structured syntax like foo.bar=18883 is available and should be checked. My first idea was to check all specified options here, but it was not enough to check section names without options in that section.

    It may be better that these are in GetConfigOptions than here.

    AkioNak commented at 9:33 AM on November 13, 2018:

    Moved them to GetConfigOptions.

  4. in src/util/system.cpp:426 in 733a8d4e84 outdated 
     421 | +    }
 422 | +    std::set_difference(
 423 | +        m_config_sections.begin(), m_config_sections.end(),
 424 | +        m_available_sections.begin(), m_available_sections.end(),
 425 | +        std::inserter(diff, diff.end()));
 426 | +    for (const auto& name: diff) {
    promag commented at 11:02 AM on November 12, 2018:

    nit, add space before :. Same throughout.

    AkioNak commented at 9:34 AM on November 13, 2018:

    done.

  5. in src/util/system.h:152 in 733a8d4e84 outdated 
     147 | @@ -148,6 +148,8 @@ class ArgsManager
 148 |      std::string m_network GUARDED_BY(cs_args);
 149 |      std::set<std::string> m_network_only_args GUARDED_BY(cs_args);
 150 |      std::map<OptionsCategory, std::map<std::string, Arg>> m_available_args GUARDED_BY(cs_args);
 151 | +    std::set<std::string> m_config_sections GUARDED_BY(cs_args);
 152 | +    std::set<std::string> m_available_sections GUARDED_BY(cs_args);
    promag commented at 11:03 AM on November 12, 2018:

    This could be a static const variable in system.cpp?

    AkioNak commented at 9:35 AM on November 13, 2018:

    Yes. changed to a static const valiable.

  6. in src/util/system.h:178 in 733a8d4e84 outdated 
     171 | @@ -170,6 +172,11 @@ class ArgsManager
 172 |       */
 173 |      void WarnForSectionOnlyArgs();
 174 |  
 175 | +    /**
 176 | +     * Log warnings for unrecognized section names in the config file.
 177 | +     */
 178 | +    void WarnForUnrecognizedSection();
    promag commented at 11:03 AM on November 12, 2018:

    nit, const.

    AkioNak commented at 9:35 AM on November 13, 2018:

    done.

  7. in src/util/system.cpp:862 in 733a8d4e84 outdated 
     858 | @@ -833,7 +859,9 @@ static bool GetConfigOptions(std::istream& stream, std::string& error, std::vect
 859 |          str = TrimString(str, pattern);
 860 |          if (!str.empty()) {
 861 |              if (*str.begin() == '[' && *str.rbegin() == ']') {
 862 | -                prefix = str.substr(1, str.size() - 2) + '.';
 863 | +                prefix = str.substr(1, str.size() - 2);
    promag commented at 11:09 AM on November 12, 2018:

    nit

    const std::string section = str.substr(1, str.size() - 2);
sections.insert(section);
prefix = section + '.';
    AkioNak commented at 9:35 AM on November 13, 2018:

    done.

  8. promag commented at 11:13 AM on November 12, 2018: member

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

  9. AkioNak commented at 3:28 PM on November 12, 2018: contributor

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

  10. AkioNak force-pushed on Nov 13, 2018
  11. AkioNak commented at 10:01 AM on November 13, 2018: contributor

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

  12. in src/util/system.cpp:844 in 7b8728a4ce outdated 
     840 | @@ -820,7 +841,7 @@ static std::string TrimString(const std::string& str, const std::string& pattern
 841 |      return str.substr(front, end - front + 1);
 842 |  }
 843 |  
 844 | -static bool GetConfigOptions(std::istream& stream, std::string& error, std::vector<std::pair<std::string, std::string>> &options)
 845 | +static bool GetConfigOptions(std::istream& stream, std::string& error, std::vector<std::pair<std::string, std::string>> &options, std::set<std::string> &sections)
    promag commented at 10:08 AM on November 13, 2018:

    nit, space should be after & for new code.

    AkioNak commented at 10:54 PM on November 13, 2018:

    Oh, thanks.

    AkioNak commented at 3:08 AM on November 15, 2018:

    done.

  13. in src/util/system.cpp:360 in 7b8728a4ce outdated 
     355 | @@ -356,6 +356,13 @@ static bool InterpretNegatedOption(std::string& key, std::string& val)
 356 |      return false;
 357 |  }
 358 |  
 359 | +// Section names to be recognized in the config file.
 360 | +const std::set<std::string> ArgsManager::m_available_sections{
    promag commented at 10:11 AM on November 13, 2018:

    IMO this doesn't have to be exposed in the class and currently it's only necessary in this unit. Suggestion: static const std::set<std::string> G_CONFIG_SECTIONS{...} and remove from header.

    AkioNak commented at 10:54 PM on November 13, 2018:

    I agree with your opinion. I will fix it tomorrow.

    AkioNak commented at 3:11 AM on November 15, 2018:

    @promag fixed and move it just before the place to refer it.

  14. promag commented at 10:18 AM on November 13, 2018: member

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

  15. DrahtBot commented at 3:20 PM on November 13, 2018: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  16. in src/util/system.h:172 in 23e3619c92 outdated 
     170 | @@ -170,6 +171,11 @@ class ArgsManager
 171 |       */
 172 |      void WarnForSectionOnlyArgs();
    MarcoFalke commented at 6:49 PM on November 15, 2018:

    For completeness, could make this const as well?

    AkioNak commented at 7:50 AM on November 16, 2018:

    sure.

    AkioNak commented at 11:31 AM on November 16, 2018:

    done.

  17. in src/util/system.cpp:422 in 23e3619c92 outdated 
     417 | +    std::set_difference(
 418 | +        m_config_sections.begin(), m_config_sections.end(),
 419 | +        G_CONFIG_SECTIONS.begin(), G_CONFIG_SECTIONS.end(),
 420 | +        std::inserter(diff, diff.end()));
 421 | +    for (const auto& name : diff) {
 422 | +        LogPrintf("Warning: Section [%s] is not recognized.\n", name);
    MarcoFalke commented at 6:57 PM on November 15, 2018:

    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
    AkioNak commented at 7:50 AM on November 16, 2018:

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

    AkioNak commented at 11:31 AM on November 16, 2018:

    done.

    AkioNak commented at 11:40 AM on November 16, 2018:

    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.

  18. MarcoFalke approved
  19. MarcoFalke commented at 6:58 PM on November 15, 2018: member

    utACK 23e3619c924560f31a01f5cdb31b4bda9df93324 (Could squash commits?)

  20. AkioNak force-pushed on Nov 16, 2018
  21. DrahtBot added the label Needs rebase on Nov 16, 2018
  22. AkioNak force-pushed on Nov 16, 2018
  23. DrahtBot removed the label Needs rebase on Nov 16, 2018
  24. in src/util/system.cpp:403 in f2092bf14f outdated 
     398 | @@ -398,7 +399,30 @@ void ArgsManager::WarnForSectionOnlyArgs()
 399 |          if (!found_result.first) continue;
 400 |  
 401 |          // otherwise, issue a warning
 402 | -        LogPrintf("Warning: Config setting for %s only applied on %s network when in [%s] section.\n", arg, m_network, m_network);
 403 | +        const std::string msg = strprintf("Config setting for %s only applied on %s network when in [%s] section.", arg, m_network, m_network);
 404 | +        uiInterface.ThreadSafeMessageBox(msg, "", CClientUIInterface::MSG_WARNING);
    MarcoFalke commented at 7:26 PM on November 16, 2018:

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

    MarcoFalke commented at 7:27 PM on November 16, 2018:

    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.
    AkioNak commented at 12:05 PM on November 19, 2018:

    Addressed. Please re-review.

  25. AkioNak force-pushed on Nov 19, 2018
  26. MarcoFalke commented at 2:53 PM on November 19, 2018: member

    utACK ecfc35ce9bfb297f01344f565bfa771bf3c6ac72. Thanks for fixing up all the feedback!

  27. MarcoFalke commented at 9:22 PM on November 19, 2018: member

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

  28. in src/util/system.cpp:413 in ecfc35ce9b outdated 
     409 | +// Section names to be recognized in the config file.
 410 | +static const std::set<std::string> G_CONFIG_SECTIONS{
 411 | +    CBaseChainParams::REGTEST,
 412 | +    CBaseChainParams::TESTNET,
 413 | +    CBaseChainParams::MAIN
 414 | +};
    MarcoFalke commented at 9:30 PM on November 19, 2018:

    nit: Could reduce scope even further and move it into the function below? (Keeping the static const ...)

    AkioNak commented at 12:25 AM on November 20, 2018:

    @MarcoFalke Thanks. I could not find out why appveyor failed.

    AkioNak commented at 3:23 PM on November 20, 2018:

    Fixed.

  29. MarcoFalke commented at 9:33 PM on November 19, 2018: member

    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.

  30. AkioNak force-pushed on Nov 20, 2018
  31. in test/functional/feature_config_args.py:39 in a04e3bd816 outdated 
      32 | @@ -33,6 +33,11 @@ def test_config_file_parser(self):
  33 |              conf.write('server=1\nrpcuser=someuser\nrpcpassword=some#pass')
  34 |          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')
  35 |  
  36 | +        with open(inc_conf_file_path, 'w', encoding='utf-8') as conf:
  37 | +            conf.write('testnot.datadir=1\n[testnet]\n')
  38 | +        self.restart_node(0)
  39 | +        self.nodes[0].stop_node(expected_stderr='Warning: Section [testnet] is not recognized.\nWarning: Section [testnot] is not recognized.')
    AkioNak commented at 6:36 AM on November 20, 2018:

    Self review: os.linesep is better way to represent multiple lines in expected_stderr.

    AkioNak commented at 3:23 PM on November 20, 2018:

    Fixed.

  32. AkioNak force-pushed on Nov 20, 2018
  33. 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.
    3fb09b9889
  34. AkioNak force-pushed on Nov 20, 2018
  35. MarcoFalke commented at 9:09 PM on November 20, 2018: member

    re-utACK 3fb09b9889665a24b34f25e9d1385a05058a28b7

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

  36. MarcoFalke added this to the milestone 0.18.0 on Nov 20, 2018
  37. laanwj merged this on Nov 21, 2018
  38. laanwj closed this on Nov 21, 2018
  39. laanwj referenced this in commit d7b0258ff0 on Nov 21, 2018
  40. promag commented at 6:46 PM on November 21, 2018: member

    utACK 3fb09b9.

  41. AkioNak deleted the branch on Dec 6, 2018
  42. MarcoFalke referenced this in commit f8456256c8 on Dec 6, 2018
  43. MarcoFalke referenced this in commit 789b0bbf2a on Mar 2, 2019
  44. deadalnix referenced this in commit fbf455c1fe on Apr 17, 2020
  45. ftrader referenced this in commit 02f9523f9a on Aug 17, 2020
  46. Munkybooty referenced this in commit d96d008196 on Jul 29, 2021
  47. Munkybooty referenced this in commit 8562d1f869 on Jul 29, 2021
  48. linuxsh2 referenced this in commit e3f0da1972 on Jul 29, 2021
  49. linuxsh2 referenced this in commit 3a1cf20f89 on Jul 30, 2021
  50. Munkybooty referenced this in commit 8fc5406d4d on Aug 2, 2021
  51. linuxsh2 referenced this in commit d4442aca01 on Aug 3, 2021
  52. Munkybooty referenced this in commit 7ed965e9f9 on Aug 3, 2021
  53. Munkybooty referenced this in commit 849d08cc44 on Aug 5, 2021
  54. Munkybooty referenced this in commit 669093adb2 on Aug 5, 2021
  55. christiancfifi referenced this in commit 0acb05be79 on Aug 27, 2021
  56. christiancfifi referenced this in commit f9d44e9d02 on Aug 27, 2021
  57. christiancfifi referenced this in commit 9c6d9d50bb on Aug 28, 2021
  58. christiancfifi referenced this in commit 07edc88f41 on Aug 29, 2021
  59. christiancfifi referenced this in commit 181bd293fb on Aug 29, 2021
  60. christiancfifi referenced this in commit 53ed42bf91 on Aug 29, 2021
  61. Munkybooty referenced this in commit ef6a5e8545 on Sep 8, 2021
  62. MarcoFalke locked this on Sep 8, 2021
Contributors
AkioNakpromagDrahtBotMarcoFalke
Labels
Utils/log/libs

Milestone
0.18.0

Linked (view graph)
#14702 Warn if unrecognised sections are present in bitcoin.conf#14783 gui: Fix boost::signals2::no_slots_error in early calls to InitWarning#15335 Fix lack of warning of unrecognized section names
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me