From: Tomek Mrugalski Date: Mon, 27 Oct 2014 18:12:53 +0000 (+0100) Subject: [3555] Changes after review: X-Git-Tag: trac3629a_base~5^2~3 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=cb2fc012c63ebeeb58e6de427e6011dd745d4eac;p=thirdparty%2Fkea.git [3555] Changes after review: - Comments updated - getRawHWAddr renamed to getHWAddrVector and moved to Lease class - HWAddr object is now copied in copy ctor and = operator --- diff --git a/src/lib/dhcpsrv/csv_lease_file6.h b/src/lib/dhcpsrv/csv_lease_file6.h index 9cd8de497e..105b4e505c 100644 --- a/src/lib/dhcpsrv/csv_lease_file6.h +++ b/src/lib/dhcpsrv/csv_lease_file6.h @@ -106,6 +106,7 @@ private: /// - fqdn_fwd /// - fqdn_rev /// - hostname + /// - hwaddr void initColumns(); /// diff --git a/src/lib/dhcpsrv/dhcpsrv_messages.mes b/src/lib/dhcpsrv/dhcpsrv_messages.mes index 3fe6bcbd9e..c7723d2a7f 100644 --- a/src/lib/dhcpsrv/dhcpsrv_messages.mes +++ b/src/lib/dhcpsrv/dhcpsrv_messages.mes @@ -313,7 +313,7 @@ testing but should not be enabled in normal circumstances. Non-persistence mode is enabled when 'persist4=no persist6=no' parameters are specified in the database access string. -% DHCPSRV_MEMFILE_READ_HWADDR_FAIL failed to read hardware address from disk: %1 +% DHCPSRV_MEMFILE_READ_HWADDR_FAIL failed to read hardware address from lease file: %1 A warning message issued when read attempt of the hardware address stored in a disk file failed. The parameter should provide the exact nature of the failure. The database read will continue, but that particular lease will no longer diff --git a/src/lib/dhcpsrv/lease.cc b/src/lib/dhcpsrv/lease.cc index 7ae7459974..4dc80b5abc 100644 --- a/src/lib/dhcpsrv/lease.cc +++ b/src/lib/dhcpsrv/lease.cc @@ -72,6 +72,11 @@ Lease4::Lease4(const Lease4& other) fixed_ = other.fixed_; comments_ = other.comments_; + // Copy the hardware address if it is defined. + if (other.hwaddr_) { + hwaddr_.reset(new HWAddr(*other.hwaddr_)); + } + if (other.client_id_) { client_id_.reset(new ClientId(other.client_id_->getClientId())); @@ -92,12 +97,10 @@ Lease4::getClientIdVector() const { } const std::vector& -Lease4::getRawHWAddr() const { +Lease::getHWAddrVector() const { if (!hwaddr_) { - // something is wrong, very wrong. Hardware address must always - // be present for all IPv4 leases. - isc_throw(BadValue, "Lease4 for address " << addr_.toText() - << " is missing a hardware address"); + static std::vector empty_vec; + return (empty_vec); } return (hwaddr_->hwaddr_); } @@ -118,12 +121,20 @@ Lease4::matches(const Lease4& other) const { // Note that hwaddr_ is now a poiner to the HWAddr structure. // We can't simply compare smart pointers, we need to compare the - // actual objects they point to. It is ok to not check whether they - // are non-NULL, as every Lease4 must have hardware address. + // actual objects they point to. If one of the leases had a hardware address + // and the other doesn't, they clearly don't match. + if ( (!hwaddr_ && other.hwaddr_) || + (hwaddr_ && !other.hwaddr_) ) { + return (false); + } + + // The lease is equal if addresses and extensions match and there is + // either the same hardware address on both or neither have hardware address + // specified. return (addr_ == other.addr_ && ext_ == other.ext_ && - *hwaddr_ == *other.hwaddr_); - + (!hwaddr_ || + *hwaddr_ == *other.hwaddr_) ); } Lease4& @@ -141,7 +152,11 @@ Lease4::operator=(const Lease4& other) { fqdn_rev_ = other.fqdn_rev_; comments_ = other.comments_; ext_ = other.ext_; - hwaddr_ = other.hwaddr_; + + // Copy the hardware address if it is defined. + if (other.hwaddr_) { + hwaddr_.reset(new HWAddr(*other.hwaddr_)); + } if (other.client_id_) { client_id_.reset(new ClientId(other.client_id_->getClientId())); @@ -211,6 +226,7 @@ Lease6::toText() const { << "Pref life: " << preferred_lft_ << "\n" << "Valid life: " << valid_lft_ << "\n" << "Cltt: " << cltt_ << "\n" + << "Hardware addr: " << (hwaddr_?hwaddr_->toText(false):"(none)") << "Subnet ID: " << subnet_id_ << "\n"; return (stream.str()); @@ -225,6 +241,7 @@ Lease4::toText() const { << "T1: " << t1_ << "\n" << "T2: " << t2_ << "\n" << "Cltt: " << cltt_ << "\n" + << "Hardware addr: " << (hwaddr_?hwaddr_->toText(false):"(none)") << "Subnet ID: " << subnet_id_ << "\n"; return (stream.str()); diff --git a/src/lib/dhcpsrv/lease.h b/src/lib/dhcpsrv/lease.h index 220e73634b..bea3715277 100644 --- a/src/lib/dhcpsrv/lease.h +++ b/src/lib/dhcpsrv/lease.h @@ -156,6 +156,15 @@ struct Lease { /// lease is equal to the FQDN data of our lease (true) or not (false). bool hasIdenticalFqdn(const Lease& other) const; + /// @brief Returns raw (as vector) hardware address + /// + /// This method is needed in multi-index container as key extractor. + /// The const reference is only valid as long as the object that returned it. + /// In the unlikely case when Lease4 does not have a hardware address, + /// the function will return an empty vector. + /// + /// @return const reference to the hardware address + const std::vector& getHWAddrVector() const; }; /// @brief Structure that holds a lease for IPv4 address @@ -230,14 +239,6 @@ struct Lease4 : public Lease { /// or an empty vector if client identifier is NULL. const std::vector& getClientIdVector() const; - /// @brief Returns raw (as vector) hardware address - /// - /// This method is needed in multi-index container as key extractor. - /// The const reference is only valid as long as the object that returned it. - /// - /// @return const reference to the hardware address - const std::vector& getRawHWAddr() const; - /// @brief Check if two objects encapsulate the lease for the same /// client. /// diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.h b/src/lib/dhcpsrv/memfile_lease_mgr.h index 0d6a0654f4..fefa785f8f 100644 --- a/src/lib/dhcpsrv/memfile_lease_mgr.h +++ b/src/lib/dhcpsrv/memfile_lease_mgr.h @@ -454,8 +454,8 @@ protected: // Lease4 object, which is a HWAddr object. Boost does not // provide a key extractor for getting a member of a member, // so we need a simple method for that. - boost::multi_index::const_mem_fun&, - &Lease4::getRawHWAddr>, + boost::multi_index::const_mem_fun&, + &Lease::getHWAddrVector>, // The subnet id is held in the subnet_id_ member of Lease4 // class. Note that the subnet_id_ is defined in the base // class (Lease) so we have to point to this class rather @@ -493,8 +493,8 @@ protected: // access the raw data using lease->hwaddr_->hwaddr_, but Boost // doesn't seem to provide a way to use member of a member for this, // so we need a simple key extractor method (getRawHWAddr). - boost::multi_index::const_mem_fun&, - &Lease4::getRawHWAddr>, + boost::multi_index::const_mem_fun&, + &Lease::getHWAddrVector>, // The subnet id is accessed through the subnet_id_ member. boost::multi_index::member > diff --git a/src/lib/dhcpsrv/tests/lease_unittest.cc b/src/lib/dhcpsrv/tests/lease_unittest.cc index fcb1b108d2..e517caaa90 100644 --- a/src/lib/dhcpsrv/tests/lease_unittest.cc +++ b/src/lib/dhcpsrv/tests/lease_unittest.cc @@ -141,6 +141,13 @@ TEST_F(Lease4Test, copyConstructor) { EXPECT_TRUE(lease == copied_lease); // Client IDs are equal, but they should be in two distinct pointers. EXPECT_FALSE(lease.client_id_ == copied_lease.client_id_); + + // Hardware addresses are equal, but they should point to two objects, + // each holding the same data. The content should be equal... + EXPECT_TRUE(*lease.hwaddr_ == *copied_lease.hwaddr_); + + // ... but it should point to different objects. + EXPECT_FALSE(lease.hwaddr_ == copied_lease.hwaddr_); } // This test verfies that the assignment operator copies all Lease4 fields @@ -172,6 +179,13 @@ TEST_F(Lease4Test, operatorAssign) { EXPECT_TRUE(lease == copied_lease); // Client IDs are equal, but they should be in two distinct pointers. EXPECT_FALSE(lease.client_id_ == copied_lease.client_id_); + + // Hardware addresses are equal, but they should point to two objects, + // each holding the same data. The content should be equal... + EXPECT_TRUE(*lease.hwaddr_ == *copied_lease.hwaddr_); + + // ... but it should point to different objects. + EXPECT_FALSE(lease.hwaddr_ == copied_lease.hwaddr_); } // This test verifies that the matches() returns true if two leases differ