diff --git a/src/ripple/app/main/Main.cpp b/src/ripple/app/main/Main.cpp index 3548677593..dbbb0b6ddb 100644 --- a/src/ripple/app/main/Main.cpp +++ b/src/ripple/app/main/Main.cpp @@ -670,6 +670,17 @@ int run (int argc, char** argv) // No arguments. Run server. if (!vm.count ("parameters")) { + // TODO: this comment can be removed in a future release - + // say 1.7 or higher + if (config->had_trailing_comments()) + { + JLOG(logs->journal("Application").warn()) << + "Trailing comments were seen in your config file. " << + "The treatment of inline/trailing comments has changed recently. " << + "Any `#` characters NOT intended to delimit comments should be " << + "preceded by a \\"; + } + // We want at least 1024 file descriptors. We'll // tweak this further. if (!adjustDescriptorLimit(1024, logs->journal("Application"))) diff --git a/src/ripple/basics/BasicConfig.h b/src/ripple/basics/BasicConfig.h index bfe75e6508..f68bdccb45 100644 --- a/src/ripple/basics/BasicConfig.h +++ b/src/ripple/basics/BasicConfig.h @@ -25,6 +25,7 @@ #include #include #include +#include #include #include #include @@ -47,6 +48,7 @@ private: std::string name_; std::vector lines_; std::vector values_; + bool had_trailing_comments_ = false; public: /** Create an empty section. */ @@ -153,12 +155,14 @@ public: T value_or(std::string const& name, T const& other) const { - auto const iter = cont().find(name); - if (iter == cont().end()) - return other; - return boost::lexical_cast(iter->second); + auto const v = get(name); + return v.is_initialized() ? *v : other; } + // indicates if trailing comments were seen + // during the appending of any lines/values + bool had_trailing_comments() const { return had_trailing_comments_; } + friend std::ostream& operator<< (std::ostream&, Section const& section); @@ -243,6 +247,13 @@ public: std::ostream& operator<< (std::ostream& ss, BasicConfig const& c); + // indicates if trailing comments were seen + // in any loaded Sections + bool had_trailing_comments() const { + return std::any_of(map_.cbegin(), map_.cend(), + [](auto s){ return s.second.had_trailing_comments(); }); + } + protected: void build (IniFileSections const& ifs); @@ -251,50 +262,41 @@ protected: //------------------------------------------------------------------------------ /** Set a value from a configuration Section - If the named value is not found, the variable is unchanged. + If the named value is not found or doesn't parse as a T, + the variable is unchanged. @return `true` if value was set. */ template bool set (T& target, std::string const& name, Section const& section) { - auto const [val, found] = section.find (name); - if (! found) - return false; + bool found_and_valid = false; try { - target = boost::lexical_cast (val); - return true; + auto const val = section.get (name); + if ((found_and_valid = val.is_initialized())) + target = *val; } catch (boost::bad_lexical_cast&) { } - return false; + return found_and_valid; } /** Set a value from a configuration Section - If the named value is not found, the variable is assigned the default. - @return `true` if named value was found in the Section. + If the named value is not found or doesn't cast to T, + the variable is assigned the default. + @return `true` if the named value was found and is valid. */ template bool set (T& target, T const& defaultValue, std::string const& name, Section const& section) { - auto const [val, found] = section.find (name); - if (! found) - return false; - try - { - // VFALCO TODO Use try_lexical_convert (boost 1.56.0) - target = boost::lexical_cast (val); - return true; - } - catch (boost::bad_lexical_cast&) - { + bool found_and_valid = set(target, name, section); + if (!found_and_valid) target = defaultValue; - } - return false; + return found_and_valid; } /** Retrieve a key/value pair from a section. @@ -307,12 +309,9 @@ T get (Section const& section, std::string const& name, T const& defaultValue = T{}) { - auto const [val, found] = section.find (name); - if (!found) - return defaultValue; try { - return boost::lexical_cast (val); + return section.value_or (name, defaultValue); } catch (boost::bad_lexical_cast&) { @@ -325,14 +324,14 @@ std::string get (Section const& section, std::string const& name, const char* defaultValue) { - auto const [val, found] = section.find (name); - if (!found) - return defaultValue; + bool found_and_valid = false; try { - return boost::lexical_cast (val); + auto const val = section.get (name); + if ((found_and_valid = val.is_initialized())) + return *val; } - catch(std::exception const&) + catch (boost::bad_lexical_cast&) { } return defaultValue; @@ -343,18 +342,7 @@ bool get_if_exists (Section const& section, std::string const& name, T& v) { - auto const [val, found] = section.find (name); - if (!found) - return false; - try - { - v = boost::lexical_cast (val); - return true; - } - catch (boost::bad_lexical_cast&) - { - } - return false; + return set(v, name, section); } template <> @@ -364,12 +352,10 @@ get_if_exists (Section const& section, std::string const& name, bool& v) { int intVal = 0; - if (get_if_exists (section, name, intVal)) - { + auto stat = get_if_exists(section, name, intVal); + if (stat) v = bool (intVal); - return true; - } - return false; + return stat; } } // ripple diff --git a/src/ripple/basics/impl/BasicConfig.cpp b/src/ripple/basics/impl/BasicConfig.cpp index ac0772443d..fa66568c9f 100644 --- a/src/ripple/basics/impl/BasicConfig.cpp +++ b/src/ripple/basics/impl/BasicConfig.cpp @@ -18,6 +18,7 @@ //============================================================================== #include +#include #include #include @@ -53,14 +54,54 @@ Section::append (std::vector const& lines) ); lines_.reserve (lines_.size() + lines.size()); - for (auto const& line : lines) + for (auto line : lines) { + auto remove_comment = [](std::string& val)->bool + { + bool removed_trailing = false; + auto comment = val.find('#'); + while(comment != std::string::npos) + { + if (comment == 0) + { + // entire value is a comment. In most cases, this + // would have already been handled by the file reader + val = ""; + break; + } + else if (val.at(comment-1) == '\\') + { + // we have an escaped comment char. Erase the escape char + // and keep looking + val.erase(comment-1,1); + } + else + { + // this must be a real comment. Extract the value + // as a substring and stop looking. + val = trim_whitespace(val.substr(0, comment)); + removed_trailing = true; + break; + } + + comment = val.find('#', comment); + } + return removed_trailing; + }; + + if (remove_comment(line) && !line.empty()) + had_trailing_comments_ = true; + + if (line.empty()) + continue; + boost::smatch match; - lines_.push_back (line); if (boost::regex_match (line, match, re1)) set (match[1], match[2]); else values_.push_back (line); + + lines_.push_back (std::move(line)); } } diff --git a/src/ripple/core/impl/Config.cpp b/src/ripple/core/impl/Config.cpp index 7418181807..28c8bd4731 100644 --- a/src/ripple/core/impl/Config.cpp +++ b/src/ripple/core/impl/Config.cpp @@ -149,7 +149,7 @@ getIniFileSection (IniFileSections& secSource, std::string const& strSection) IniFileSections::mapped_type* smtResult; it = secSource.find (strSection); if (it == secSource.end ()) - smtResult = 0; + smtResult = nullptr; else smtResult = & (it->second); return smtResult; diff --git a/src/test/core/Config_test.cpp b/src/test/core/Config_test.cpp index cc35102a09..03e72ea2fc 100644 --- a/src/test/core/Config_test.cpp +++ b/src/test/core/Config_test.cpp @@ -786,6 +786,159 @@ r.ripple.com 51235 } + void testComments() + { + struct TestCommentData + { + std::string_view line; + std::string_view field; + std::string_view expect; + bool had_comment; + }; + + std::array< TestCommentData, 13> tests = {{ + { "password = aaaa\\#bbbb", "password", "aaaa#bbbb", false}, + { "password = aaaa#bbbb", "password", "aaaa", true}, + { "password = aaaa #bbbb", "password", "aaaa", true}, + // since the value is all comment, this doesn't parse as k=v : + { "password = #aaaa #bbbb", "", "password =", true}, + { "password = aaaa\\# #bbbb", "password", "aaaa#", true}, + { "password = aaaa\\##bbbb", "password", "aaaa#", true}, + { "aaaa#bbbb", "", "aaaa", true}, + { "aaaa\\#bbbb", "", "aaaa#bbbb", false}, + { "aaaa\\##bbbb", "", "aaaa#", true}, + { "aaaa #bbbb", "", "aaaa", true}, + { "1 #comment", "", "1", true}, + { "#whole thing is comment", "", "", false}, + { " #whole comment with space", "", "", false} + }}; + + for (auto const& t : tests) + { + Section s; + s.append(t.line.data()); + BEAST_EXPECT(s.had_trailing_comments() == t.had_comment); + if (t.field.empty()) + { + BEAST_EXPECTS(s.legacy() == t.expect, s.legacy()); + } + else + { + std::string field; + BEAST_EXPECTS(set(field, t.field.data(), s), t.line); + BEAST_EXPECTS(field == t.expect, t.line); + } + } + + { + Section s; + s.append("online_delete = 3000"); + std::uint32_t od = 0; + BEAST_EXPECT(set(od, "online_delete", s)); + BEAST_EXPECTS(od == 3000, *(s.get("online_delete"))); + } + + { + Section s; + s.append("online_delete = 2000 #my comment on this"); + std::uint32_t od = 0; + BEAST_EXPECT(set(od, "online_delete", s)); + BEAST_EXPECTS(od == 2000, *(s.get("online_delete"))); + } + } + + void testGetters() + { + using namespace std::string_literals; + Section s {"MySection"}; + s.append("a_string = mystring"); + s.append("positive_int = 2"); + s.append("negative_int = -3"); + s.append("bool_ish = 1"); + + { + auto val_1 = "value 1"s; + BEAST_EXPECT(set(val_1, "a_string", s)); + BEAST_EXPECT(val_1 == "mystring"); + + auto val_2 = "value 2"s; + BEAST_EXPECT(!set(val_2, "not_a_key", s)); + BEAST_EXPECT(val_2 == "value 2"); + BEAST_EXPECT(!set(val_2, "default"s, "not_a_key", s)); + BEAST_EXPECT(val_2 == "default"); + + auto val_3 = get(s, "a_string"); + BEAST_EXPECT(val_3 == "mystring"); + auto val_4 = get(s, "not_a_key"); + BEAST_EXPECT(val_4 == ""); + auto val_5 = get(s, "not_a_key", "default"); + BEAST_EXPECT(val_5 == "default"); + + auto val_6 = "value 6"s; + BEAST_EXPECT(get_if_exists(s, "a_string", val_6)); + BEAST_EXPECT(val_6 == "mystring"); + + auto val_7 = "value 7"s; + BEAST_EXPECT(!get_if_exists(s, "not_a_key", val_7)); + BEAST_EXPECT(val_7 == "value 7"); + } + + { + int val_1 = 1; + BEAST_EXPECT(set(val_1, "positive_int", s)); + BEAST_EXPECT(val_1 == 2); + + int val_2 = 2; + BEAST_EXPECT(set(val_2, "negative_int", s)); + BEAST_EXPECT(val_2 == -3); + + int val_3 = 3; + BEAST_EXPECT(!set(val_3, "a_string", s)); + BEAST_EXPECT(val_3 == 3); + + auto val_4 = get(s, "positive_int"); + BEAST_EXPECT(val_4 == 2); + auto val_5 = get(s, "not_a_key"); + BEAST_EXPECT(val_5 == 0); + auto val_6 = get(s, "not_a_key", 5); + BEAST_EXPECT(val_6 == 5); + auto val_7 = get(s, "a_string", 6); + BEAST_EXPECT(val_7 == 6); + + int val_8 = 8; + BEAST_EXPECT(get_if_exists(s, "positive_int", val_8)); + BEAST_EXPECT(val_8 == 2); + + auto val_9 = 9; + BEAST_EXPECT(!get_if_exists(s, "not_a_key", val_9)); + BEAST_EXPECT(val_9 == 9); + + auto val_10 = 10; + BEAST_EXPECT(!get_if_exists(s, "a_string", val_10)); + BEAST_EXPECT(val_10 == 10); + + BEAST_EXPECT(s.get("not_a_key") == boost::none); + try + { + s.get("a_string"); + fail(); + } + catch(boost::bad_lexical_cast&) + { + pass(); + } + } + + { + bool flag_1 = false; + BEAST_EXPECT(get_if_exists(s, "bool_ish", flag_1)); + BEAST_EXPECT(flag_1 == true); + + bool flag_2 = false; + BEAST_EXPECT(!get_if_exists(s, "not_a_key", flag_2)); + BEAST_EXPECT(flag_2 == false); + } + } void run () override { @@ -797,6 +950,8 @@ r.ripple.com 51235 testSetup (true); testPort (); testWhitespace (); + testComments (); + testGetters (); } };