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.4.0~42
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e4600726936a83ee571b8168e73db360c484cc6f;p=thirdparty%2Fkea.git

[#2942] addressed review comments
---

diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc
index 235b051766..5512a96b10 100644
--- a/src/lib/dhcp/libdhcp++.cc
+++ b/src/lib/dhcp/libdhcp++.cc
@@ -1062,7 +1062,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 f1b7e1b6c5..26224602ff 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 06dcab6bc2..f17f9f13a8 100644
--- a/src/lib/dhcp/pkt.h
+++ b/src/lib/dhcp/pkt.h
@@ -370,9 +370,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.
     ///
@@ -928,7 +929,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 41ef6b8666..595adfb6e8 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 e38ad59a37..e413b6ea59 100644
--- a/src/lib/dhcp/tests/protocol_util_unittest.cc
+++ b/src/lib/dhcp/tests/protocol_util_unittest.cc
@@ -542,7 +542,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);
@@ -595,7 +595,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);