[#2942] addressed review comments

diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc
index 565e24a31f210eedba5fdaf6c880fe88859d6471..8c50a87a35783c0deb782605a7733c67036c4f09 100644 (file)
--- 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 f2f9a49b3944dfa2d4f0cc595ced88669e0848cc..892c2e8fe2be9a4469ce414b1e639bbf97f0c991 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 d17a210cb5b830570f1086d1fa5060497e107d97..a4004226f88c246686d0b5a67d15a7d114486f8a 100644 (file)
--- 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 1111e72dbb979913c883432eb4e363ab02986ef8..66b10017b2560b22b1892b4acc0c2f2f5133fa74 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 f50639adc50762c200a37f0537a8066f7d2840a5..7aa4da39e69166e5f0f321962b59c1ad2ce677ea 100644 (file)
--- 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);