[#3084] More review comments

src/bin/dhcp4/dhcp4_messages.h b/src/bin/dhcp4/dhcp4_messages.*
    New messages
    DHCP4_SERVER_INITIATED_DECLINE_UPDATE_FAILED
    DHCP4_SERVER_INITIATED_DECLINE_ADD_FAILED
    DHCP4_SERVER_INITIATED_DECLINE_RESOURCE_BUSY

src/bin/dhcp4/dhcp4_srv.*
    Dhcpv4Srv::sendResponseNoThrow() - restored argument passing by ref
    Dhcpv4Srv::serverDecline() - use ResourceHandle, try add if update fails

diff --git a/src/bin/dhcp4/dhcp4_messages.cc b/src/bin/dhcp4/dhcp4_messages.cc
index de44d9f591ce6febb9bc2d76ee7936fdc97e1f97..5e7f9bf8c1535789b7b31d10222688bd0d5a6336 100644 (file)
--- a/src/bin/dhcp4/dhcp4_messages.cc
+++ b/src/bin/dhcp4/dhcp4_messages.cc
@@ -164,7 +164,10 @@ extern const isc::log::MessageID DHCP4_RESPONSE_HOSTNAME_DATA = "DHCP4_RESPONSE_
 extern const isc::log::MessageID DHCP4_RESPONSE_HOSTNAME_GENERATE = "DHCP4_RESPONSE_HOSTNAME_GENERATE";
 extern const isc::log::MessageID DHCP4_SERVER_FAILED = "DHCP4_SERVER_FAILED";
 extern const isc::log::MessageID DHCP4_SERVER_INITIATED_DECLINE = "DHCP4_SERVER_INITIATED_DECLINE";
+extern const isc::log::MessageID DHCP4_SERVER_INITIATED_DECLINE_ADD_FAILED = "DHCP4_SERVER_INITIATED_DECLINE_ADD_FAILED";
 extern const isc::log::MessageID DHCP4_SERVER_INITIATED_DECLINE_FAILED = "DHCP4_SERVER_INITIATED_DECLINE_FAILED";
+extern const isc::log::MessageID DHCP4_SERVER_INITIATED_DECLINE_RESOURCE_BUSY = "DHCP4_SERVER_INITIATED_DECLINE_RESOURCE_BUSY";
+extern const isc::log::MessageID DHCP4_SERVER_INITIATED_DECLINE_UPDATE_FAILED = "DHCP4_SERVER_INITIATED_DECLINE_UPDATE_FAILED";
 extern const isc::log::MessageID DHCP4_SHUTDOWN = "DHCP4_SHUTDOWN";
 extern const isc::log::MessageID DHCP4_SHUTDOWN_REQUEST = "DHCP4_SHUTDOWN_REQUEST";
 extern const isc::log::MessageID DHCP4_SRV_CONSTRUCT_ERROR = "DHCP4_SRV_CONSTRUCT_ERROR";
@@ -345,7 +348,10 @@ const char* values[] = {
     "DHCP4_RESPONSE_HOSTNAME_GENERATE", "%1: server has generated hostname %2 for the client",
     "DHCP4_SERVER_FAILED", "server failed: %1",
     "DHCP4_SERVER_INITIATED_DECLINE", "Lease for addr %1 has been found to be already in use. The lease will be unavailable for %2 seconds.",
+    "DHCP4_SERVER_INITIATED_DECLINE_ADD_FAILED", "%1: error adding a lease for address %2",
     "DHCP4_SERVER_INITIATED_DECLINE_FAILED", "%1: error on server-initiated decline lease for address %2: %3",
+    "DHCP4_SERVER_INITIATED_DECLINE_RESOURCE_BUSY", "%1: error declining a lease for address %2",
+    "DHCP4_SERVER_INITIATED_DECLINE_UPDATE_FAILED", "%1: error updating lease for address %2",
     "DHCP4_SHUTDOWN", "server shutdown",
     "DHCP4_SHUTDOWN_REQUEST", "shutdown of server requested",
     "DHCP4_SRV_CONSTRUCT_ERROR", "error creating Dhcpv4Srv object, reason: %1",

diff --git a/src/bin/dhcp4/dhcp4_messages.h b/src/bin/dhcp4/dhcp4_messages.h
index 4dd3797350836288420da59761bc4494dd79cd5e..a5baf1e75f9b5cf20e08db9ecb354f8c3abad180 100644 (file)
--- a/src/bin/dhcp4/dhcp4_messages.h
+++ b/src/bin/dhcp4/dhcp4_messages.h
@@ -165,7 +165,10 @@ extern const isc::log::MessageID DHCP4_RESPONSE_HOSTNAME_DATA;
 extern const isc::log::MessageID DHCP4_RESPONSE_HOSTNAME_GENERATE;
 extern const isc::log::MessageID DHCP4_SERVER_FAILED;
 extern const isc::log::MessageID DHCP4_SERVER_INITIATED_DECLINE;
+extern const isc::log::MessageID DHCP4_SERVER_INITIATED_DECLINE_ADD_FAILED;
 extern const isc::log::MessageID DHCP4_SERVER_INITIATED_DECLINE_FAILED;
+extern const isc::log::MessageID DHCP4_SERVER_INITIATED_DECLINE_RESOURCE_BUSY;
+extern const isc::log::MessageID DHCP4_SERVER_INITIATED_DECLINE_UPDATE_FAILED;
 extern const isc::log::MessageID DHCP4_SHUTDOWN;
 extern const isc::log::MessageID DHCP4_SHUTDOWN_REQUEST;
 extern const isc::log::MessageID DHCP4_SRV_CONSTRUCT_ERROR;

diff --git a/src/bin/dhcp4/dhcp4_messages.mes b/src/bin/dhcp4/dhcp4_messages.mes
index fdf6b9525960180d9561f8976a16a60a4d4df08b..947eaeb86696c43f486d4cedb4455399ae07d8ec 100644 (file)
--- a/src/bin/dhcp4/dhcp4_messages.mes
+++ b/src/bin/dhcp4/dhcp4_messages.mes
@@ -1065,6 +1065,12 @@ identification information. The second argument holds the IPv4 address
 for which the decline was attempted. The last one contains the reason of
 failure.
 
+% DHCP4_HOOK_LEASE4_OFFER_ARGUMENT_MISSING hook callouts did not set an argument as expected %1 for %2
+This error message is printed when none of the callouts installed on the
+lease4_offer hook point set an expected argument in the callout status.
+This is a programming error in the installed hook libraries.  Details of
+the argument and the query in process at the time are provided log arguments.
+
 % DHCP4_SERVER_INITIATED_DECLINE Lease for addr %1 has been found to be already in use. The lease will be unavailable for %2 seconds.
 This informational message is printed when the server has detected via
 ICMP ECHO (i.e. ping check) or other means that a lease which should be
@@ -1074,8 +1080,21 @@ supposed to use. The server will fully recover from this situation, but if
 the underlying problem of a misconfigured or rogue device is not solved, this
 address may be declined again in the future.
 
-% DHCP4_HOOK_LEASE4_OFFER_ARGUMENT_MISSING hook callouts did not set an argument as expected %1 for %2
-This error message is printed when none of the callouts installed on the
-lease4_offer hook point set an expected argument in the callout status.
-This is a programming error in the installed hook libraries.  Details of
-the argument and the query in process at the time are provided log arguments.
+% DHCP4_SERVER_INITIATED_DECLINE_UPDATE_FAILED %1: error updating lease for address %2
+This error message indicates that the server failed to update a lease in the
+lease store to the DECLINED state  The first argument includes the client and
+the transaction identification information. The second argument holds the IPv4
+address for which the decline was attempted.
+
+% DHCP4_SERVER_INITIATED_DECLINE_ADD_FAILED %1: error adding a lease for address %2
+This error message indicates that the server failed to add a DECLINED lease to
+the lease store  The first argument includes the client and the transaction
+identification information. The second argument holds the IPv4 address for which
+the decline was attempted.
+
+% DHCP4_SERVER_INITIATED_DECLINE_RESOURCE_BUSY %1: error declining a lease for address %2
+This error message indicates that the while one server thread was attempting
+to mark a lease as DECLINED, it was already locked by another thread.
+The first argument includes the client and the transaction identification
+information.  The second argument holds the IPv4 address for which the decline
+was attempted.

diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc
index 7b05c043a16f821ab9ceb3655e9e820a72f092da..bbf7e9dd2d448203ea3097296acbcec9e62ccf65 100644 (file)
--- a/src/bin/dhcp4/dhcp4_srv.cc
+++ b/src/bin/dhcp4/dhcp4_srv.cc
@@ -37,6 +37,7 @@
 #include <dhcpsrv/lease_mgr.h>
 #include <dhcpsrv/lease_mgr_factory.h>
 #include <dhcpsrv/ncr_generator.h>
+#include <dhcpsrv/resource_handler.h>
 #include <dhcpsrv/shared_network.h>
 #include <dhcpsrv/subnet.h>
 #include <dhcpsrv/subnet_selector.h>
@@ -52,6 +53,7 @@
 #include <cryptolink/cryptolink.h>
 #include <process/cfgrpt/config_report.h>
 
+
 #ifdef HAVE_MYSQL
 #include <dhcpsrv/mysql_lease_mgr.h>
 #endif
@@ -1599,8 +1601,8 @@ Dhcpv4Srv::processDhcp4Query(Pkt4Ptr& query, Pkt4Ptr& rsp,
 }
 
 void
-Dhcpv4Srv::sendResponseNoThrow(hooks::CalloutHandlePtr callout_handle,
-                               Pkt4Ptr query, Pkt4Ptr rsp) {
+Dhcpv4Srv::sendResponseNoThrow(hooks::CalloutHandlePtr& callout_handle,
+                               Pkt4Ptr& query, Pkt4Ptr& rsp) {
     try {
             processPacketPktSend(callout_handle, query, rsp);
             processPacketBufferSend(callout_handle, rsp);
@@ -3948,25 +3950,48 @@ Dhcpv4Srv::declineLease(const Lease4Ptr& lease, const Pkt4Ptr& decline,
 void
 Dhcpv4Srv::serverDecline(hooks::CalloutHandlePtr& /* callout_handle */, Pkt4Ptr& query,
                          Lease4Ptr lease, bool lease_exists) {
+
+    // Check if the resource is busy i.e. can be modified by another thread
+    // for another client. Highly unlikely.
+    ResourceHandler4 resource_handler;
+    if (MultiThreadingMgr::instance().getMode() && !resource_handler.tryLock4(lease->addr_)) {
+        LOG_ERROR(lease4_logger, DHCP4_SERVER_INITIATED_DECLINE_RESOURCE_BUSY)
+            .arg(query->getLabel())
+            .arg(lease->addr_.toText());
+        return;
+    }
+
     // We need to disassociate the lease from the client. Once we move a lease
     // to declined state, it is no longer associated with the client in any
     // way.
     lease->decline(CfgMgr::instance().getCurrentCfg()->getDeclinePeriod());
 
-    // Add or update the lease in the database.
-    try {
-        if (lease_exists) {
+    // If the lease already exists, update it in the database.
+    if (lease_exists) {
+        try {
             LeaseMgrFactory::instance().updateLease4(lease);
-        } else {
+        } catch (const NoSuchLease& ex) {
+            // We expected the lease to exist but it doesn't so let's add
+            // try to add it.
+            lease_exists = false;
+        } catch (const Exception& ex) {
+            // Update failed.
+            LOG_ERROR(lease4_logger, DHCP4_SERVER_INITIATED_DECLINE_UPDATE_FAILED)
+                .arg(query->getLabel())
+                .arg(lease->addr_.toText());
+            return;
+        }
+    }
+
+    if (!lease_exists) {
+        try {
             LeaseMgrFactory::instance().addLease(lease);
+        } catch (const Exception& ex) {
+            LOG_ERROR(lease4_logger, DHCP4_SERVER_INITIATED_DECLINE_ADD_FAILED)
+                .arg(query->getLabel())
+                .arg(lease->addr_.toText());
+            return;
         }
-    } catch (const Exception& ex) {
-        // Update failed.
-        LOG_ERROR(lease4_logger, DHCP4_SERVER_INITIATED_DECLINE_FAILED)
-            .arg(query->getLabel())
-            .arg(lease->addr_.toText())
-            .arg(ex.what());
-        return;
     }
 
     // Bump up the statistics.  If the lease does not exist (i.e. offer-lifetime == 0) we

diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h
index 52346e8decbdcb4878b8fa41715573c71900e4fb..97e9d7ba5292746252f86af86cad5fe17741a7f3 100644 (file)
--- a/src/bin/dhcp4/dhcp4_srv.h
+++ b/src/bin/dhcp4/dhcp4_srv.h
@@ -359,8 +359,8 @@ public:
     /// @param callout_handle pointer to the callout handle.
     /// @param query A pointer to the packet to be processed.
     /// @param rsp A pointer to the response.
-    void sendResponseNoThrow(hooks::CalloutHandlePtr callout_handle,
-                             Pkt4Ptr query, Pkt4Ptr rsp);
+    void sendResponseNoThrow(hooks::CalloutHandlePtr& callout_handle,
+                             Pkt4Ptr& query, Pkt4Ptr& rsp);
 
     /// @brief Process a single incoming DHCPv4 packet.
     ///