diff --git a/db/config.cc b/db/config.cc index 912e2af2d4..01fdafe07e 100644 --- a/db/config.cc +++ b/db/config.cc @@ -112,6 +112,17 @@ void config_file::named_value struct log_legacy_value : public named_value { diff --git a/main.cc b/main.cc index a3eb9fcf65..148584a814 100644 --- a/main.cc +++ b/main.cc @@ -101,7 +101,13 @@ read_config(bpo::variables_map& opts, db::config& cfg) { file = relative_conf_dir("scylla.yaml").string(); } return check_direct_io_support(file).then([file, &cfg] { - return cfg.read_from_file(file); + return cfg.read_from_file(file, [](auto & opt, auto & msg, auto status) { + auto level = log_level::warn; + if (status.value_or(db::config::value_status::Invalid) != db::config::value_status::Invalid) { + level = log_level::error; + } + startlog.log(level, "{} : {}", msg, opt); + }); }).handle_exception([file](auto ep) { startlog.error("Could not read configuration file {}: {}", file, ep); return make_exception_future<>(ep); diff --git a/tests/config_test.cc b/tests/config_test.cc index 2935a10383..5d071587f8 100644 --- a/tests/config_test.cc +++ b/tests/config_test.cc @@ -826,7 +826,11 @@ inline std::basic_ostream & operator<<(std::basic_ostream & os SEASTAR_TEST_CASE(test_parse_yaml) { config cfg; - cfg.read_from_yaml(cassandra_conf); + cfg.read_from_yaml(cassandra_conf, [](auto& opt, auto& msg, auto status) { + if (status != config::value_status::Invalid) { + throw std::invalid_argument(msg + " : " + opt); + } + }); BOOST_CHECK_EQUAL(cfg.cluster_name(), "Test Cluster"); BOOST_CHECK_EQUAL(cfg.cluster_name.is_set(), true); @@ -861,4 +865,34 @@ SEASTAR_TEST_CASE(test_parse_yaml) { return make_ready_future<>(); } +SEASTAR_TEST_CASE(test_parse_broken) { + config cfg; + bool ok = false; + + // this should become an "unknown option" kind of error. + cfg.read_from_yaml(R"foo(bork_bnork: + apa + ko +)foo", [&](auto& opt, auto& msg, auto status) { + if (!status) { // unknown + ok = true; + } + }); + + BOOST_REQUIRE(ok); + + ok = false; + + // this should be a value parsing error. + cfg.read_from_yaml(R"foo(commitlog_segment_size_in_mb: flaska +)foo", [&](auto& opt, auto& msg, auto status) { + if (status && status != config::value_status::Invalid) { // option exists and is valid. + ok = true; + } + }); + + BOOST_REQUIRE(ok); + + return make_ready_future<>(); +} diff --git a/utils/config_file.cc b/utils/config_file.cc index 994833ed99..e61533f39d 100644 --- a/utils/config_file.cc +++ b/utils/config_file.cc @@ -241,13 +241,18 @@ utils::config_file::add_options(bpo::options_description_easy_init& init) { return init; } -void utils::config_file::read_from_yaml(const sstring& yaml) { - read_from_yaml(yaml.c_str()); +void utils::config_file::read_from_yaml(const sstring& yaml, error_handler h) { + read_from_yaml(yaml.c_str(), std::move(h)); } -void utils::config_file::read_from_yaml(const char* yaml) { +void utils::config_file::read_from_yaml(const char* yaml, error_handler h) { std::unordered_map values; + if (!h) { + h = [](auto & opt, auto & msg, auto) { + throw std::invalid_argument(msg + " : " + opt); + }; + } /* * Note: this is not very "half-fault" tolerant. I.e. there could be * yaml syntax errors that origin handles and still sets the options @@ -261,7 +266,8 @@ void utils::config_file::read_from_yaml(const char* yaml) { auto i = std::find_if(_cfgs.begin(), _cfgs.end(), [&label](const config_src& cfg) { return cfg.name() == label; }); if (i == _cfgs.end()) { - throw std::invalid_argument("Unknown option " + label); + h(label, "Unknown option", stdx::nullopt); + continue; } config_src& cfg = *i; @@ -272,7 +278,8 @@ void utils::config_file::read_from_yaml(const char* yaml) { } switch (cfg.status()) { case value_status::Invalid: - throw std::invalid_argument("Option " + label + " is not applicable"); + h(label, "Option is not applicable", cfg.status()); + continue; case value_status::Unused: default: break; @@ -281,7 +288,13 @@ void utils::config_file::read_from_yaml(const char* yaml) { continue; } // Still, a syntax error is an error warning, not a fail - cfg.set_value(node.second); + try { + cfg.set_value(node.second); + } catch (std::exception& e) { + h(label, e.what(), cfg.status()); + } catch (...) { + h(label, "Could not convert value", cfg.status()); + } } } @@ -305,19 +318,19 @@ utils::config_file::configs utils::config_file::unset_values() const { return res; } -future<> utils::config_file::read_from_file(file f) { - return f.size().then([this, f](size_t s) { - return do_with(make_file_input_stream(f), [this, s](input_stream& in) { - return in.read_exactly(s).then([this](temporary_buffer buf) { - read_from_yaml(sstring(buf.begin(), buf.end())); +future<> utils::config_file::read_from_file(file f, error_handler h) { + return f.size().then([this, f, h](size_t s) { + return do_with(make_file_input_stream(f), [this, s, h](input_stream& in) { + return in.read_exactly(s).then([this, h](temporary_buffer buf) { + read_from_yaml(sstring(buf.begin(), buf.end()), h); }); }); }); } -future<> utils::config_file::read_from_file(const sstring& filename) { - return open_file_dma(filename, open_flags::ro).then([this](file f) { - return read_from_file(std::move(f)); +future<> utils::config_file::read_from_file(const sstring& filename, error_handler h) { + return open_file_dma(filename, open_flags::ro).then([this, h](file f) { + return read_from_file(std::move(f), h); }); } diff --git a/utils/config_file.hh b/utils/config_file.hh index ec4f82fc7e..15e7ce309a 100644 --- a/utils/config_file.hh +++ b/utils/config_file.hh @@ -143,10 +143,25 @@ public: boost::program_options::options_description_easy_init& add_options(boost::program_options::options_description_easy_init&); - void read_from_yaml(const sstring&); - void read_from_yaml(const char *); - future<> read_from_file(const sstring&); - future<> read_from_file(file); + /** + * Default behaviour for yaml parser is to throw on + * unknown stuff, invalid opts or conversion errors. + * + * Error handling function allows overriding this. + * + * error: