From 606b06cef9bae2038a10dad2ac89db8f93708e84 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Wed, 13 Nov 2013 17:20:57 +0100 Subject: [PATCH] [2940] Fixed handling of the NULL client identifier in the Memfile backend --- src/lib/dhcpsrv/lease.cc | 9 ++++++ src/lib/dhcpsrv/lease.h | 6 ++++ src/lib/dhcpsrv/memfile_lease_mgr.cc | 10 +++--- src/lib/dhcpsrv/memfile_lease_mgr.h | 32 +++---------------- .../tests/memfile_lease_mgr_unittest.cc | 27 +++++++++++++--- 5 files changed, 47 insertions(+), 37 deletions(-) diff --git a/src/lib/dhcpsrv/lease.cc b/src/lib/dhcpsrv/lease.cc index 81a4f87076..fdc05c460e 100644 --- a/src/lib/dhcpsrv/lease.cc +++ b/src/lib/dhcpsrv/lease.cc @@ -74,6 +74,15 @@ Lease4::Lease4(const Lease4& other) } } +std::vector +Lease4::getClientIdVector() const { + if(!client_id_) { + return std::vector(); + } + + return (client_id_->getClientId()); +} + Lease4& Lease4::operator=(const Lease4& other) { if (this != &other) { diff --git a/src/lib/dhcpsrv/lease.h b/src/lib/dhcpsrv/lease.h index 496f38a0da..3f98491f8b 100644 --- a/src/lib/dhcpsrv/lease.h +++ b/src/lib/dhcpsrv/lease.h @@ -209,6 +209,12 @@ struct Lease4 : public Lease { /// @param other the @c Lease4 object to be copied. Lease4(const Lease4& other); + /// @brief Returns a client identifier. + /// + /// @return A client identifier as vector, or an empty vector if client + /// identifier is NULL. + std::vector getClientIdVector() const; + /// @brief Assignment operator. /// /// @param other the @c Lease4 object to be assigned. diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.cc b/src/lib/dhcpsrv/memfile_lease_mgr.cc index 4f73278a76..9eefb1ff1e 100644 --- a/src/lib/dhcpsrv/memfile_lease_mgr.cc +++ b/src/lib/dhcpsrv/memfile_lease_mgr.cc @@ -80,7 +80,7 @@ Memfile_LeaseMgr::getLease4(const HWAddr& hwaddr) const { lease != idx.end(); ++lease) { // Every Lease4 has a hardware address, so we can compare it - if((* lease)->hwaddr_ == hwaddr.hwaddr_) { + if((*lease)->hwaddr_ == hwaddr.hwaddr_) { collection.push_back((* lease)); } } @@ -113,9 +113,9 @@ Memfile_LeaseMgr::getLease4(const HWAddr& hwaddr, SubnetID subnet_id) const { } Lease4Collection -Memfile_LeaseMgr::getLease4(const ClientId& clientid) const { +Memfile_LeaseMgr::getLease4(const ClientId& client_id) const { LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL, - DHCPSRV_MEMFILE_GET_CLIENTID).arg(clientid.toText()); + DHCPSRV_MEMFILE_GET_CLIENTID).arg(client_id.toText()); typedef Memfile_LeaseMgr::Lease4Storage::nth_index<0>::type SearchIndex; Lease4Collection collection; const SearchIndex& idx = storage4_.get<0>(); @@ -124,8 +124,8 @@ Memfile_LeaseMgr::getLease4(const ClientId& clientid) const { // client-id is not mandatory in DHCPv4. There can be a lease that does // not have a client-id. Dereferencing null pointer would be a bad thing - if((*lease)->client_id_ && *(*lease)->client_id_ == clientid) { - collection.push_back((* lease)); + if((*lease)->client_id_ && *(*lease)->client_id_ == client_id) { + collection.push_back((*lease)); } } diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.h b/src/lib/dhcpsrv/memfile_lease_mgr.h index aa81f0075b..2012a5b057 100644 --- a/src/lib/dhcpsrv/memfile_lease_mgr.h +++ b/src/lib/dhcpsrv/memfile_lease_mgr.h @@ -330,20 +330,8 @@ protected: // lease: client id and subnet id. boost::multi_index::composite_key< Lease4, - // The client id value is not directly accessible through the - // Lease4 object as it is wrapped with the ClientIdPtr object. - // Therefore we use the KeyFromKeyExtractor class to access - // client id through this cascaded structure. The client id - // is used as an index value. - KeyFromKeyExtractor< - // Specify that the vector holding client id value can be obtained - // from the ClientId object. - boost::multi_index::const_mem_fun, - &ClientId::getClientId>, - // Specify that the ClientId object (actually pointer to it) can - // be accessed by the client_id_ member of Lease4 class. - boost::multi_index::member - >, + boost::multi_index::const_mem_fun, + &Lease4::getClientIdVector>, // The subnet id is accessed through the subnet_id_ member. boost::multi_index::member > @@ -355,20 +343,8 @@ protected: // lease: client id and subnet id. boost::multi_index::composite_key< Lease4, - // The client id value is not directly accessible through the - // Lease4 object as it is wrapped with the ClientIdPtr object. - // Therefore we use the KeyFromKeyExtractor class to access - // client id through this cascaded structure. The client id - // is used as an index value. - KeyFromKeyExtractor< - // Specify that the vector holding client id value can be obtained - // from the ClientId object. - boost::multi_index::const_mem_fun, - &ClientId::getClientId>, - // Specify that the ClientId object (actually pointer to it) can - // be accessed by the client_id_ member of Lease4 class. - boost::multi_index::member - >, + boost::multi_index::const_mem_fun, + &Lease4::getClientIdVector>, // The hardware address is held in the hwaddr_ member of the // Lease4 object. boost::multi_index::member, diff --git a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc index a995f1aa81..2f12929f40 100644 --- a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc @@ -164,14 +164,17 @@ TEST_F(MemfileLeaseMgrTest, getLease4NullClientId) { // Let's initialize a specific lease ... But this time // We keep its client id for further lookup and // We clearly 'reset' it ... - Lease4Ptr lease = initializeLease4(straddress4_[4]); - ClientIdPtr client_id = lease->client_id_; - lease->client_id_ = ClientIdPtr(); - EXPECT_TRUE(lease_mgr->addLease(lease)); + Lease4Ptr leaseA = initializeLease4(straddress4_[4]); + ClientIdPtr client_id = leaseA->client_id_; + leaseA->client_id_ = ClientIdPtr(); + EXPECT_TRUE(lease_mgr->addLease(leaseA)); Lease4Collection returned = lease_mgr->getLease4(*client_id); // Shouldn't have our previous lease ... ASSERT_EQ(0, returned.size()); + Lease4Ptr leaseB = initializeLease4(straddress4_[5]); + // Shouldn't throw any null pointer exception + EXPECT_NO_THROW(lease_mgr->addLease(leaseB)); } // Checks lease4 retrieval through HWAddr @@ -216,4 +219,20 @@ TEST_F(MemfileLeaseMgrTest, getLease4ClientIdHWAddrSubnetId) { EXPECT_TRUE(lease == Lease4Ptr()); } +TEST_F(MemfileLeaseMgrTest, getLease4ClientIdVector) { + const LeaseMgr::ParameterMap pmap; + boost::scoped_ptr lease_mgr(new Memfile_LeaseMgr(pmap)); + + const std::vector vec; + Lease4Ptr lease = initializeLease4(straddress4_[7]); + // Check that this lease has null client-id + ASSERT_TRUE(lease->client_id_ == ClientIdPtr()); + // Check that this return empty vector + ASSERT_TRUE(lease->getClientIdVector() == vec); + // Let's take a lease with client-id not null + lease = initializeLease4(straddress4_[6]); + // Check that they return same client-id value + ASSERT_TRUE(lease->client_id_->getClientId() == lease->getClientIdVector()); +} + }; // end of anonymous namespace -- GitLab