From: Thomas Markwalder Date: Tue, 5 Feb 2019 13:24:15 +0000 (-0500) Subject: [#399,!215] Addressed review comments X-Git-Tag: 426-cassandra-unit-tests-ends-with-success-even-though-they-fail_base~2^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2b845cb053578428805537c258eb922571a7774d;p=thirdparty%2Fkea.git [#399,!215] Addressed review comments --- diff --git a/src/bin/dhcp4/tests/config_backend_unittest.cc b/src/bin/dhcp4/tests/config_backend_unittest.cc index a16a8cfc56..75d574b4bc 100644 --- a/src/bin/dhcp4/tests/config_backend_unittest.cc +++ b/src/bin/dhcp4/tests/config_backend_unittest.cc @@ -441,8 +441,10 @@ TEST_F(Dhcp4CBTest, mergeSharedNetworks) { staged_network = networks->getByName("two"); ASSERT_TRUE(staged_network); - // Subnet3, which is in db2 should not have been merged, since it is - // backend data is first found, first used. + // Subnet3, which is in db2 should not have been merged. + // We queried db1 first and the query returned data. In + // other words, we iterate over the backends, asking for + // data. We use the first data, we find. staged_network = networks->getByName("three"); ASSERT_FALSE(staged_network); } diff --git a/src/lib/dhcpsrv/cfg_shared_networks.cc b/src/lib/dhcpsrv/cfg_shared_networks.cc index c1558f7350..3ab6e3626e 100644 --- a/src/lib/dhcpsrv/cfg_shared_networks.cc +++ b/src/lib/dhcpsrv/cfg_shared_networks.cc @@ -24,8 +24,8 @@ CfgSharedNetworks4::merge(const CfgSharedNetworks4& other) { auto& index = networks_.get(); // Iterate over the subnets to be merged. They will replace the existing - // subnets with the same id. All new subnets will be inserted into the - // configuration into which we're merging. + // subnets with the same id. All new subnets will be inserted into this + // configuration. auto other_networks = other.getAll(); for (auto other_network = other_networks->begin(); other_network != other_networks->end(); ++other_network) { diff --git a/src/lib/dhcpsrv/cfg_shared_networks.h b/src/lib/dhcpsrv/cfg_shared_networks.h index c9094623f3..833b8234b9 100644 --- a/src/lib/dhcpsrv/cfg_shared_networks.h +++ b/src/lib/dhcpsrv/cfg_shared_networks.h @@ -124,7 +124,8 @@ public: /// @return true if there is a network with a specified server identifier. bool hasNetworkWithServerId(const asiolink::IOAddress& server_id) const; - /// @brief Merges specified shared network configuration into this configuration. + /// @brief Merges specified shared network configuration into this + /// configuration. /// /// This method merges networks from the @c other configuration into this /// configuration. The general rule is that existing networks are replaced @@ -132,14 +133,14 @@ public: /// /// For each network in @c other, do the following: /// - /// - Any associated subnets are removed. Shared networks retreived from - /// config backends, do not carry their associated subnets (if any) with them. - /// Subnet assignments are maintained by subnet merges. - /// - If a shared network of the same name, already exists in this + /// - Any associated subnets are removed. Shared networks retrieved from + /// config backends, do not carry their associated subnets (if any) with + /// them. (Subnet assignments are maintained by subnet merges). + /// - If a shared network of the same name already exists in this /// configuration: - /// - All of its associated subnets are movde to the "other" network - /// - The existing network is removed from this configuration - /// - The "other" network is added + /// - All of its associated subnets are moved to the "other" network. + /// - The existing network is removed from this configuration. + /// - The "other" network is added to this configuration. /// /// @warning The merge operation may affect the @c other configuration. /// Therefore, the caller must not rely on the data held in the @c other diff --git a/src/lib/dhcpsrv/cfg_subnets4.cc b/src/lib/dhcpsrv/cfg_subnets4.cc index bdcf2fe58a..fa097d329c 100644 --- a/src/lib/dhcpsrv/cfg_subnets4.cc +++ b/src/lib/dhcpsrv/cfg_subnets4.cc @@ -64,7 +64,8 @@ CfgSubnets4::merge(CfgSharedNetworks4Ptr networks, // subnets with the same id. All new subnets will be inserted into the // configuration into which we're merging. auto other_subnets = other.getAll(); - for (auto other_subnet = other_subnets->begin(); other_subnet != other_subnets->end(); + for (auto other_subnet = other_subnets->begin(); + other_subnet != other_subnets->end(); ++other_subnet) { // Check if there is a subnet with the same ID. @@ -74,7 +75,7 @@ CfgSubnets4::merge(CfgSharedNetworks4Ptr networks, // Subnet found. auto existing_subnet = *subnet_it; - // Continue if the existing subnet and other subnet + // If the existing subnet and other subnet // are the same instance skip it. if (existing_subnet == *other_subnet) { continue; @@ -106,7 +107,7 @@ CfgSubnets4::merge(CfgSharedNetworks4Ptr networks, } else { // This implies the shared-network collection we were given // is out of sync with the subnets we were given. - isc_throw(InvalidOperation, "Cannot assign subnet ID of '" + isc_throw(InvalidOperation, "Cannot assign subnet ID of " << (*other_subnet)->getID() << " to shared network: " << network_name << ", network does not exist"); diff --git a/src/lib/dhcpsrv/cfg_subnets4.h b/src/lib/dhcpsrv/cfg_subnets4.h index 762d8637f7..b6c1b80fb9 100644 --- a/src/lib/dhcpsrv/cfg_subnets4.h +++ b/src/lib/dhcpsrv/cfg_subnets4.h @@ -60,9 +60,9 @@ public: /// are the authority on their shared network assignments. It is also /// assumed that @ networks is the list of shared networks that should be /// used in making assignments. The general concept is that the overarching - /// merge process will first merge shared networks and then pass that list of - /// networks into this method. Subnets from @c other are then merged into this - /// configuration as follows: + /// merge process will first merge shared networks and then pass that list + /// of networks into this method. Subnets from @c other are then merged + /// into this configuration as follows: /// /// For each subnet in @c other: /// @@ -70,7 +70,7 @@ public: /// -# If it belongs to a shared network, remove it from that network /// -# Remove the subnet from this configuration and discard it /// - /// - Add the subnet from other to this configuration. + /// - Add the subnet from @c other to this configuration. /// - If that subnet is associated to shared network, find that network /// in @ networks and add that subnet to it. /// @@ -81,7 +81,7 @@ public: /// merged configuration. /// /// @param networks collection of shared networks that to which assignments - /// should be added. In other words, the list of shared networks that belong + /// should be added. In other words, the list of shared networks that belong /// to the same SrvConfig instance we are merging into. /// @param other the subnet configuration to be merged into this /// configuration. diff --git a/src/lib/dhcpsrv/shared_network.cc b/src/lib/dhcpsrv/shared_network.cc index 014442fa7b..3ae74b2f19 100644 --- a/src/lib/dhcpsrv/shared_network.cc +++ b/src/lib/dhcpsrv/shared_network.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2017-2018 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2017-2019 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -319,12 +319,14 @@ SharedNetwork6::add(const Subnet6Ptr& subnet) { Impl::add(subnets_, subnet); // Associate the subnet with this network. setSharedNetwork(subnet); + subnet->setSharedNetworkName(name_); } void SharedNetwork6::del(const SubnetID& subnet_id) { Subnet6Ptr subnet = Impl::del(subnets_, subnet_id); clearSharedNetwork(subnet); + subnet->setSharedNetworkName(""); } void diff --git a/src/lib/dhcpsrv/tests/cfg_shared_networks4_unittest.cc b/src/lib/dhcpsrv/tests/cfg_shared_networks4_unittest.cc index 918f46e278..5d704fba3b 100644 --- a/src/lib/dhcpsrv/tests/cfg_shared_networks4_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_shared_networks4_unittest.cc @@ -193,7 +193,7 @@ TEST(CfgSharedNetworks4Test, mergeNetworks) { SharedNetwork4Ptr network2(new SharedNetwork4("network2")); network2->setValid(Triplet(200)); - // Create network3 with one subnets. + // Create network3 with one subnet. SharedNetwork4Ptr network3(new SharedNetwork4("network3")); network3->setValid(Triplet(300)); ASSERT_NO_THROW(network3->add(subnet3)); @@ -240,7 +240,7 @@ TEST(CfgSharedNetworks4Test, mergeNetworks) { // Should still have 3 networks. - // Network1 should have doubled its valid life time but still only have + // Network1 should have doubled its valid lifetime but still only have // the orignal two subnets. Merge should discard assocations on CB // subnets and preserve the associations from existing config. ASSERT_EQ(3, cfg_to.getAll()->size()); @@ -251,7 +251,7 @@ TEST(CfgSharedNetworks4Test, mergeNetworks) { ASSERT_NO_FATAL_FAILURE(checkMergedNetwork(cfg_to, "network2", Triplet(200), std::vector())); - // Network1 should have doubled its valid life time and still subnet3. + // Network1 should have doubled its valid lifetime and still subnet3. ASSERT_NO_FATAL_FAILURE(checkMergedNetwork(cfg_to, "network3", Triplet(600), std::vector{SubnetID(3)})); } diff --git a/src/lib/dhcpsrv/tests/cfg_subnets4_unittest.cc b/src/lib/dhcpsrv/tests/cfg_subnets4_unittest.cc index bfea1173f8..c0984b26f9 100644 --- a/src/lib/dhcpsrv/tests/cfg_subnets4_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_subnets4_unittest.cc @@ -159,7 +159,7 @@ TEST(CfgSubnets4Test, mergeSubnets) { // Empty network pointer. SharedNetwork4Ptr no_network; - // Add Subnets1,2, and 4 to shared networks. + // Add Subnets 1, 2 and 4 to shared networks. ASSERT_NO_THROW(shared_network1->add(subnet1)); ASSERT_NO_THROW(shared_network2->add(subnet2)); ASSERT_NO_THROW(shared_network2->add(subnet4));