From: Francis Dupont Date: Mon, 11 May 2020 07:50:15 +0000 (+0200) Subject: [#1147] Checkpoint before alloc cleanup X-Git-Tag: Kea-1.7.9~134 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=be8c8f76eb33a6dac22445d96c98cbc397214779;p=thirdparty%2Fkea.git [#1147] Checkpoint before alloc cleanup --- diff --git a/src/bin/dhcp4/client_handler.cc b/src/bin/dhcp4/client_handler.cc index 700e2bc05f..4d68679523 100644 --- a/src/bin/dhcp4/client_handler.cc +++ b/src/bin/dhcp4/client_handler.cc @@ -175,7 +175,7 @@ ClientHandler::tryLock(Pkt4Ptr query, ContinuationPtr cont) { } if (!duid && !hwaddr) { // Can't do something useful: cross fingers. - return (false); + return (true); } ClientPtr holder_id; @@ -195,7 +195,7 @@ ClientHandler::tryLock(Pkt4Ptr query, ContinuationPtr cont) { } if (!holder_id) { if (!hwaddr) { - return (false); + return (true); } // Try to acquire the by-hw-addr lock and return the holder // when it failed. @@ -204,7 +204,7 @@ ClientHandler::tryLock(Pkt4Ptr query, ContinuationPtr cont) { if (!holder_hw) { locked_hwaddr_ = hwaddr; lockByHWAddr(); - return (false); + return (true); } } @@ -265,7 +265,7 @@ ClientHandler::tryLock(Pkt4Ptr query, ContinuationPtr cont) { stats::StatsMgr::instance().addValue("pkt4-receive-drop", static_cast(1)); } - return (true); + return (false); } } // namespace dhcp diff --git a/src/bin/dhcp4/client_handler.h b/src/bin/dhcp4/client_handler.h index 166dd5a281..5a29074c40 100644 --- a/src/bin/dhcp4/client_handler.h +++ b/src/bin/dhcp4/client_handler.h @@ -50,14 +50,13 @@ public: /// @brief Tries to acquires a client. /// /// Lookup the client: - /// - if not found insert the client in the clients map and return false - /// - if found and has a continuation put the continuation in the holder + /// - if not found insert the client in the clients map and return true + /// - if found, if has a continuation put it in the holder, /// and return false - /// - if found and has no continuation return true /// /// @param query The query from the client. /// @param cont The continuation in the case the client was held. - /// @return false if the client was acquired, true if there is already + /// @return true if the client was acquired, false if there is already /// a query from the same client. bool tryLock(Pkt4Ptr query, ContinuationPtr cont = ContinuationPtr()); diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index bdd6a2c46f..2b5008b59a 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -1261,7 +1261,7 @@ Dhcpv4Srv::processDhcp4Query(Pkt4Ptr& query, Pkt4Ptr& rsp, ContinuationPtr cont = makeContinuation(std::bind(&Dhcpv4Srv::processDhcp4QueryAndSendResponse, this, query, rsp, allow_packet_park)); - if (client_handler.tryLock(query, cont)) { + if (!client_handler.tryLock(query, cont)) { return; } } diff --git a/src/bin/dhcp4/tests/client_handler_unittest.cc b/src/bin/dhcp4/tests/client_handler_unittest.cc index c1c39b3172..51721f34af 100644 --- a/src/bin/dhcp4/tests/client_handler_unittest.cc +++ b/src/bin/dhcp4/tests/client_handler_unittest.cc @@ -139,7 +139,7 @@ TEST_F(ClientHandleTest, oneQuery) { // Try to lock it. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler.tryLock(dis)); + EXPECT_NO_THROW(duplicate = !client_handler.tryLock(dis)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -167,7 +167,7 @@ TEST_F(ClientHandleTest, sharedQueriesById) { // Try to lock it with the discover. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler.tryLock(dis)); + EXPECT_NO_THROW(duplicate = !client_handler.tryLock(dis)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -176,7 +176,7 @@ TEST_F(ClientHandleTest, sharedQueriesById) { ClientHandler client_handler2; // Try to lock it with a request. - EXPECT_NO_THROW(duplicate = client_handler2.tryLock(req)); + EXPECT_NO_THROW(duplicate = !client_handler2.tryLock(req)); // Should return true (race with the duplicate). EXPECT_TRUE(duplicate); @@ -204,7 +204,7 @@ TEST_F(ClientHandleTest, sharedQueriesByHWAddr) { // Try to lock it with the discover. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler.tryLock(dis)); + EXPECT_NO_THROW(duplicate = !client_handler.tryLock(dis)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -213,7 +213,7 @@ TEST_F(ClientHandleTest, sharedQueriesByHWAddr) { ClientHandler client_handler2; // Try to lock it with a request. - EXPECT_NO_THROW(duplicate = client_handler2.tryLock(req)); + EXPECT_NO_THROW(duplicate = !client_handler2.tryLock(req)); // Should return true (race with the duplicate). EXPECT_TRUE(duplicate); @@ -240,7 +240,7 @@ TEST_F(ClientHandleTest, sharedQueriesByHWAddrOnly) { // Try to lock it with the discover. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler.tryLock(dis)); + EXPECT_NO_THROW(duplicate = !client_handler.tryLock(dis)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -249,7 +249,7 @@ TEST_F(ClientHandleTest, sharedQueriesByHWAddrOnly) { ClientHandler client_handler2; // Try to lock it with a request. - EXPECT_NO_THROW(duplicate = client_handler2.tryLock(req)); + EXPECT_NO_THROW(duplicate = !client_handler2.tryLock(req)); // Should return true (race with the duplicate). EXPECT_TRUE(duplicate); @@ -279,7 +279,7 @@ TEST_F(ClientHandleTest, sequence) { // Try to lock it with the discover. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler.tryLock(dis)); + EXPECT_NO_THROW(duplicate = !client_handler.tryLock(dis)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -295,7 +295,7 @@ TEST_F(ClientHandleTest, sequence) { // Try to lock it with a request. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler2.tryLock(req)); + EXPECT_NO_THROW(duplicate = !client_handler2.tryLock(req)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -326,7 +326,7 @@ TEST_F(ClientHandleTest, notSharedQueries) { // Try to lock it with the discover. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler.tryLock(dis)); + EXPECT_NO_THROW(duplicate = !client_handler.tryLock(dis)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -335,7 +335,7 @@ TEST_F(ClientHandleTest, notSharedQueries) { ClientHandler client_handler2; // Try to lock it with a request. - EXPECT_NO_THROW(duplicate = client_handler2.tryLock(req)); + EXPECT_NO_THROW(duplicate = !client_handler2.tryLock(req)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -358,7 +358,7 @@ TEST_F(ClientHandleTest, noClientIdHWAddr) { // Try to lock it with the discover. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler.tryLock(dis)); + EXPECT_NO_THROW(duplicate = !client_handler.tryLock(dis)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -367,7 +367,7 @@ TEST_F(ClientHandleTest, noClientIdHWAddr) { ClientHandler client_handler2; // Try to lock it with a request. - EXPECT_NO_THROW(duplicate = client_handler2.tryLock(req)); + EXPECT_NO_THROW(duplicate = !client_handler2.tryLock(req)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -403,7 +403,7 @@ TEST_F(ClientHandleTest, doubleTryLockById) { // Try to lock it. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler.tryLock(dis)); + EXPECT_NO_THROW(duplicate = !client_handler.tryLock(dis)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -427,7 +427,7 @@ TEST_F(ClientHandleTest, doubleTryLockByHWAddr) { // Try to lock it. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler.tryLock(dis)); + EXPECT_NO_THROW(duplicate = !client_handler.tryLock(dis)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -468,7 +468,7 @@ TEST_F(ClientHandleTest, serializeTwoQueriesById) { // Try to lock it with the discover. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler.tryLock(dis, cont1)); + EXPECT_NO_THROW(duplicate = !client_handler.tryLock(dis, cont1)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -481,10 +481,10 @@ TEST_F(ClientHandleTest, serializeTwoQueriesById) { makeContinuation(std::bind(&ClientHandleTest::setCalled2, this)); // Try to lock it with a request. - EXPECT_NO_THROW(duplicate = client_handler2.tryLock(req, cont2)); + EXPECT_NO_THROW(duplicate = !client_handler2.tryLock(req, cont2)); - // Should return false (multi-threading enforces serialization). - EXPECT_FALSE(duplicate); + // Should return true (race with the duplicate). + EXPECT_TRUE(duplicate); } catch (const std::exception& ex) { ADD_FAILURE() << "unexpected exception: " << ex.what(); } @@ -526,7 +526,7 @@ TEST_F(ClientHandleTest, serializeTwoQueriesByHWAddr) { // Try to lock it with the discover. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler.tryLock(dis, cont1)); + EXPECT_NO_THROW(duplicate = !client_handler.tryLock(dis, cont1)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -539,10 +539,10 @@ TEST_F(ClientHandleTest, serializeTwoQueriesByHWAddr) { makeContinuation(std::bind(&ClientHandleTest::setCalled2, this)); // Try to lock it with a request. - EXPECT_NO_THROW(duplicate = client_handler2.tryLock(req, cont2)); + EXPECT_NO_THROW(duplicate = !client_handler2.tryLock(req, cont2)); - // Should return false (multi-threading enforces serialization). - EXPECT_FALSE(duplicate); + // Should return true (race with the duplicate). + EXPECT_TRUE(duplicate); } catch (const std::exception& ex) { ADD_FAILURE() << "unexpected exception: " << ex.what(); } @@ -580,7 +580,7 @@ TEST_F(ClientHandleTest, serializeNoContById) { // Try to lock it with the discover. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler.tryLock(dis)); + EXPECT_NO_THROW(duplicate = !client_handler.tryLock(dis)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -589,9 +589,9 @@ TEST_F(ClientHandleTest, serializeNoContById) { ClientHandler client_handler2; // Try to lock it with a request. - EXPECT_NO_THROW(duplicate = client_handler2.tryLock(req)); + EXPECT_NO_THROW(duplicate = !client_handler2.tryLock(req)); - // Should return true (duplicate without continuation). + // Should return true (race with the duplicate). EXPECT_TRUE(duplicate); } catch (const std::exception& ex) { ADD_FAILURE() << "unexpected exception: " << ex.what(); @@ -628,7 +628,7 @@ TEST_F(ClientHandleTest, serializeNoContByHWAddr) { // Try to lock it with the discover. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler.tryLock(dis)); + EXPECT_NO_THROW(duplicate = !client_handler.tryLock(dis)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -637,9 +637,9 @@ TEST_F(ClientHandleTest, serializeNoContByHWAddr) { ClientHandler client_handler2; // Try to lock it with a request. - EXPECT_NO_THROW(duplicate = client_handler2.tryLock(req)); + EXPECT_NO_THROW(duplicate = !client_handler2.tryLock(req)); - // Should return true (duplicate without continuation). + // Should return true (race with the duplicate). EXPECT_TRUE(duplicate); } catch (const std::exception& ex) { ADD_FAILURE() << "unexpected exception: " << ex.what(); @@ -686,7 +686,7 @@ TEST_F(ClientHandleTest, serializeThreeQueriesById) { // Try to lock it with the discover. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler.tryLock(dis, cont1)); + EXPECT_NO_THROW(duplicate = !client_handler.tryLock(dis, cont1)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -699,10 +699,10 @@ TEST_F(ClientHandleTest, serializeThreeQueriesById) { makeContinuation(std::bind(&ClientHandleTest::setCalled2, this)); // Try to lock it with a request. - EXPECT_NO_THROW(duplicate = client_handler2.tryLock(req, cont2)); + EXPECT_NO_THROW(duplicate = !client_handler2.tryLock(req, cont2)); - // Should return false (multi-threading enforces serialization). - EXPECT_FALSE(duplicate); + // Should return true (race with the duplicate). + EXPECT_TRUE(duplicate); // Get a third client handler. ClientHandler client_handler3; @@ -712,10 +712,10 @@ TEST_F(ClientHandleTest, serializeThreeQueriesById) { makeContinuation(std::bind(&ClientHandleTest::setCalled3, this)); // Try to lock it with a release. - EXPECT_NO_THROW(duplicate = client_handler3.tryLock(rel, cont3)); + EXPECT_NO_THROW(duplicate = !client_handler3.tryLock(rel, cont3)); - // Should return false (multi-threading enforces serialization). - EXPECT_FALSE(duplicate); + // Should return true (race with the duplicate). + EXPECT_TRUE(duplicate); } catch (const std::exception& ex) { ADD_FAILURE() << "unexpected exception: " << ex.what(); } @@ -764,7 +764,7 @@ TEST_F(ClientHandleTest, serializeThreeQueriesHWAddr) { // Try to lock it with the discover. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler.tryLock(dis, cont1)); + EXPECT_NO_THROW(duplicate = !client_handler.tryLock(dis, cont1)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -777,10 +777,10 @@ TEST_F(ClientHandleTest, serializeThreeQueriesHWAddr) { makeContinuation(std::bind(&ClientHandleTest::setCalled2, this)); // Try to lock it with a request. - EXPECT_NO_THROW(duplicate = client_handler2.tryLock(req, cont2)); + EXPECT_NO_THROW(duplicate = !client_handler2.tryLock(req, cont2)); - // Should return false (multi-threading enforces serialization). - EXPECT_FALSE(duplicate); + // Should return true (race with the duplicate). + EXPECT_TRUE(duplicate); // Get a third client handler. ClientHandler client_handler3; @@ -790,10 +790,10 @@ TEST_F(ClientHandleTest, serializeThreeQueriesHWAddr) { makeContinuation(std::bind(&ClientHandleTest::setCalled3, this)); // Try to lock it with a release. - EXPECT_NO_THROW(duplicate = client_handler3.tryLock(rel, cont3)); + EXPECT_NO_THROW(duplicate = !client_handler3.tryLock(rel, cont3)); - // Should return false (multi-threading enforces serialization). - EXPECT_FALSE(duplicate); + // Should return true (race with the duplicate). + EXPECT_TRUE(duplicate); } catch (const std::exception& ex) { ADD_FAILURE() << "unexpected exception: " << ex.what(); } @@ -846,7 +846,7 @@ TEST_F(ClientHandleTest, serializeThreeQueriesMixed) { // Try to lock it with the discover. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler.tryLock(dis, cont1)); + EXPECT_NO_THROW(duplicate = !client_handler.tryLock(dis, cont1)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -859,10 +859,10 @@ TEST_F(ClientHandleTest, serializeThreeQueriesMixed) { makeContinuation(std::bind(&ClientHandleTest::setCalled2, this)); // Try to lock it with a request. - EXPECT_NO_THROW(duplicate = client_handler2.tryLock(req, cont2)); + EXPECT_NO_THROW(duplicate = !client_handler2.tryLock(req, cont2)); - // Should return false (multi-threading enforces serialization). - EXPECT_FALSE(duplicate); + // Should return true (race with the duplicate). + EXPECT_TRUE(duplicate); // Get a third client handler. ClientHandler client_handler3; @@ -872,10 +872,10 @@ TEST_F(ClientHandleTest, serializeThreeQueriesMixed) { makeContinuation(std::bind(&ClientHandleTest::setCalled3, this)); // Try to lock it with a release. - EXPECT_NO_THROW(duplicate = client_handler3.tryLock(rel, cont3)); + EXPECT_NO_THROW(duplicate = !client_handler3.tryLock(rel, cont3)); - // Should return false (multi-threading enforces serialization). - EXPECT_FALSE(duplicate); + // Should return true (race with the duplicate). + EXPECT_TRUE(duplicate); } catch (const std::exception& ex) { ADD_FAILURE() << "unexpected exception: " << ex.what(); } diff --git a/src/bin/dhcp6/client_handler.cc b/src/bin/dhcp6/client_handler.cc index e2eba6f62e..f7c323a960 100644 --- a/src/bin/dhcp6/client_handler.cc +++ b/src/bin/dhcp6/client_handler.cc @@ -98,7 +98,7 @@ ClientHandler::tryLock(Pkt6Ptr query, ContinuationPtr cont) { const DuidPtr& duid = query->getClientId(); if (!duid) { // Can't do something useful: cross fingers. - return (false); + return (true); } if (duid->getDuid().empty()) { // A lot of code assumes this will never happen... @@ -113,7 +113,7 @@ ClientHandler::tryLock(Pkt6Ptr query, ContinuationPtr cont) { locked_ = duid; client_.reset(new Client(query, duid)); lock(); - return (false); + return (true); } } // This query can be a duplicate so put the continuation. @@ -144,7 +144,7 @@ ClientHandler::tryLock(Pkt6Ptr query, ContinuationPtr cont) { .arg(holder->thread_); stats::StatsMgr::instance().addValue("pkt6-receive-drop", static_cast(1)); - return (true); + return (false); } } // namespace dhcp diff --git a/src/bin/dhcp6/client_handler.h b/src/bin/dhcp6/client_handler.h index 9e9b248fa4..b7a4a2783d 100644 --- a/src/bin/dhcp6/client_handler.h +++ b/src/bin/dhcp6/client_handler.h @@ -48,14 +48,13 @@ public: /// @brief Tries to acquires a client. /// /// Lookup the client: - /// - if not found insert the client in the clients map and return false - /// - if found and has a continuation put the continuation in the holder + /// - if not found insert the client in the clients map and return true + /// - if found, if has a continuation put it in the holder, /// and return false - /// - if found and has no continuation return true /// /// @param query The query from the client. /// @param cont The continuation in the case the client was held. - /// @return false if the client was acquired, true if there is already + /// @return true if the client was acquired, false if there is already /// a query from the same client. bool tryLock(Pkt6Ptr query, ContinuationPtr cont = ContinuationPtr()); diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 8a272cd335..2bfeba1f9b 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -854,7 +854,7 @@ Dhcpv6Srv::processDhcp6Query(Pkt6Ptr& query, Pkt6Ptr& rsp) { ContinuationPtr cont = makeContinuation(std::bind(&Dhcpv6Srv::processDhcp6QueryAndSendResponse, this, query, rsp)); - drop = client_handler.tryLock(query, cont); + drop = !client_handler.tryLock(query, cont); } // Stop here if ClientHandler tryLock decided the packet is a duplicate. diff --git a/src/bin/dhcp6/tests/client_handler_unittest.cc b/src/bin/dhcp6/tests/client_handler_unittest.cc index dc3b330443..3a7a9669f9 100644 --- a/src/bin/dhcp6/tests/client_handler_unittest.cc +++ b/src/bin/dhcp6/tests/client_handler_unittest.cc @@ -123,7 +123,7 @@ TEST_F(ClientHandleTest, oneQuery) { // Try to lock it. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler.tryLock(sol)); + EXPECT_NO_THROW(duplicate = !client_handler.tryLock(sol)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -149,7 +149,7 @@ TEST_F(ClientHandleTest, sharedQueries) { // Try to lock it with the solicit. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler.tryLock(sol)); + EXPECT_NO_THROW(duplicate = !client_handler.tryLock(sol)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -158,7 +158,7 @@ TEST_F(ClientHandleTest, sharedQueries) { ClientHandler client_handler2; // Try to lock it with a request. - EXPECT_NO_THROW(duplicate = client_handler2.tryLock(req)); + EXPECT_NO_THROW(duplicate = !client_handler2.tryLock(req)); // Should return true (race with the duplicate). EXPECT_TRUE(duplicate); @@ -184,7 +184,7 @@ TEST_F(ClientHandleTest, sequence) { // Try to lock it with the solicit. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler.tryLock(sol)); + EXPECT_NO_THROW(duplicate = !client_handler.tryLock(sol)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -200,7 +200,7 @@ TEST_F(ClientHandleTest, sequence) { // Try to lock it with a request. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler2.tryLock(req)); + EXPECT_NO_THROW(duplicate = !client_handler2.tryLock(req)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -227,7 +227,7 @@ TEST_F(ClientHandleTest, notSharedQueries) { // Try to lock it with the solicit. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler.tryLock(sol)); + EXPECT_NO_THROW(duplicate = !client_handler.tryLock(sol)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -236,7 +236,7 @@ TEST_F(ClientHandleTest, notSharedQueries) { ClientHandler client_handler2; // Try to lock it with a request. - EXPECT_NO_THROW(duplicate = client_handler2.tryLock(req)); + EXPECT_NO_THROW(duplicate = !client_handler2.tryLock(req)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -259,7 +259,7 @@ TEST_F(ClientHandleTest, noClientId) { // Try to lock it with the solicit. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler.tryLock(sol)); + EXPECT_NO_THROW(duplicate = !client_handler.tryLock(sol)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -268,7 +268,7 @@ TEST_F(ClientHandleTest, noClientId) { ClientHandler client_handler2; // Try to lock it with a request. - EXPECT_NO_THROW(duplicate = client_handler2.tryLock(req)); + EXPECT_NO_THROW(duplicate = !client_handler2.tryLock(req)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -304,7 +304,7 @@ TEST_F(ClientHandleTest, doubleTryLock) { // Try to lock it. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler.tryLock(sol)); + EXPECT_NO_THROW(duplicate = !client_handler.tryLock(sol)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -342,7 +342,7 @@ TEST_F(ClientHandleTest, serializeTwoQueries) { // Try to lock it with the solicit. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler.tryLock(sol, cont1)); + EXPECT_NO_THROW(duplicate = !client_handler.tryLock(sol, cont1)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -355,10 +355,10 @@ TEST_F(ClientHandleTest, serializeTwoQueries) { makeContinuation(std::bind(&ClientHandleTest::setCalled2, this)); // Try to lock it with a request. - EXPECT_NO_THROW(duplicate = client_handler2.tryLock(req, cont2)); + EXPECT_NO_THROW(duplicate = !client_handler2.tryLock(req, cont2)); - // Should return false (multi-threading enforces serialization). - EXPECT_FALSE(duplicate); + // Should return true (race with the duplicate). + EXPECT_TRUE(duplicate); } catch (const std::exception& ex) { ADD_FAILURE() << "unexpected exception: " << ex.what(); } @@ -394,7 +394,7 @@ TEST_F(ClientHandleTest, serializeNoCont) { // Try to lock it with the solicit. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler.tryLock(sol)); + EXPECT_NO_THROW(duplicate = !client_handler.tryLock(sol)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -403,9 +403,9 @@ TEST_F(ClientHandleTest, serializeNoCont) { ClientHandler client_handler2; // Try to lock it with a request. - EXPECT_NO_THROW(duplicate = client_handler2.tryLock(req)); + EXPECT_NO_THROW(duplicate = !client_handler2.tryLock(req)); - // Should return true (duplicate without continuation). + // Should return true (race with the duplicate). EXPECT_TRUE(duplicate); } catch (const std::exception& ex) { ADD_FAILURE() << "unexpected exception: " << ex.what(); @@ -448,7 +448,7 @@ TEST_F(ClientHandleTest, serializeThreeQueries) { // Try to lock it with the solicit. bool duplicate = false; - EXPECT_NO_THROW(duplicate = client_handler.tryLock(sol, cont1)); + EXPECT_NO_THROW(duplicate = !client_handler.tryLock(sol, cont1)); // Should return false (no duplicate). EXPECT_FALSE(duplicate); @@ -461,10 +461,10 @@ TEST_F(ClientHandleTest, serializeThreeQueries) { makeContinuation(std::bind(&ClientHandleTest::setCalled2, this)); // Try to lock it with a request. - EXPECT_NO_THROW(duplicate = client_handler2.tryLock(req, cont2)); + EXPECT_NO_THROW(duplicate = !client_handler2.tryLock(req, cont2)); - // Should return false (multi-threading enforces serialization). - EXPECT_FALSE(duplicate); + // Should return true (race with the duplicate). + EXPECT_TRUE(duplicate); // Get a third client handler. ClientHandler client_handler3; @@ -474,10 +474,10 @@ TEST_F(ClientHandleTest, serializeThreeQueries) { makeContinuation(std::bind(&ClientHandleTest::setCalled3, this)); // Try to lock it with a renew. - EXPECT_NO_THROW(duplicate = client_handler3.tryLock(ren, cont3)); + EXPECT_NO_THROW(duplicate = !client_handler3.tryLock(ren, cont3)); - // Should return false (multi-threading enforces serialization). - EXPECT_FALSE(duplicate); + // Should return true (race with the duplicate). + EXPECT_TRUE(duplicate); } catch (const std::exception& ex) { ADD_FAILURE() << "unexpected exception: " << ex.what(); } diff --git a/src/lib/dhcpsrv/resource_handler.cc b/src/lib/dhcpsrv/resource_handler.cc new file mode 100644 index 0000000000..cbd1c099b8 --- /dev/null +++ b/src/lib/dhcpsrv/resource_handler.cc @@ -0,0 +1,100 @@ +// Copyright (C) 2020 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 +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +#include + +#include +#include + +using namespace std; +using namespace isc::asiolink; +using namespace isc::util; + +namespace isc { +namespace dhcp { + +mutex ResourceHandler::mutex_; + +ResourceHandler::ResourceContainer ResourceHandler::resources_; + +ResourceHandler::ResourceHandler() : owned_() { +} + +ResourceHandler::~ResourceHandler() { + for (auto res : owned_) { + lock_guard lock_(mutex_); + unLockInternal(res->type_, res->addr_); + } + owned_.clear(); +} + +ResourceHandler::ResourcePtr +ResourceHandler::lookup(Lease::Type type, const asiolink::IOAddress& addr) { + auto key = boost::make_tuple(type, addr.toBytes()); + auto it = resources_.find(key); + if (it == resources_.end()) { + return (ResourcePtr()); + } + return (*it); +} + +void +ResourceHandler::lock(Lease::Type type, const asiolink::IOAddress& addr) { + ResourcePtr res(new Resource(type, addr)); + // Assume insert will never fail so not checking its result. + resources_.insert(res); + owned_.insert(res); +} + +void +ResourceHandler::unLockInternal(Lease::Type type, + const asiolink::IOAddress& addr) { + auto key = boost::make_tuple(type, addr.toBytes()); + auto it = resources_.find(key); + if (it == resources_.end()) { + return; + } + resources_.erase(it); +} + +bool +ResourceHandler::tryLock(Lease::Type type, const asiolink::IOAddress& addr) { + ResourcePtr holder; + // Try to acquire the lock and return the holder when it failed. + lock_guard lock_(mutex_); + holder = lookup(type, addr); + if (holder) { + return (false); + } + lock(type, addr); + return (true); +} + +bool +ResourceHandler::isLocked(Lease::Type type, const asiolink::IOAddress& addr) { + auto key = boost::make_tuple(type, addr.toBytes()); + auto it = owned_.find(key); + return (it != owned_.end()); +} + +void +ResourceHandler::unLock(Lease::Type type, const asiolink::IOAddress& addr) { + auto key = boost::make_tuple(type, addr.toBytes()); + auto it = owned_.find(key); + if (it == owned_.end()) { + isc_throw(InvalidParameter, + "does not owne " << Lease::typeToText(type) << " " + << addr.toText()); + } + { + lock_guard lock_(mutex_); + unLockInternal(type, addr); + } + owned_.erase(it); +} + +} // namespace dhcp +} // namespace isc diff --git a/src/lib/dhcpsrv/resource_handler.h b/src/lib/dhcpsrv/resource_handler.h new file mode 100644 index 0000000000..6430d6101d --- /dev/null +++ b/src/lib/dhcpsrv/resource_handler.h @@ -0,0 +1,214 @@ +// Copyright (C) 2020 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 +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +#ifndef RESOURCE_HANDLER_H +#define RESOURCE_HANDLER_H + +#include +#include + +#include +#include +#include +#include +#include +#include +#include + +#include + +namespace isc { +namespace dhcp { + +/// @brief Resource race avoidance RAII handler. +class ResourceHandler : public boost::noncopyable { +public: + + /// @brief Constructor. + ResourceHandler(); + + /// @brief Destructor. + /// + /// Releases owned resources. + virtual ~ResourceHandler(); + + /// @brief Tries to acquires a resource. + /// + /// Lookup the resource, if not found insert the resource + /// in the resource container and return true, if found return false + /// + /// @param type Type of the resource, member of @c Lease::Type enum. + /// @param addr The address or prefix aka the resource. + /// @return true if the resource was acquired, false if the resource is + /// busy i.e. owned by a handler. + bool tryLock(Lease::Type type, const asiolink::IOAddress& addr); + + /// @brief Checks if a resource is owned by this handler. + /// + /// @param type Type of the resource, member of @c Lease::Type enum. + /// @param addr The address or prefix aka the resource. + /// @return true if this handler owns the resource, false otherwise. + bool isLocked(Lease::Type type, const asiolink::IOAddress& addr); + + /// @brief Releases a resource. + /// + /// Remove the resource from the resource container. + /// + /// @param type Type of the resource, member of @c Lease::Type enum. + /// @param addr The address or prefix aka the resource. + /// @throw when we do not own the resource. + void unLock(Lease::Type type, const asiolink::IOAddress& addr); + +private: + + /// Type definitions. + //@{ + + /// @brief Structure representing a resource. + struct Resource { + + /// @brief Constructor. + /// + /// @param addr The address or prefix aka the resource.. + Resource(Lease::Type type, const asiolink::IOAddress& addr) + : type_(type), addr_(addr) { + } + + /// @brief The type. + Lease::Type type_; + + /// @brief The resource. + asiolink::IOAddress addr_; + + /// @brief The key extractor. + std::vector toBytes() const { + return (addr_.toBytes()); + } + }; + + /// @brief The type of shared pointers to resources. + typedef boost::shared_ptr ResourcePtr; + + /// @brief The type of the resource container. + typedef boost::multi_index_container< + + // This container stores pointers to resource objects. + ResourcePtr, + + // Start specification of indexes here. + boost::multi_index::indexed_by< + + // First index is used to search by type and address. + boost::multi_index::hashed_unique< + boost::multi_index::composite_key< + Resource, + // Lease type. + boost::multi_index::member< + Resource, Lease::Type, &Resource::type_ + >, + // Address bytes. + boost::multi_index::const_mem_fun< + Resource, std::vector, &Resource::toBytes + > + > + > + > + > ResourceContainer; + + //@} + + /// Class members. + //@{ + + /// @brief The resource container. + static ResourceContainer resources_; + + /// @brief Mutex to protect the resource container. + static std::mutex mutex_; + + /// @brief Lookup a resource. + /// + /// The mutex must be held by the caller. + /// + /// @param type Type of the resource, member of @c Lease::Type enum. + /// @param addr The address or prefix aka the resource. + /// @return The busy resource or null. + static ResourcePtr + lookup(Lease::Type type, const asiolink::IOAddress& addr); + + //@} + + /// Instance members. + //@{ + + /// @brief Acquire a resource. + /// + /// The mutex must be held by the caller. + /// + /// @param type Type of the resource, member of @c Lease::Type enum. + /// @param addr The address or prefix aka the resource. + void lock(Lease::Type type, const asiolink::IOAddress& addr); + + /// @brief Release a resource. + /// + /// The mutex must be held by the caller. + /// + /// Remove the resource from the resource container. + /// + /// @param type Type of the resource, member of @c Lease::Type enum. + /// @param addr The address or prefix aka the resource. + void unLockInternal(Lease::Type type, const asiolink::IOAddress& addr); + + /// @brief List of resources this handler owns. + ResourceContainer owned_; + + //@} +}; + +/// @brief Resource race avoidance RAII handler for DHCPv4. +class ResourceHandler4 : public ResourceHandler { +public: + + /// @brief Destructor. + /// + /// Releases owned resources. + virtual ~ResourceHandler4() { } + + /// @brief Tries to acquires a resource. + /// + /// Lookup the resource, if not found insert the resource + /// in the resource container and return true, if found return false + /// + /// @param addr The address aka the resource. + /// @return true if the resource was acquired, false if the resource is + /// busy i.e. owned by a handler. + bool tryLock4(const asiolink::IOAddress& addr) { + return (tryLock(Lease::TYPE_V4, addr)); + } + + /// @brief Checks if a resource is owned by this handler. + /// + /// @param addr The address aka the resource. + /// @return true if this handler owns the resource, false otherwise. + bool isLocked4(const asiolink::IOAddress& addr) { + return (isLocked(Lease::TYPE_V4, addr)); + } + + /// @brief Releases a resource. + /// + /// Remove the resource from the resource container. + /// + /// @param addr The address aka the resource. + /// @throw when we do not own the resource. + void unLock4(const asiolink::IOAddress& addr) { + unLock(Lease::TYPE_V4, addr); + } +}; + +} // namespace isc +} // namespace dhcp + +#endif // RESOURCE_HANDLER_H diff --git a/src/lib/dhcpsrv/tests/resource_handler_unittest.cc b/src/lib/dhcpsrv/tests/resource_handler_unittest.cc new file mode 100644 index 0000000000..1fe6e4c1c0 --- /dev/null +++ b/src/lib/dhcpsrv/tests/resource_handler_unittest.cc @@ -0,0 +1,36 @@ +// Copyright (C) 2020 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 +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +#include +#include +#include + +using namespace isc; +using namespace isc::dhcp; + +namespace { + +// Verifies behavior with empty block. +TEST(ResourceHandleTest, empty) { + try { + // Get a resource handler. + ResourceHandler resource_handler; + } catch (const std::exception& ex) { + ADD_FAILURE() << "unexpected exception: " << ex.what(); + } +} + +// Verifies behavior with empty block (v4). +TEST(ResourceHandleTest, empty4) { + try { + // Get a resource handler. + ResourceHandler4 resource_handler; + } catch (const std::exception& ex) { + ADD_FAILURE() << "unexpected exception: " << ex.what(); + } +} + +} // end of anonymous namespace