[#3440] Checkpoint: added code, UT to update

4 files changed, 90 insertions, 94 deletions

diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc
index fd790a6ebf..87660b0515 100644
--- a/src/lib/dhcp/libdhcp++.cc
+++ b/src/lib/dhcp/libdhcp++.cc
@@ -30,6 +30,8 @@
 
 #include <limits>
 #include <list>
+#include <unordered_map>
+#include <vector>
 
 using namespace std;
 using namespace isc::dhcp;
@@ -482,6 +484,69 @@ LibDHCP::unpackOptions4(const OptionBuffer& buf, const string& option_space,
 
     // The buffer being read comprises a set of options, each starting with
     // a one-byte type code and a one-byte length field.
+
+    // Track seen options in a first pass.
+    vector<bool> seen(256, false);
+    unordered_map<uint8_t, size_t> counts;
+    while (offset < buf.size()) {
+        // Get the option type
+        uint8_t opt_type = buf[offset++];
+
+        // DHO_END is a special, one octet long option
+        // Valid in dhcp4 space or when check is true and
+        // there is a sub-option configured for this code.
+        if ((opt_type == DHO_END) && (space_is_dhcp4 || flex_end)) {
+            // Done.
+            break;
+        }
+
+        // DHO_PAD is just a padding after DHO_END. Let's continue parsing
+        // in case we receive a message without DHO_END.
+        // Valid in dhcp4 space or when check is true and
+        // there is a sub-option configured for this code.
+        if ((opt_type == DHO_PAD) && (space_is_dhcp4 || flex_pad)) {
+            continue;
+        }
+
+        if (offset + 1 > buf.size()) {
+            // Error case.
+            break;
+        }
+
+        uint8_t opt_len = buf[offset++];
+        if (offset + opt_len > buf.size()) {
+            // Error case.
+            break;
+        }
+
+        // See below for this special case.
+        if (space_is_dhcp4 && opt_len == 0 && opt_type == DHO_HOST_NAME) {
+            continue;
+        }
+
+        offset += opt_len;
+
+        // Mark as seen.
+        if (!seen[opt_type]) {
+            seen[opt_type] = true;
+            continue;
+        }
+
+        // Already seen.
+        size_t& count = counts[opt_type];
+        if (count == 0) {
+            // Default value for size_t is 0 but this option was already seen.
+            count = 2;
+        } else {
+            ++count;
+        }
+    }
+
+    // Fusing option buffers.
+    unordered_map<uint8_t, OptionBuffer> fused;
+
+    // Second pass.
+    offset = 0;
     while (offset < buf.size()) {
         // Save the current offset for backtracking
         last_offset = offset;
@@ -536,6 +601,25 @@ LibDHCP::unpackOptions4(const OptionBuffer& buf, const string& option_space,
             continue;
         }
 
+        OptionBuffer obuf(buf.begin() + offset, buf.begin() + offset + opt_len);
+        offset += opt_len;
+
+        // Concatenate multiple instance of an option.
+        try {
+            size_t count = counts.at(opt_type);
+            OptionBuffer& previous = fused[opt_type];
+            previous.insert(previous.end(), obuf.begin(), obuf.end());
+            if (count <= 1) {
+                // last occurrence: build the option.
+                obuf = previous;
+            } else {
+                counts[opt_type] = count - 1;
+                continue;
+            }
+        } catch (const std::out_of_range&) {
+            // Regular case.
+        }
+
         // Get all definitions with the particular option code. Note
         // that option code is non-unique within this container
         // however at this point we expect to get one option
@@ -592,9 +676,7 @@ LibDHCP::unpackOptions4(const OptionBuffer& buf, const string& option_space,
                       " This will be supported once support for option spaces"
                       " is implemented");
         } else if (num_defs == 0) {
-            opt = OptionPtr(new Option(Option::V4, opt_type,
-                                       buf.begin() + offset,
-                                       buf.begin() + offset + opt_len));
+            opt = OptionPtr(new Option(Option::V4, opt_type, obuf));
             opt->setEncapsulatedSpace(DHCP4_OPTION_SPACE);
         } else {
             try {
@@ -602,9 +684,7 @@ LibDHCP::unpackOptions4(const OptionBuffer& buf, const string& option_space,
                 // the option instance from the provided buffer chunk.
                 const OptionDefinitionPtr& def = *(range.first);
                 isc_throw_assert(def);
-                opt = def->optionFactory(Option::V4, opt_type,
-                                         buf.begin() + offset,
-                                         buf.begin() + offset + opt_len);
+                opt = def->optionFactory(Option::V4, opt_type, obuf);
             } catch (const SkipThisOptionError&) {
                 opt.reset();
             }
@@ -614,79 +694,11 @@ LibDHCP::unpackOptions4(const OptionBuffer& buf, const string& option_space,
         if (opt) {
             options.insert(std::make_pair(opt_type, opt));
         }
-
-        offset += opt_len;
     }
     last_offset = offset;
     return (last_offset);
 }
 
-bool
-LibDHCP::fuseOptions4(OptionCollection& options) {
-    bool result = false;
-    // We need to loop until all options have been fused.
-    for (;;) {
-        uint32_t found = 0;
-        bool found_suboptions = false;
-        // Iterate over all options in the container.
-        for (auto const& option : options) {
-            OptionPtr candidate = option.second;
-            OptionCollection& sub_options = candidate->getMutableOptions();
-            // Fuse suboptions recursively, if any.
-            if (sub_options.size()) {
-                // Fusing suboptions might result in new options with multiple
-                // options having the same code, so we need to iterate again
-                // until no option needs fusing.
-                found_suboptions = LibDHCP::fuseOptions4(sub_options);
-                if (found_suboptions) {
-                    result = true;
-                }
-            }
-            OptionBuffer data;
-            OptionCollection suboptions;
-            // Make a copy of the options so we can safely iterate over the
-            // old container.
-            OptionCollection copy = options;
-            for (auto const& old_option : copy) {
-                if (old_option.first == option.first) {
-                    // Copy the option data to the buffer.
-                    data.insert(data.end(), old_option.second->getData().begin(),
-                                old_option.second->getData().end());
-                    suboptions.insert(old_option.second->getOptions().begin(),
-                                      old_option.second->getOptions().end());
-                    // Other options might need fusing, so we need to iterate
-                    // again until no options needs fusing.
-                    found++;
-                }
-            }
-            if (found > 1) {
-                result = true;
-                // Erase the old options from the new container so that only the
-                // new option is present.
-                copy.erase(option.first);
-                // Create new option with entire data.
-                OptionPtr new_option(new Option(candidate->getUniverse(),
-                                                candidate->getType(), data));
-                // Recreate suboptions container.
-                new_option->getMutableOptions() = suboptions;
-                // Add the new option to the new container.
-                copy.insert(make_pair(candidate->getType(), new_option));
-                // After all options have been fused and new option added,
-                // update the option container with the new container.
-                options = copy;
-                break;
-            } else {
-                found = 0;
-            }
-        }
-        // No option needs fusing, so we can exit the loop.
-        if ((found <= 1) && !found_suboptions) {
-            break;
-        }
-    }
-    return (result);
-}
-
 namespace { // Anonymous namespace.
 
 // VIVCO part of extendVendorOptions4.
diff --git a/src/lib/dhcp/libdhcp++.h b/src/lib/dhcp/libdhcp++.h
index 3de55167ce..89a02be275 100644
--- a/src/lib/dhcp/libdhcp++.h
+++ b/src/lib/dhcp/libdhcp++.h
@@ -291,14 +291,6 @@ public:
                                  size_t* relay_msg_offset = 0,
                                  size_t* relay_msg_len = 0);
 
-    /// @brief Fuse multiple options with the same option code in long options
-    /// (RFC3396).
-    ///
-    /// @param options The option container which needs to be updated with fused
-    /// options.
-    /// @return True if any option has been fused, false otherwise.
-    static bool fuseOptions4(isc::dhcp::OptionCollection& options);
-
     /// @brief Extend vendor options from fused options in multiple OptionVendor
     /// or OptionVendorClass options and add respective suboptions or values.
     ///
diff --git a/src/lib/dhcp/pkt4.cc b/src/lib/dhcp/pkt4.cc
index d552eead56..88bbbce5d2 100644
--- a/src/lib/dhcp/pkt4.cc
+++ b/src/lib/dhcp/pkt4.cc
@@ -222,14 +222,6 @@ Pkt4::unpack() {
     // }
     (void)offset;
 
-    // The RFC3396 adds support for multiple options using the same code fused
-    // into long options.
-    // All instances of the same option are fused together, including merging
-    // the suboption lists and fusing suboptions. As a result, the options will
-    // store more than 255 bytes of data and the regex parsers can effectively
-    // access the entire data.
-    LibDHCP::fuseOptions4(options_);
-
     // Kea supports multiple vendor options so it needs to split received and
     // fused options in multiple OptionVendor instances.
     LibDHCP::extendVendorOptions4(options_);
diff --git a/src/lib/dhcp/tests/libdhcp++_unittest.cc b/src/lib/dhcp/tests/libdhcp++_unittest.cc
index 948a12963d..ae5dc18976 100644
--- a/src/lib/dhcp/tests/libdhcp++_unittest.cc
+++ b/src/lib/dhcp/tests/libdhcp++_unittest.cc
@@ -1548,7 +1548,7 @@ TEST_F(LibDhcpTest, fuseLongOption) {
         col.insert(std::make_pair(231, option));
     }
     ASSERT_EQ(256, col.size());
-    LibDHCP::fuseOptions4(col);
+    //// LibDHCP::fuseOptions4(col);
 
     ASSERT_EQ(1, col.size());
     uint8_t index = 0;
@@ -1588,7 +1588,7 @@ TEST_F(LibDhcpTest, fuseLongOptionWithLongSuboption) {
     col.insert(std::make_pair(213, rai));
     ASSERT_EQ(1, col.size());
     ASSERT_EQ(256, col.begin()->second->getOptions().size());
-    LibDHCP::fuseOptions4(col);
+    //// LibDHCP::fuseOptions4(col);
 
     ASSERT_EQ(1, col.size());
     ASSERT_EQ(1, col.begin()->second->getOptions().size());
@@ -1644,7 +1644,7 @@ TEST_F(LibDhcpTest, extendVivco) {
     options.insert(make_pair(DHO_VIVCO_SUBOPTIONS, opt2));
     options.insert(make_pair(DHO_VIVCO_SUBOPTIONS, opt3));
     EXPECT_EQ(options.size(), 3);
-    EXPECT_NO_THROW(LibDHCP::fuseOptions4(options));
+    //// LibDHCP::fuseOptions4(options);
     EXPECT_EQ(options.size(), 1);
     EXPECT_NO_THROW(LibDHCP::extendVendorOptions4(options));
     EXPECT_EQ(options.size(), 2);
@@ -1712,7 +1712,7 @@ TEST_F(LibDhcpTest, extendVivso) {
                                                       option.second->getType(),
                                                       buffer))));
     }
-    ASSERT_NO_THROW(LibDHCP::fuseOptions4(options));
+    //// LibDHCP::fuseOptions4(options);
     ASSERT_EQ(options.size(), 1);
     ASSERT_EQ(options.count(DHO_VIVSO_SUBOPTIONS), 1);
     ASSERT_EQ(options.find(DHO_VIVSO_SUBOPTIONS)->second->getType(), DHO_VIVSO_SUBOPTIONS);