From: Marcin Siodelski Date: Fri, 1 Apr 2016 15:44:32 +0000 (+0200) Subject: [4302] Address further review comments. X-Git-Tag: trac4106_update_base~60^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=634cf7a2d4f92a62ec52e871d12d129ab9d86019;p=thirdparty%2Fkea.git [4302] Address further review comments. - cppcheck issues - Unique index for prefix and prefix length. --- diff --git a/src/bin/admin/scripts/mysql/dhcpdb_create.mysql b/src/bin/admin/scripts/mysql/dhcpdb_create.mysql index bd159d547c..85b768b0ba 100755 --- a/src/bin/admin/scripts/mysql/dhcpdb_create.mysql +++ b/src/bin/admin/scripts/mysql/dhcpdb_create.mysql @@ -413,7 +413,7 @@ CREATE UNIQUE INDEX key_dhcp4_ipv4_address_subnet_id ON hosts (ipv4_address ASC # Create index to search for reservations using address/prefix and prefix # length. -CREATE INDEX key_dhcp6_address_prefix_len ON ipv6_reservations (address ASC , prefix_len ASC); +CREATE UNIQUE INDEX key_dhcp6_address_prefix_len ON ipv6_reservations (address ASC , prefix_len ASC); # Create a table mapping host identifiers to their names. Values in this # table are used as a foreign key in hosts table to guarantee that only diff --git a/src/lib/dhcpsrv/mysql_host_data_source.cc b/src/lib/dhcpsrv/mysql_host_data_source.cc index 1c2300b8b6..d364d7283f 100644 --- a/src/lib/dhcpsrv/mysql_host_data_source.cc +++ b/src/lib/dhcpsrv/mysql_host_data_source.cc @@ -718,6 +718,8 @@ public: : MySqlHostExchange(), reserv_type_(0), reserv_type_null_(MLM_FALSE), ipv6_address_buffer_len_(0), prefix_len_(0), iaid_(0) { + memset(ipv6_address_buffer_, 0, sizeof(ipv6_address_buffer_)); + // Append additional columns returned by the queries. columns_.push_back("address"); columns_.push_back("prefix_len"); @@ -922,7 +924,7 @@ public: /// /// Initialize class members representing a single IPv6 reservation. MySqlIPv6ReservationExchange() - : host_id_(0), address_("::"), prefix_len_(0), type_(0), + : host_id_(0), address_("::"), address_len_(0), prefix_len_(0), type_(0), iaid_(0), resv_(IPv6Resrv::TYPE_NA, asiolink::IOAddress("::"), 128) { // Reset error table. diff --git a/src/lib/dhcpsrv/tests/generic_host_data_source_unittest.cc b/src/lib/dhcpsrv/tests/generic_host_data_source_unittest.cc index 4818d03d9b..90b70a6e30 100644 --- a/src/lib/dhcpsrv/tests/generic_host_data_source_unittest.cc +++ b/src/lib/dhcpsrv/tests/generic_host_data_source_unittest.cc @@ -27,7 +27,7 @@ GenericHostDataSourceTest::~GenericHostDataSourceTest() { } std::string -GenericHostDataSourceTest::generateHWAddr() { +GenericHostDataSourceTest::generateHWAddr(const bool new_identifier) { /// @todo: Consider moving this somewhere to lib/testutils. // Let's use something that is easily printable. That's convenient @@ -43,18 +43,20 @@ GenericHostDataSourceTest::generateHWAddr() { << static_cast(hwaddr[i]); } - // Increase the address for the next time we use it. - // This is primitive, but will work for 65k unique - // addresses. - hwaddr[sizeof(hwaddr) - 1]++; - if (hwaddr[sizeof(hwaddr) - 1] == 0) { - hwaddr[sizeof(hwaddr) - 2]++; + if (new_identifier) { + // Increase the address for the next time we use it. + // This is primitive, but will work for 65k unique + // addresses. + hwaddr[sizeof(hwaddr) - 1]++; + if (hwaddr[sizeof(hwaddr) - 1] == 0) { + hwaddr[sizeof(hwaddr) - 2]++; + } } return (tmp.str()); } std::string -GenericHostDataSourceTest::generateDuid() { +GenericHostDataSourceTest::generateDuid(const bool new_identifier) { /// @todo: Consider moving this somewhere to lib/testutils. // Let's use something that is easily printable. That's convenient @@ -70,9 +72,11 @@ GenericHostDataSourceTest::generateDuid() { // Increase the DUID for the next time we use it. // This is primitive, but will work for 65k unique // DUIDs. - duid[sizeof(duid) - 1]++; - if (duid[sizeof(duid) - 1] == 0) { - duid[sizeof(duid) - 2]++; + if (new_identifier) { + duid[sizeof(duid) - 1]++; + if (duid[sizeof(duid) - 1] == 0) { + duid[sizeof(duid) - 2]++; + } } return (tmp.str()); } @@ -106,17 +110,18 @@ HostPtr GenericHostDataSourceTest::initializeHost4(std::string address, HostPtr GenericHostDataSourceTest::initializeHost6(std::string address, Host::IdentifierType identifier, - bool prefix) { + bool prefix, + bool new_identifier) { string ident; string ident_type; switch (identifier) { case Host::IDENT_HWADDR: - ident = generateHWAddr(); + ident = generateHWAddr(new_identifier); ident_type = "hw-address"; break; case Host::IDENT_DUID: - ident = generateDuid(); + ident = generateDuid(new_identifier); ident_type = "duid"; break; default: @@ -724,24 +729,32 @@ GenericHostDataSourceTest::testSubnetId6(int subnets, Host::IdentifierType id) { // Make sure we have a pointer to the host data source. ASSERT_TRUE(hdsptr_); - HostPtr host = initializeHost6("2001:db8::0", id, true); - + HostPtr host; + IOAddress current_address("2001:db8::0"); for (int i = 0; i < subnets; ++i) { + // Last boolean value set to false indicates that the same identifier + // must be used for each generated host. + host = initializeHost6(current_address.toText(), id, true, false); + host->setIPv4SubnetID(i + 1000); host->setIPv6SubnetID(i + 1000); // Check that the same host can have reservations in multiple subnets. EXPECT_NO_THROW(hdsptr_->add(host)); + + // Increase address to make sure we don't assign the same address + // in different subnets. + current_address = IOAddress::increase(current_address); } // Check that the reservations can be retrieved from each subnet separately. for (int i = 0; i < subnets; ++i) { // Try to retrieve the host - ConstHostPtr from_hds = hdsptr_->get6(i + 1000, host->getDuid(), - host->getHWAddress()); + ConstHostPtr from_hds = hdsptr_->get6(i + 1000, id, &host->getIdentifier()[0], + host->getIdentifier().size()); - ASSERT_TRUE(from_hds); + ASSERT_TRUE(from_hds) << "failed for i=" << i; EXPECT_EQ(i + 1000, from_hds->getIPv6SubnetID()); } diff --git a/src/lib/dhcpsrv/tests/generic_host_data_source_unittest.h b/src/lib/dhcpsrv/tests/generic_host_data_source_unittest.h index 1e9259bfa5..e2b468dbc8 100644 --- a/src/lib/dhcpsrv/tests/generic_host_data_source_unittest.h +++ b/src/lib/dhcpsrv/tests/generic_host_data_source_unittest.h @@ -49,19 +49,26 @@ public: /// @param address IPv6 address to be reserved /// @param id type of identifier (IDENT_DUID or IDENT_HWADDR are supported) /// @param prefix reservation type (true = prefix, false = address) + /// @param new_identifier Boolean value indicating if new host + /// identifier should be generated or the same as previously. /// /// @return generated Host object HostPtr initializeHost6(std::string address, Host::IdentifierType id, - bool prefix); + bool prefix, bool new_identifier = true); /// @brief Generates a hardware address in text version. /// + /// @param increase A boolean value indicating if new address (increased) + /// must be generated or the same address as previously. /// @return HW address in textual form acceptable by Host constructor - std::string generateHWAddr(); + std::string generateHWAddr(const bool new_identifier = true); /// @brief Generates a hardware address in text version. + /// + /// @param increase A boolean value indicating if new DUID (increased) + /// must be generated or the same DUID as previously. /// @return DUID in textual form acceptable by Host constructor - std::string generateDuid(); + std::string generateDuid(const bool new_identifier = true); /// @brief Compares hardware addresses of the two hosts. /// diff --git a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc index b393847be2..baa33c2167 100644 --- a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc @@ -2213,9 +2213,6 @@ GenericLeaseMgrTest::testGetDeclinedLeases4() { EXPECT_NE(0, declined_state); EXPECT_NE(0, default_state); - // Remember expired leases returned. - std::vector saved_expired_leases = expired_leases; - // Remove expired leases again. expired_leases.clear(); @@ -2366,9 +2363,6 @@ GenericLeaseMgrTest::testGetDeclinedLeases6() { EXPECT_NE(0, declined_state); EXPECT_NE(0, default_state); - // Remember expired leases returned. - std::vector saved_expired_leases = expired_leases; - // Remove expired leases again. expired_leases.clear(); diff --git a/src/lib/dhcpsrv/testutils/schema_mysql_copy.h b/src/lib/dhcpsrv/testutils/schema_mysql_copy.h index 8656d6d496..05db8a1dd7 100755 --- a/src/lib/dhcpsrv/testutils/schema_mysql_copy.h +++ b/src/lib/dhcpsrv/testutils/schema_mysql_copy.h @@ -270,7 +270,7 @@ const char* create_statement[] = { "ON hosts " "(ipv4_address ASC, dhcp4_subnet_id ASC)", - "CREATE INDEX key_dhcp6_address_prefix_len " + "CREATE UNIQUE INDEX key_dhcp6_address_prefix_len " "ON ipv6_reservations (address ASC , prefix_len ASC)", "CREATE TABLE IF NOT EXISTS host_identifier_type ("