From 0df2dec81451e55b89d17129f75a64e1951a0bb2 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Mon, 20 Nov 2023 12:47:19 -0500 Subject: [PATCH] [#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 --- src/bin/dhcp4/dhcp4_messages.cc | 6 ++++ src/bin/dhcp4/dhcp4_messages.h | 3 ++ src/bin/dhcp4/dhcp4_messages.mes | 29 ++++++++++++++--- src/bin/dhcp4/dhcp4_srv.cc | 53 +++++++++++++++++++++++--------- src/bin/dhcp4/dhcp4_srv.h | 4 +-- 5 files changed, 74 insertions(+), 21 deletions(-) diff --git a/src/bin/dhcp4/dhcp4_messages.cc b/src/bin/dhcp4/dhcp4_messages.cc index de44d9f591..5e7f9bf8c1 100644 --- 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 4dd3797350..a5baf1e75f 100644 --- 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 fdf6b95259..947eaeb866 100644 --- 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 7b05c043a1..bbf7e9dd2d 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -37,6 +37,7 @@ #include #include #include +#include #include #include #include @@ -52,6 +53,7 @@ #include #include + #ifdef HAVE_MYSQL #include #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 { - LeaseMgrFactory::instance().addLease(lease); + } 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 52346e8dec..97e9d7ba52 100644 --- 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. ///