From: Razvan Becheriu <razvan@isc.org>
Date: Wed, 28 Jun 2023 14:32:31 +0000 (+0300)
Subject: [#2942] addressed review comments
X-Git-Tag: Kea-2.2.1~6
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=56f684c5dc4dbc9c59ed656c440784283c81dd87;p=thirdparty%2Fkea.git

[#2942] addressed review comments
---

diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc
index 565e24a31f..8c50a87a35 100644
--- a/src/lib/dhcp/libdhcp++.cc
+++ b/src/lib/dhcp/libdhcp++.cc
@@ -941,7 +941,7 @@ LibDHCP::splitOptions4(OptionCollection& options,
         // Let's not do this forever if there is a bug hiding here somewhere...
         // 65535 times should be enough for any packet load...
         if (tries == std::numeric_limits<uint16_t>::max()) {
-            isc_throw(BadValue, "packet split failed after trying "
+            isc_throw(Unexpected, "packet split failed after trying "
                      << tries << " times.");
         }
         bool found = false;
diff --git a/src/lib/dhcp/pkt.cc b/src/lib/dhcp/pkt.cc
index f2f9a49b39..892c2e8fe2 100644
--- a/src/lib/dhcp/pkt.cc
+++ b/src/lib/dhcp/pkt.cc
@@ -37,13 +37,13 @@ Pkt::Pkt(const uint8_t* buf, uint32_t len, const isc::asiolink::IOAddress& local
     }
 }
 
-void
+OptionCollection
 Pkt::cloneOptions() {
     OptionCollection options;
     for (auto const& option : options_) {
         options.emplace(std::make_pair(option.second->getType(), option.second->clone()));
     }
-    options_ = options;
+    return (options);
 }
 
 void
diff --git a/src/lib/dhcp/pkt.h b/src/lib/dhcp/pkt.h
index d17a210cb5..a4004226f8 100644
--- a/src/lib/dhcp/pkt.h
+++ b/src/lib/dhcp/pkt.h
@@ -328,9 +328,10 @@ protected:
 
 public:
 
-    /// @brief Clones all options so that they can be safely modified - some
-    /// options reference objects directly in the server running configuration.
-    void cloneOptions();
+    /// @brief Clones all options so that they can be safely modified.
+    ///
+    /// @return A container with option clones.
+    OptionCollection cloneOptions();
 
     /// @brief Returns the first option of specified type.
     ///
@@ -866,7 +867,7 @@ public:
     ///
     /// @param pkt Pointer to the packet.
     ScopedPktOptionsCopy(PktType& pkt) : pkt_(pkt), options_(pkt.options_) {
-        pkt.cloneOptions();
+        pkt_.options_ = pkt_.cloneOptions();
     }
 
     /// @brief Destructor.
diff --git a/src/lib/dhcp/pkt4.cc b/src/lib/dhcp/pkt4.cc
index 1111e72dbb..66b10017b2 100644
--- a/src/lib/dhcp/pkt4.cc
+++ b/src/lib/dhcp/pkt4.cc
@@ -73,6 +73,9 @@ Pkt4::pack() {
         isc_throw(InvalidOperation, "Can't build Pkt4 packet. HWAddr not set.");
     }
 
+    // This object is necessary to restore the packet options after performing
+    // splitOptions4 when function scope ends. It creates a container of option
+    // clones which are split and packed.
     ScopedPkt4OptionsCopy scoped_options(*this);
 
     // Clear the output buffer to make sure that consecutive calls to pack()
diff --git a/src/lib/dhcp/tests/protocol_util_unittest.cc b/src/lib/dhcp/tests/protocol_util_unittest.cc
index f50639adc5..7aa4da39e6 100644
--- a/src/lib/dhcp/tests/protocol_util_unittest.cc
+++ b/src/lib/dhcp/tests/protocol_util_unittest.cc
@@ -428,7 +428,7 @@ TEST(ScopedOptionsCopy, pkt4OptionsCopy) {
     ASSERT_EQ(option, pkt->getOption(DHO_BOOT_FILE_NAME));
     std::string expected = pkt->toText();
     pkt->pack();
-    auto buf = pkt->getBuffer();
+    OutputBuffer buf = pkt->getBuffer();
     {
         ScopedPkt4OptionsCopy oc(*pkt);
         ASSERT_NE(pkt->options_, options);
@@ -481,7 +481,7 @@ TEST(ScopedOptionsCopy, pkt6OptionsCopy) {
     ASSERT_EQ(option, pkt->getOption(D6O_BOOTFILE_URL));
     std::string expected = pkt->toText();
     pkt->pack();
-    auto buf = pkt->getBuffer();
+    OutputBuffer buf = pkt->getBuffer();
     {
         ScopedPkt6OptionsCopy oc(*pkt);
         ASSERT_NE(pkt->options_, options);