diff --git a/llarp/config/config.cpp b/llarp/config/config.cpp index aa02da5c2..67d92fa2f 100644 --- a/llarp/config/config.cpp +++ b/llarp/config/config.cpp @@ -1179,6 +1179,7 @@ namespace llarp "lokid", "rpc", RelayOnly, + Required, Comment{ "oxenmq control address for for communicating with oxend. Depends on oxend's", "lmq-local-control configuration option. By default this value should be", diff --git a/llarp/config/definition.cpp b/llarp/config/definition.cpp index aa199ebea..76177603c 100644 --- a/llarp/config/definition.cpp +++ b/llarp/config/definition.cpp @@ -152,10 +152,8 @@ namespace llarp const std::string& section, std::vector comments) { auto& sectionComments = m_sectionComments[section]; - for (size_t i = 0; i < comments.size(); ++i) - { - sectionComments.emplace_back(std::move(comments[i])); - } + for (auto& c : comments) + sectionComments.emplace_back(std::move(c)); } void @@ -198,13 +196,22 @@ namespace llarp if (useValues and def->getNumberFound() > 0) { for (const auto& val : def->valuesAsString()) - fmt::format_to(sect_append, "\n{}={}\n", name, val); + fmt::format_to(sect_append, "\n{}={}", name, val); + *sect_append = '\n'; } - else if (not(def->hidden and not has_comment)) + else if (not def->hidden) { - for (const auto& val : def->defaultValuesAsString()) - fmt::format_to(sect_append, "\n{}{}={}\n", def->required ? "" : "#", name, val); + if (auto defaults = def->defaultValuesAsString(); not defaults.empty()) + for (const auto& val : defaults) + fmt::format_to(sect_append, "\n{}{}={}", def->required ? "" : "#", name, val); + else + // We have no defaults so we append it as "#opt-name=" so that we show the option name, + // and make it simple to uncomment and edit to the desired value. + fmt::format_to(sect_append, "\n#{}=", name); + *sect_append = '\n'; } + else if (has_comment) + *sect_append = '\n'; }); if (sect_str.empty()) diff --git a/llarp/config/definition.hpp b/llarp/config/definition.hpp index 8f9225c28..6cacd3180 100644 --- a/llarp/config/definition.hpp +++ b/llarp/config/definition.hpp @@ -202,6 +202,16 @@ namespace llarp OptionDefinition(std::string section_, std::string name_, Options&&... opts) : OptionDefinitionBase(section_, name_, opts...) { + constexpr bool has_default = + ((config::is_default_array || config::is_default) || ...); + constexpr bool has_required = + (std::is_same_v, config::Required_t> || ...); + constexpr bool has_hidden = + (std::is_same_v, config::Hidden_t> || ...); + static_assert( + not(has_default and has_required), "Default{...} and Required are mutually exclusive"); + static_assert(not(has_hidden and has_required), "Hidden and Required are mutually exclusive"); + (extractDefault(std::forward(opts)), ...); (extractAcceptor(std::forward(opts)), ...); (extractComments(std::forward(opts)), ...); diff --git a/test/config/test_llarp_config_definition.cpp b/test/config/test_llarp_config_definition.cpp index e3edec29c..80005d58c 100644 --- a/test/config/test_llarp_config_definition.cpp +++ b/test/config/test_llarp_config_definition.cpp @@ -104,7 +104,7 @@ TEST_CASE("OptionDefinition acceptor throws test", "[config]") TEST_CASE("OptionDefinition tryAccept missing option test", "[config]") { int unset = -1; - llarp::OptionDefinition def("foo", "bar", Required, Default{1}, [&](int arg) { + llarp::OptionDefinition def("foo", "bar", Required, [&](int arg) { (void)arg; unset = 0; // should never be called }); @@ -170,7 +170,6 @@ TEST_CASE("ConfigDefinition required test", "[config]") config.defineOption(std::make_unique>( "router", "threads", - Default{1}, Required)); CHECK_THROWS(config.validateRequiredFields()); @@ -186,13 +185,13 @@ TEST_CASE("ConfigDefinition section test", "[config]") config.defineOption(std::make_unique>( "foo", "bar", - Required, - Default{1})); + Required + )); config.defineOption(std::make_unique>( "goo", "bar", - Required, - Default{1})); + Required + )); CHECK_THROWS(config.validateRequiredFields()); diff --git a/test/config/test_llarp_config_output.cpp b/test/config/test_llarp_config_output.cpp index 13eb64c72..5ba47bf21 100644 --- a/test/config/test_llarp_config_output.cpp +++ b/test/config/test_llarp_config_output.cpp @@ -8,37 +8,37 @@ TEST_CASE("ConfigDefinition simple generate test", "[config]") { llarp::ConfigDefinition config{true}; - config.defineOption("foo", "bar", Required, Default{1}); + config.defineOption("foo", "bar", Required); config.defineOption("foo", "baz", Default{2}); - config.defineOption(std::make_unique>( - "foo", "quux", Required, Default{"hello"})); + config.defineOption( + std::make_unique>("foo", "quux", Default{"hello"})); - config.defineOption("argle", "bar", RelayOnly, Required, Default{3}); + config.defineOption("argle", "bar", RelayOnly, Required); config.defineOption("argle", "baz", Default{4}); - config.defineOption("argle", "quux", Required, Default{"the quick brown fox"}); + config.defineOption("argle", "quux", Default{"the quick brown fox"}); - config.defineOption("not", "for-routers", ClientOnly, Required, Default{1}); + config.defineOption("not", "for-routers", ClientOnly, Default{1}); std::string output = config.generateINIConfig(); CHECK(output == R"raw([foo] -bar=1 +#bar= #baz=2 -quux=hello +#quux=hello [argle] -bar=3 +#bar= #baz=4 -quux=the quick brown fox +#quux=the quick brown fox )raw"); } @@ -46,16 +46,24 @@ TEST_CASE("ConfigDefinition useValue test", "[config]") { llarp::ConfigDefinition config{true}; - config.defineOption("foo", "bar", Required, Default{1}); + config.defineOption("foo", "bar", Default{1}); - constexpr auto expected = "[foo]\n\n\nbar=1\n"; + constexpr auto expected = R"raw([foo] + + +#bar=1 +)raw"; CHECK(config.generateINIConfig(false) == expected); CHECK(config.generateINIConfig(true) == expected); config.addConfigValue("foo", "bar", "2"); - constexpr auto expectedWhenValueProvided = "[foo]\n\n\nbar=2\n"; + constexpr auto expectedWhenValueProvided = R"raw([foo] + + +bar=2 +)raw"; CHECK(config.generateINIConfig(false) == expected); CHECK(config.generateINIConfig(true) == expectedWhenValueProvided); @@ -67,8 +75,7 @@ TEST_CASE("ConfigDefinition section comments test") config.addSectionComments("foo", {"test comment"}); config.addSectionComments("foo", {"test comment 2"}); - config.defineOption(std::make_unique>( - "foo", "bar", Required, Default{1})); + config.defineOption(std::make_unique>("foo", "bar", Default{1})); std::string output = config.generateINIConfig(); @@ -77,7 +84,7 @@ TEST_CASE("ConfigDefinition section comments test") # test comment 2 -bar=1 +#bar=1 )raw"); } @@ -87,18 +94,21 @@ TEST_CASE("ConfigDefinition option comments test") config.addOptionComments("foo", "bar", {"test comment 1"}); config.addOptionComments("foo", "bar", {"test comment 2"}); - config.defineOption("foo", "bar", Required, Default{1}); + config.defineOption("foo", "bar", Default{1}); - config.defineOption("foo", "far", Default{"abc"}, + config.defineOption( + "foo", + "far", + Default{"abc"}, Comment{ - "Fill in the missing values:", - "___defg", + "Fill in the missing values:", + "___defg", }); config.defineOption("client", "omg", ClientOnly, Default{1}, Comment{"hi"}); config.defineOption("relay", "ftw", RelayOnly, Default{1}, Comment{"bye"}); - // has comment, so still gets shown. + // has comment, but is hidden: we show only the comment but not the value/default. config.defineOption("foo", "old-bar", Hidden, Default{456}); config.addOptionComments("foo", "old-bar", {"old bar option"}); @@ -112,14 +122,13 @@ TEST_CASE("ConfigDefinition option comments test") # test comment 1 # test comment 2 -bar=1 +#bar=1 # Fill in the missing values: # ___defg #far=abc # old bar option -#old-bar=456 [relay] @@ -139,7 +148,7 @@ TEST_CASE("ConfigDefinition empty comments test") config.addOptionComments("foo", "bar", {"option comment"}); config.addOptionComments("foo", "bar", {""}); - config.defineOption("foo", "bar", Required, Default{1}); + config.defineOption("foo", "bar", Default{1}); std::string output = config.generateINIConfig(); @@ -150,7 +159,7 @@ TEST_CASE("ConfigDefinition empty comments test") # option comment # -bar=1 +#bar=1 )raw"); } @@ -161,10 +170,10 @@ TEST_CASE("ConfigDefinition multi option comments") config.addSectionComments("foo", {"foo section comment"}); config.addOptionComments("foo", "bar", {"foo bar option comment"}); - config.defineOption("foo", "bar", Required, Default{1}); + config.defineOption("foo", "bar", Default{1}); config.addOptionComments("foo", "baz", {"foo baz option comment"}); - config.defineOption("foo", "baz", Required, Default{1}); + config.defineOption("foo", "baz", Default{1}); std::string output = config.generateINIConfig(); @@ -173,10 +182,9 @@ TEST_CASE("ConfigDefinition multi option comments") # foo bar option comment -bar=1 +#bar=1 # foo baz option comment -baz=1 +#baz=1 )raw"); } - diff --git a/test/config/test_llarp_config_values.cpp b/test/config/test_llarp_config_values.cpp index 8276cc59c..f0e9ebe17 100644 --- a/test/config/test_llarp_config_values.cpp +++ b/test/config/test_llarp_config_values.cpp @@ -75,6 +75,8 @@ run_config_test(mocks::Network env, std::string_view ini_str) throw std::runtime_error{"non zero return"}; } +const std::string ini_minimal = "[lokid]\nrpc=ipc://dummy\n"; + TEST_CASE("service node bind section on valid network", "[config]") { std::unordered_multimap env{ @@ -92,15 +94,14 @@ TEST_CASE("service node bind section on valid network", "[config]") REQUIRE(not mock_net.IsBogon(*maybe_addr)); } - SECTION("empty config") + SECTION("minimal config") { - std::string_view ini_str = ""; - REQUIRE_NOTHROW(run_config_test(env, ini_str)); + REQUIRE_NOTHROW(run_config_test(env, ini_minimal)); } SECTION("explicit bind via ifname") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [bind] mock0=443 )"; @@ -108,7 +109,7 @@ mock0=443 } SECTION("explicit bind via ip address") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [bind] inbound=1.1.1.1:443 )"; @@ -116,7 +117,7 @@ inbound=1.1.1.1:443 } SECTION("explicit bind via ip address with old syntax") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [bind] 1.1.1.1=443 )"; @@ -125,7 +126,7 @@ inbound=1.1.1.1:443 } SECTION("ip spoof fails") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [router] public-ip=8.8.8.8 public-port=443 @@ -136,7 +137,7 @@ inbound=1.1.1.1:443 } SECTION("explicit bind via ifname but fails from non existing ifname") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [bind] ligma0=443 )"; @@ -145,7 +146,7 @@ ligma0=443 SECTION("explicit bind via ifname but fails from using loopback") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [bind] lo=443 )"; @@ -154,7 +155,7 @@ lo=443 SECTION("explicit bind via explicit loopback") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [bind] inbound=127.0.0.1:443 )"; @@ -162,7 +163,7 @@ inbound=127.0.0.1:443 } SECTION("public ip provided but no bind section") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [router] public-ip=1.1.1.1 public-port=443 @@ -171,7 +172,7 @@ public-port=443 } SECTION("public ip provided with ip in bind section") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [router] public-ip=1.1.1.1 public-port=443 @@ -196,7 +197,7 @@ TEST_CASE("service node bind section on nat network", "[config]") SECTION("public ip provided via inbound directive") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [router] public-ip=1.1.1.1 public-port=443 @@ -209,7 +210,7 @@ inbound=10.1.1.1:443 SECTION("public ip provided with bind via ifname") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [router] public-ip=1.1.1.1 public-port=443 @@ -222,7 +223,7 @@ mock0=443 SECTION("public ip provided bind via wildcard ip") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [router] public-ip=1.1.1.1 public-port=443 @@ -232,7 +233,6 @@ inbound=0.0.0.0:443 )"; REQUIRE_THROWS(run_config_test(env, ini_str)); } - } TEST_CASE("service node bind section with multiple public ip", "[config]") @@ -242,14 +242,9 @@ TEST_CASE("service node bind section with multiple public ip", "[config]") {"mock0", llarp::IPRange::FromIPv4(2, 1, 1, 1, 32)}, {"lo", llarp::IPRange::FromIPv4(127, 0, 0, 1, 8)}, }; - SECTION("empty config") - { - std::string_view ini_str = ""; - REQUIRE_NOTHROW(run_config_test(env, ini_str)); - } SECTION("with old style wildcard for inbound and no public ip, fails") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [bind] 0.0.0.0=443 )"; @@ -257,7 +252,7 @@ TEST_CASE("service node bind section with multiple public ip", "[config]") } SECTION("with old style wildcard for outbound") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [bind] *=1443 )"; @@ -265,7 +260,7 @@ TEST_CASE("service node bind section with multiple public ip", "[config]") } SECTION("with wildcard via inbound directive no public ip given, fails") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [bind] inbound=0.0.0.0:443 )"; @@ -274,7 +269,7 @@ inbound=0.0.0.0:443 } SECTION("with wildcard via inbound directive primary public ip given") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [router] public-ip=1.1.1.1 public-port=443 @@ -286,7 +281,7 @@ inbound=0.0.0.0:443 } SECTION("with wildcard via inbound directive secondary public ip given") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [router] public-ip=2.1.1.1 public-port=443 @@ -298,7 +293,7 @@ inbound=0.0.0.0:443 } SECTION("with bind via interface name") { - std::string_view ini_str = R"( + auto ini_str = ini_minimal + R"( [bind] mock0=443 )";