[#2942] addressed review comments

diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc
index 235b0517660cabe44d72ccffed759577c18cd479..5512a96b1050e97facf8152ce24be1f5154ad7e1 100644 (file)
--- 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 f1b7e1b6c51712f0001e06198e1afe1f23c4807e..26224602ff5837f849bef03e3d0f8c0194cd57aa 100644 (file)
--- 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 06dcab6bc25a91775757936d20bd73f9d3f67492..f17f9f13a8660c9fdf58621d0613fd086c854474 100644 (file)
--- 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 41ef6b866637d7a97d125c5cbc86ab39963960bc..595adfb6e8f06d5088f6ee6419e791990c94d520 100644 (file)
--- 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 e38ad59a3722c8e42a74ef71c5b95931b8d4f433..e413b6ea5989fbc512dcb4dd1ed51dd7da2563af 100644 (file)
--- 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);