[#2022] Checkpoint: began reorg

5 files changed, 126 insertions, 83 deletions

diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc
index c0ae4630ce..3087cbfdcc 100644
--- a/src/bin/dhcp4/dhcp4_srv.cc
+++ b/src/bin/dhcp4/dhcp4_srv.cc
@@ -928,14 +928,22 @@ Dhcpv4Srv::sendPacket(const Pkt4Ptr& packet) {
     IfaceMgr::instance().send(packet);
 }
 
-bool
-Dhcpv4Srv::earlyGHRLookup(const Pkt4Ptr& query,
-                          AllocEngine::ClientContext4Ptr ctx) {
+void
+Dhcpv4Srv::initContext0(const Pkt4Ptr& query,
+                        AllocEngine::ClientContext4Ptr ctx) {
     // Pointer to client's query.
     ctx->query_ = query;
 
     // Hardware address.
     ctx->hwaddr_ = query->getHWAddr();
+}
+
+bool
+Dhcpv4Srv::earlyGHRLookup(const Pkt4Ptr& query,
+                          AllocEngine::ClientContext4Ptr ctx) {
+
+    // First part of context initialization.
+    initContext0(query, ctx);
 
     // Get the early-global-reservations-lookup flag value.
     data::ConstElementPtr egrl = CfgMgr::instance().getCurrentCfg()->
@@ -1115,7 +1123,7 @@ Dhcpv4Srv::runOne() {
 }
 
 void
-Dhcpv4Srv::processPacketAndSendResponseNoThrow(Pkt4Ptr& query) {
+Dhcpv4Srv::processPacketAndSendResponseNoThrow(Pkt4Ptr query) {
     try {
         processPacketAndSendResponse(query);
     } catch (const std::exception& e) {
@@ -1127,9 +1135,8 @@ Dhcpv4Srv::processPacketAndSendResponseNoThrow(Pkt4Ptr& query) {
 }
 
 void
-Dhcpv4Srv::processPacketAndSendResponse(Pkt4Ptr& query) {
-    Pkt4Ptr rsp;
-    processPacket(query, rsp);
+Dhcpv4Srv::processPacketAndSendResponse(Pkt4Ptr query) {
+    Pkt4Ptr rsp = processPacket(query);
     if (!rsp) {
         return;
     }
@@ -1139,8 +1146,8 @@ Dhcpv4Srv::processPacketAndSendResponse(Pkt4Ptr& query) {
     processPacketBufferSend(callout_handle, rsp);
 }
 
-void
-Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp, bool allow_packet_park) {
+Pkt4Ptr
+Dhcpv4Srv::processPacket(Pkt4Ptr query, bool allow_packet_park) {
     query->addPktEvent("process_started");
 
     // All packets belong to ALL.
@@ -1185,7 +1192,7 @@ Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp, bool allow_packet_park) {
                 .arg(query->getRemoteAddr().toText())
                 .arg(query->getLocalAddr().toText())
                 .arg(query->getIface());
-            return;
+            return (Pkt4Ptr());;
         }
 
         // Callouts decided to skip the next processing step. The next
@@ -1233,7 +1240,7 @@ Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp, bool allow_packet_park) {
                                                       static_cast<int64_t>(1));
             isc::stats::StatsMgr::instance().addValue("pkt4-receive-drop",
                                                       static_cast<int64_t>(1));
-            return;
+            return (Pkt4Ptr());
         }
     }
 
@@ -1258,7 +1265,7 @@ Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp, bool allow_packet_park) {
         // Increase the statistic of dropped packets.
         isc::stats::StatsMgr::instance().addValue("pkt4-receive-drop",
                                                   static_cast<int64_t>(1));
-        return;
+        return (Pkt4Ptr());
     }
 
     // We have sanity checked (in accept() that the Message Type option
@@ -1303,7 +1310,7 @@ Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp, bool allow_packet_park) {
             LOG_DEBUG(hooks_logger, DBG_DHCP4_HOOKS,
                       DHCP4_HOOK_PACKET_RCVD_SKIP)
                 .arg(query->getLabel());
-            return;
+            return (Pkt4Ptr());
         }
 
         callout_handle->getArgument("query4", query);
@@ -1316,17 +1323,17 @@ Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp, bool allow_packet_park) {
             .arg(query->toText());
         isc::stats::StatsMgr::instance().addValue("pkt4-receive-drop",
                                                   static_cast<int64_t>(1));
-        return;
+        return (Pkt4Ptr());
     }
 
-    processDhcp4Query(query, rsp, allow_packet_park);
+    return (processDhcp4Query(query, allow_packet_park));
 }
 
 void
-Dhcpv4Srv::processDhcp4QueryAndSendResponse(Pkt4Ptr& query, Pkt4Ptr& rsp,
+Dhcpv4Srv::processDhcp4QueryAndSendResponse(Pkt4Ptr query,
                                             bool allow_packet_park) {
     try {
-        processDhcp4Query(query, rsp, allow_packet_park);
+        Pkt4Ptr rsp = processDhcp4Query(query, allow_packet_park);
         if (!rsp) {
             return;
         }
@@ -1341,9 +1348,8 @@ Dhcpv4Srv::processDhcp4QueryAndSendResponse(Pkt4Ptr& query, Pkt4Ptr& rsp,
     }
 }
 
-void
-Dhcpv4Srv::processDhcp4Query(Pkt4Ptr& query, Pkt4Ptr& rsp,
-                             bool allow_packet_park) {
+Pkt4Ptr
+Dhcpv4Srv::processDhcp4Query(Pkt4Ptr query, bool allow_packet_park) {
     // Create a client race avoidance RAII handler.
     ClientHandler client_handler;
 
@@ -1355,18 +1361,30 @@ Dhcpv4Srv::processDhcp4Query(Pkt4Ptr& query, Pkt4Ptr& rsp,
          (query->getType() == DHCPDECLINE))) {
         ContinuationPtr cont =
             makeContinuation(std::bind(&Dhcpv4Srv::processDhcp4QueryAndSendResponse,
-                                       this, query, rsp, allow_packet_park));
+                                       this, query, allow_packet_park));
         if (!client_handler.tryLock(query, cont)) {
-            return;
+            return (Pkt4Ptr());
         }
     }
 
     AllocEngine::ClientContext4Ptr ctx(new AllocEngine::ClientContext4());
     if (!earlyGHRLookup(query, ctx)) {
-        return;
+        return (Pkt4Ptr());
     }
 
+    Pkt4Ptr rsp;
     try {
+        sanityCheck(query);
+        if ((query->getType() == DHCPDISCOVER) ||
+            (query->getType() == DHCPREQUEST) ||
+            (query->getType() == DHCPINFORM)) {
+            bool drop = false;
+            ctx->subnet_ = selectSubnet(query, drop);
+            // Stop here if selectSubnet decided to drop the packet
+            if (drop) {
+                return (Pkt4Ptr());
+            }
+        }
         switch (query->getType()) {
         case DHCPDISCOVER:
             rsp = processDiscover(query, ctx);
@@ -1506,8 +1524,7 @@ Dhcpv4Srv::processDhcp4Query(Pkt4Ptr& query, Pkt4Ptr& rsp,
                             .arg(query->getLabel());
                         isc::stats::StatsMgr::instance().addValue("pkt4-receive-drop",
                                                                   static_cast<int64_t>(1));
-                        rsp.reset();
-                        return;
+                        return (Pkt4Ptr());
                     }
                 }
 
@@ -1603,6 +1620,7 @@ Dhcpv4Srv::processDhcp4Query(Pkt4Ptr& query, Pkt4Ptr& rsp,
         Subnet4Ptr subnet = (ctx ? ctx->subnet_ : Subnet4Ptr());
         processPacketPktSend(callout_handle, query, rsp, subnet);
     }
+    return (rsp);
 }
 
 void
@@ -3471,18 +3489,8 @@ Dhcpv4Srv::getNetmaskOption(const Subnet4Ptr& subnet) {
 
 Pkt4Ptr
 Dhcpv4Srv::processDiscover(Pkt4Ptr& discover, AllocEngine::ClientContext4Ptr& context) {
-    // server-id is forbidden.
-    sanityCheck(discover, FORBIDDEN);
-
     bool drop = false;
-    Subnet4Ptr subnet = selectSubnet(discover, drop);
-
-    // Stop here if selectSubnet decided to drop the packet
-    if (drop) {
-        return (Pkt4Ptr());
-    }
-
-    Dhcpv4Exchange ex(alloc_engine_, discover, context, subnet, drop);
+    Dhcpv4Exchange ex(alloc_engine_, discover, context, context->subnet_, drop);
 
     // Stop here if Dhcpv4Exchange constructor decided to drop the packet
     if (drop) {
@@ -3550,19 +3558,8 @@ Dhcpv4Srv::processDiscover(Pkt4Ptr& discover, AllocEngine::ClientContext4Ptr& co
 
 Pkt4Ptr
 Dhcpv4Srv::processRequest(Pkt4Ptr& request, AllocEngine::ClientContext4Ptr& context) {
-    // Since we cannot distinguish  between client states
-    // we'll make server-id is optional for REQUESTs.
-    sanityCheck(request, OPTIONAL);
-
     bool drop = false;
-    Subnet4Ptr subnet = selectSubnet(request, drop);
-
-    // Stop here if selectSubnet decided to drop the packet
-    if (drop) {
-        return (Pkt4Ptr());
-    }
-
-    Dhcpv4Exchange ex(alloc_engine_, request, context, subnet, drop);
+    Dhcpv4Exchange ex(alloc_engine_, request, context, context->subnet_, drop);
 
     // Stop here if Dhcpv4Exchange constructor decided to drop the packet
     if (drop) {
@@ -3632,10 +3629,6 @@ Dhcpv4Srv::processRequest(Pkt4Ptr& request, AllocEngine::ClientContext4Ptr& cont
 
 void
 Dhcpv4Srv::processRelease(Pkt4Ptr& release, AllocEngine::ClientContext4Ptr& context) {
-    // Server-id is mandatory in DHCPRELEASE (see table 5, RFC2131)
-    // but ISC DHCP does not enforce this, so we'll follow suit.
-    sanityCheck(release, OPTIONAL);
-
     // Try to find client-id. Note that for the DHCPRELEASE we don't check if the
     // match-client-id configuration parameter is disabled because this parameter
     // is configured for subnets and we don't select subnet for the DHCPRELEASE.
@@ -3783,10 +3776,6 @@ Dhcpv4Srv::processRelease(Pkt4Ptr& release, AllocEngine::ClientContext4Ptr& cont
 
 void
 Dhcpv4Srv::processDecline(Pkt4Ptr& decline, AllocEngine::ClientContext4Ptr& context) {
-    // Server-id is mandatory in DHCPDECLINE (see table 5, RFC2131)
-    // but ISC DHCP does not enforce this, so we'll follow suit.
-    sanityCheck(decline, OPTIONAL);
-
     // Client is supposed to specify the address being declined in
     // Requested IP address option, but must not set its ciaddr.
     // (again, see table 5 in RFC2131).
@@ -4081,19 +4070,8 @@ Dhcpv4Srv::serverDeclineNoThrow(hooks::CalloutHandlePtr& callout_handle, Pkt4Ptr
 
 Pkt4Ptr
 Dhcpv4Srv::processInform(Pkt4Ptr& inform, AllocEngine::ClientContext4Ptr& context) {
-    // server-id is supposed to be forbidden (as is requested address)
-    // but ISC DHCP does not enforce either. So neither will we.
-    sanityCheck(inform, OPTIONAL);
-
     bool drop = false;
-    Subnet4Ptr subnet = selectSubnet(inform, drop);
-
-    // Stop here if selectSubnet decided to drop the packet
-    if (drop) {
-        return (Pkt4Ptr());
-    }
-
-    Dhcpv4Exchange ex(alloc_engine_, inform, context, subnet, drop);
+    Dhcpv4Exchange ex(alloc_engine_, inform, context, context->subnet_, drop);
 
     // Stop here if Dhcpv4Exchange constructor decided to drop the packet
     if (drop) {
@@ -4399,6 +4377,36 @@ Dhcpv4Srv::acceptServerId(const Pkt4Ptr& query) const {
 }
 
 void
+Dhcpv4Srv::sanityCheck(const Pkt4Ptr& query) {
+    switch (query->getType()) {
+    case DHCPDISCOVER:
+        // server-id is forbidden.
+        sanityCheck(query, FORBIDDEN);
+        break;
+    case DHCPREQUEST:
+        // Since we cannot distinguish  between client states
+        // we'll make server-id is optional for REQUESTs.
+        sanityCheck(query, OPTIONAL);
+        break;
+    case DHCPRELEASE:
+        // Server-id is mandatory in DHCPRELEASE (see table 5, RFC2131)
+        // but ISC DHCP does not enforce this, so we'll follow suit.
+        sanityCheck(query, OPTIONAL);
+        break;
+    case DHCPDECLINE:
+        // Server-id is mandatory in DHCPDECLINE (see table 5, RFC2131)
+        // but ISC DHCP does not enforce this, so we'll follow suit.
+        sanityCheck(query, OPTIONAL);
+        break;
+    case DHCPINFORM:
+        // server-id is supposed to be forbidden (as is requested address)
+        // but ISC DHCP does not enforce either. So neither will we.
+        sanityCheck(query, OPTIONAL);
+        break;
+    }
+}
+
+void
 Dhcpv4Srv::sanityCheck(const Pkt4Ptr& query, RequirementLevel serverid) {
     OptionPtr server_id = query->getOption(DHO_DHCP_SERVER_IDENTIFIER);
     switch (serverid) {
diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h
index b2c100e769..b4e8f5a06c 100644
--- a/src/bin/dhcp4/dhcp4_srv.h
+++ b/src/bin/dhcp4/dhcp4_srv.h
@@ -1,4 +1,4 @@
-// Copyright (C) 2011-2023 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2024 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
@@ -344,7 +344,7 @@ public:
     /// methods, generates appropriate answer, sends the answer to the client.
     ///
     /// @param query A pointer to the packet to be processed.
-    void processPacketAndSendResponse(Pkt4Ptr& query);
+    void processPacketAndSendResponse(Pkt4Ptr query);
 
     /// @brief Process a single incoming DHCPv4 packet and sends the response.
     ///
@@ -352,7 +352,7 @@ public:
     /// methods, generates appropriate answer, sends the answer to the client.
     ///
     /// @param query A pointer to the packet to be processed.
-    void processPacketAndSendResponseNoThrow(Pkt4Ptr& query);
+    void processPacketAndSendResponseNoThrow(Pkt4Ptr query);
 
     /// @brief Process an unparked DHCPv4 packet and sends the response.
     ///
@@ -369,20 +369,18 @@ public:
     /// methods, generates appropriate answer.
     ///
     /// @param query A pointer to the packet to be processed.
-    /// @param rsp A pointer to the response.
     /// @param allow_packet_park Indicates if parking a packet is allowed.
-    void processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp,
-                       bool allow_packet_park = true);
+    /// @return A pointer to the response.
+    Pkt4Ptr processPacket(Pkt4Ptr query, bool allow_packet_park = true);
 
     /// @brief Process a single incoming DHCPv4 query.
     ///
     /// It calls per-type processXXX methods, generates appropriate answer.
     ///
     /// @param query A pointer to the packet to be processed.
-    /// @param rsp A pointer to the response.
     /// @param allow_packet_park Indicates if parking a packet is allowed.
-    void processDhcp4Query(Pkt4Ptr& query, Pkt4Ptr& rsp,
-                           bool allow_packet_park);
+    /// @return A pointer to the response.
+    Pkt4Ptr processDhcp4Query(Pkt4Ptr query, bool allow_packet_park);
 
     /// @brief Process a single incoming DHCPv4 query.
     ///
@@ -390,9 +388,8 @@ public:
     /// sends the answer to the client.
     ///
     /// @param query A pointer to the packet to be processed.
-    /// @param rsp A pointer to the response.
     /// @param allow_packet_park Indicates if parking a packet is allowed.
-    void processDhcp4QueryAndSendResponse(Pkt4Ptr& query, Pkt4Ptr& rsp,
+    void processDhcp4QueryAndSendResponse(Pkt4Ptr query,
                                           bool allow_packet_park);
 
     /// @brief Instructs the server to shut down.
@@ -466,6 +463,13 @@ public:
         return (test_send_responses_to_source_);
     }
 
+    /// @brief Initialize client context (first part).
+    ///
+    /// @param query The query message.
+    /// @param ctx Pointer to client context.
+    void initContext0(const Pkt4Ptr& query,
+                      AllocEngine::ClientContext4Ptr ctx);
+
     /// @brief Initialize client context and perform early global
     /// reservations lookup.
     ///
@@ -576,6 +580,17 @@ protected:
     ///
     /// Checks if mandatory option is really there, that forbidden option
     /// is not there, and that client-id or server-id appears only once.
+    /// Calls the second method with the requirement level from the
+    /// message type.
+    ///
+    /// @param query Pointer to the client's message.
+    /// @throw RFCViolation if any issues are detected
+    static void sanityCheck(const Pkt4Ptr& query);
+
+    /// @brief Verifies if specified packet meets RFC requirements
+    ///
+    /// Checks if mandatory option is really there, that forbidden option
+    /// is not there, and that client-id or server-id appears only once.
     ///
     /// @param query Pointer to the client's message.
     /// @param serverid expectation regarding server-id option
diff --git a/src/bin/dhcp4/dhcp4to6_ipc.cc b/src/bin/dhcp4/dhcp4to6_ipc.cc
index e3fbcf9abe..717552d100 100644
--- a/src/bin/dhcp4/dhcp4to6_ipc.cc
+++ b/src/bin/dhcp4/dhcp4to6_ipc.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2015-2022 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2015-2024 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
@@ -104,7 +104,7 @@ void Dhcp4to6Ipc::handler(int /* fd */) {
     // From Dhcpv4Srv::runOne() processing and after
     Pkt4Ptr rsp;
 
-    ControlledDhcpv4Srv::getInstance()->processPacket(query, rsp, false);
+    rsp = ControlledDhcpv4Srv::getInstance()->processPacket(query, false);
 
     if (!rsp) {
         return;
diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.cc b/src/bin/dhcp4/tests/dhcp4_test_utils.cc
index 57fd6df892..fa304f3cc1 100644
--- a/src/bin/dhcp4/tests/dhcp4_test_utils.cc
+++ b/src/bin/dhcp4/tests/dhcp4_test_utils.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2023 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2024 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
diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.h b/src/bin/dhcp4/tests/dhcp4_test_utils.h
index b6e93fcd22..dce7e2ca44 100644
--- a/src/bin/dhcp4/tests/dhcp4_test_utils.h
+++ b/src/bin/dhcp4/tests/dhcp4_test_utils.h
@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2023 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2024 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
@@ -208,6 +208,12 @@ public:
     Pkt4Ptr processDiscover(Pkt4Ptr& discover) {
         AllocEngine::ClientContext4Ptr context(new AllocEngine::ClientContext4());
         earlyGHRLookup(discover, context);
+        sanityCheck(discover);
+        bool drop = false;
+        context->subnet_ = selectSubnet(discover, drop);
+        if (drop) {
+            return (Pkt4Ptr ());
+        }
         return (processDiscover(discover, context));
     }
 
@@ -218,6 +224,12 @@ public:
     Pkt4Ptr processRequest(Pkt4Ptr& request) {
         AllocEngine::ClientContext4Ptr context(new AllocEngine::ClientContext4());
         earlyGHRLookup(request, context);
+        sanityCheck(request);
+        bool drop = false;
+        context->subnet_ = selectSubnet(request, drop);
+        if (drop) {
+            return (Pkt4Ptr ());
+        }
         return (processRequest(request, context));
     }
 
@@ -227,6 +239,7 @@ public:
     void processRelease(Pkt4Ptr& release) {
         AllocEngine::ClientContext4Ptr context(new AllocEngine::ClientContext4());
         earlyGHRLookup(release, context);
+        sanityCheck(release);
         processRelease(release, context);
     }
 
@@ -236,6 +249,7 @@ public:
     void processDecline(Pkt4Ptr& decline) {
         AllocEngine::ClientContext4Ptr context(new AllocEngine::ClientContext4());
         earlyGHRLookup(decline, context);
+        sanityCheck(decline);
         processDecline(decline, context);
     }
 
@@ -246,6 +260,12 @@ public:
     Pkt4Ptr processInform(Pkt4Ptr& inform) {
         AllocEngine::ClientContext4Ptr context(new AllocEngine::ClientContext4());
         earlyGHRLookup(inform, context);
+        sanityCheck(inform);
+        bool drop = false;
+        context->subnet_ = selectSubnet(inform, drop);
+        if (drop) {
+            return (Pkt4Ptr ());
+        }
         return (processInform(inform, context));
     }