From 56939d5d79f3bc2ea6cc9177c7ee33ddadd31011 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Thu, 21 Feb 2019 10:40:51 -0500 Subject: [PATCH] [#402,!224] Addressed review comments --- .../dhcp4/tests/config_backend_unittest.cc | 28 +++++++++---------- src/lib/dhcpsrv/srv_config.cc | 1 - src/lib/dhcpsrv/srv_config.h | 25 +++++++++++------ src/lib/dhcpsrv/tests/srv_config_unittest.cc | 3 +- 4 files changed, 33 insertions(+), 24 deletions(-) diff --git a/src/bin/dhcp4/tests/config_backend_unittest.cc b/src/bin/dhcp4/tests/config_backend_unittest.cc index 59078ac8f6..4a446cc928 100644 --- a/src/bin/dhcp4/tests/config_backend_unittest.cc +++ b/src/bin/dhcp4/tests/config_backend_unittest.cc @@ -217,19 +217,19 @@ TEST_F(Dhcp4CBTest, mergeGlobals) { extractConfig(base_config); // Make some globals: - StampedValuePtr serverHostname(new StampedValue("server-hostname", "isc.example.org")); - StampedValuePtr declinePeriod(new StampedValue("decline-probation-period", Element::create(86400))); - StampedValuePtr calcTeeTimes(new StampedValue("calculate-tee-times", Element::create(bool(false)))); - StampedValuePtr t2Percent(new StampedValue("t2-percent", Element::create(0.75))); - StampedValuePtr renewTimer(new StampedValue("renew-timer", Element::create(500))); + StampedValuePtr server_hostname(new StampedValue("server-hostname", "isc.example.org")); + StampedValuePtr decline_period(new StampedValue("decline-probation-period", Element::create(86400))); + StampedValuePtr calc_tee_times(new StampedValue("calculate-tee-times", Element::create(bool(false)))); + StampedValuePtr t2_percent(new StampedValue("t2-percent", Element::create(0.75))); + StampedValuePtr renew_timer(new StampedValue("renew-timer", Element::create(500))); // Let's add all of the globals to the second backend. This will verify // we find them there. - db2_->createUpdateGlobalParameter4(ServerSelector::ALL(), serverHostname); - db2_->createUpdateGlobalParameter4(ServerSelector::ALL(), declinePeriod); - db2_->createUpdateGlobalParameter4(ServerSelector::ALL(), calcTeeTimes); - db2_->createUpdateGlobalParameter4(ServerSelector::ALL(), t2Percent); - db2_->createUpdateGlobalParameter4(ServerSelector::ALL(), renewTimer); + db2_->createUpdateGlobalParameter4(ServerSelector::ALL(), server_hostname); + db2_->createUpdateGlobalParameter4(ServerSelector::ALL(), decline_period); + db2_->createUpdateGlobalParameter4(ServerSelector::ALL(), calc_tee_times); + db2_->createUpdateGlobalParameter4(ServerSelector::ALL(), t2_percent); + db2_->createUpdateGlobalParameter4(ServerSelector::ALL(), renew_timer); // Should parse and merge without error. ASSERT_NO_FATAL_FAILURE(configure(base_config, CONTROL_RESULT_SUCCESS, "")); @@ -251,10 +251,10 @@ TEST_F(Dhcp4CBTest, mergeGlobals) { ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal("rebind-timer", Element::create(800))); // Verify that the implicit globals from the backend are there. - ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(serverHostname)); - ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(calcTeeTimes)); - ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(t2Percent)); - ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(renewTimer)); + ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(server_hostname)); + ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(calc_tee_times)); + ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(t2_percent)); + ASSERT_NO_FATAL_FAILURE(checkConfiguredGlobal(renew_timer)); } // This test verifies that externally configured option definitions diff --git a/src/lib/dhcpsrv/srv_config.cc b/src/lib/dhcpsrv/srv_config.cc index 6794dc1209..162aedc17b 100644 --- a/src/lib/dhcpsrv/srv_config.cc +++ b/src/lib/dhcpsrv/srv_config.cc @@ -262,7 +262,6 @@ SrvConfig::extractConfiguredGlobals(isc::data::ConstElementPtr config) { for (auto value = values.begin(); value != values.end(); ++value) { if (value->second->getType() != Element::list && value->second->getType() != Element::map) { - std::cout << "adding config'd global: " << value->first << std::endl; addConfiguredGlobal(value->first, value->second); } } diff --git a/src/lib/dhcpsrv/srv_config.h b/src/lib/dhcpsrv/srv_config.h index 54849ff272..d1c4a91b27 100644 --- a/src/lib/dhcpsrv/srv_config.h +++ b/src/lib/dhcpsrv/srv_config.h @@ -668,14 +668,23 @@ private: /// @brief Merges the DHCPv4 globals specified in the given configuration /// into this configuration. /// - /// This function iterates over the given config's explicitly - /// configured globals and either adds them to or updates the value - /// of this config's configured globals. - /// - /// It then iterates over the merged list, setting all of the - /// corresponding configuration member values (e.g. c@ - /// SrvConfig::echo_client_id_, @c SrvConfig::server_tag_). - /// + /// Configurable global values may be specified either via JSON + /// configuration (e.g. "echo-client-id":true) or as global parameters + /// within a configuration back end. Regardless of the source, these + /// values once provided, are stored in @c SrvConfig::configured_globals_. + /// Any such value that does not have an explicit specification should be + /// considered "unspecified" at the global scope. + /// + /// This function adds the configured globals from the "other" config + /// into this config's configured globals. If a value already exists + /// in this config, it will be overwritten with the value from the + /// "other" config. + /// + /// It then iterates over this merged list of globals, setting + /// any of the corresponding SrvConfig members that map to a + /// a configurable parameter (e.g. c@ SrvConfig::echo_client_id_, + /// @c SrvConfig::server_tag_). + /// /// @param other An object holding the configuration to be merged /// into this configuration. void mergeGlobals4(const SrvConfig& other); diff --git a/src/lib/dhcpsrv/tests/srv_config_unittest.cc b/src/lib/dhcpsrv/tests/srv_config_unittest.cc index cff1d6b23d..f46a07a83b 100644 --- a/src/lib/dhcpsrv/tests/srv_config_unittest.cc +++ b/src/lib/dhcpsrv/tests/srv_config_unittest.cc @@ -1055,7 +1055,8 @@ TEST_F(SrvConfigTest, mergeGlobals4) { ASSERT_NO_THROW(expected_globals = Element::fromJSON(exp_globals)) << "exp_globals didn't parse, test is broken"; - EXPECT_TRUE(expected_globals->equals(*(cfg_to.getConfiguredGlobals()))); + EXPECT_TRUE(isEquivalent(expected_globals, cfg_to.getConfiguredGlobals())); + } } // end of anonymous namespace -- 2.47.2