From: Thomas Markwalder <tmark@isc.org>
Date: Wed, 19 Dec 2018 20:41:29 +0000 (-0500)
Subject: [#363, !177] kea-dhcp4 now emits message type option (53) first
X-Git-Tag: 111-configure-options-with-cql-not-working-properly_base~1^2~1
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c580327cfed73ea50f24874d468a1ff8151ed8a5;p=thirdparty%2Fkea.git

[#363, !177] kea-dhcp4 now emits message type option (53) first

src/lib/dhcp/libdhcp++.*
    LibDHCP::packOptions4() - added logic to emit message type option (53) first

src/lib/dhcp/tests/pkt4_unittest.cc
    Updated unit tests accordingly
---

diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc
index ced705dd29..9ef5c11c09 100644
--- a/src/lib/dhcp/libdhcp++.cc
+++ b/src/lib/dhcp/libdhcp++.cc
@@ -787,14 +787,29 @@ size_t LibDHCP::unpackVendorOptions4(const uint32_t vendor_id, const OptionBuffe
 
 void
 LibDHCP::packOptions4(isc::util::OutputBuffer& buf,
-                     const OptionCollection& options) {
+                     const OptionCollection& options,
+                     bool top /* = false */) {
     OptionPtr agent;
     OptionPtr end;
+
+    // We only look for type when we're the top level
+    // call that starts packing for options for a packet.
+    // This way we avoid doing type logic in all ensuing
+    // recursive calls.
+    if (top) {
+        auto x = options.find(DHO_DHCP_MESSAGE_TYPE);
+        if (x != options.end()) {
+            x->second->pack(buf);
+        }
+    }
+
     for (OptionCollection::const_iterator it = options.begin();
          it != options.end(); ++it) {
 
-        // RAI and END options must be last.
+        // TYPE is already done, RAI and END options must be last.
         switch (it->first) {
+            case DHO_DHCP_MESSAGE_TYPE:
+                break;
             case DHO_DHCP_AGENT_OPTIONS:
                 agent = it->second;
                 break;
diff --git a/src/lib/dhcp/libdhcp++.h b/src/lib/dhcp/libdhcp++.h
index c505656793..2f09840b1a 100644
--- a/src/lib/dhcp/libdhcp++.h
+++ b/src/lib/dhcp/libdhcp++.h
@@ -178,13 +178,22 @@ public:
     /// may be different reasons (option too large, option malformed,
     /// too many options etc.)
     ///
-    /// This is v4 specific version, which stores Relay Agent Information
-    /// option and END options last.
+    /// This is v4 specific version, which stores DHCP message type first,
+    /// and the Relay Agent Information option and END options last. This
+    /// function is initially called to pack the options for a packet in
+    /// @ref Pkt4::pack(). That call leads to it being called recursively in
+    /// vai @ref Option::packOptions(). Thus the logic used to output the
+    /// message type should only be executed by the top-most. This is governed
+    /// by the paramater top, below.
     ///
     /// @param buf output buffer (assembled options will be stored here)
     /// @param options collection of options to store to
+    /// @param top indicates if this is the first call to pack the options.
+    /// When true logic to emit the message type is executed. It defaults to
+    /// false.
     static void packOptions4(isc::util::OutputBuffer& buf,
-                             const isc::dhcp::OptionCollection& options);
+                             const isc::dhcp::OptionCollection& options,
+                             bool top = false);
 
     /// @brief Stores DHCPv6 options in a buffer.
     ///
diff --git a/src/lib/dhcp/pkt4.cc b/src/lib/dhcp/pkt4.cc
index 007e3f91fc..a354eb0bbe 100644
--- a/src/lib/dhcp/pkt4.cc
+++ b/src/lib/dhcp/pkt4.cc
@@ -135,7 +135,7 @@ Pkt4::pack() {
         // write DHCP magic cookie
         buffer_out_.writeUint32(DHCP_OPTIONS_COOKIE);
 
-        LibDHCP::packOptions4(buffer_out_, options_);
+        LibDHCP::packOptions4(buffer_out_, options_, true);
 
         // add END option that indicates end of options
         // (End option is very simple, just a 255 octet)
diff --git a/src/lib/dhcp/tests/pkt4_unittest.cc b/src/lib/dhcp/tests/pkt4_unittest.cc
index 930f76547a..c052865a37 100644
--- a/src/lib/dhcp/tests/pkt4_unittest.cc
+++ b/src/lib/dhcp/tests/pkt4_unittest.cc
@@ -47,9 +47,9 @@ namespace {
 /// variable length data so as there are no restrictions
 /// on a length of their data.
 static uint8_t v4_opts[] = {
+    53, 1, 2, // Message Type (required to not throw exception during unpack)
     12,  3, 0,   1,  2, // Hostname
     14,  3, 10, 11, 12, // Merit Dump File
-    53, 1, 2, // Message Type (required to not throw exception during unpack)
     60,  3, 20, 21, 22, // Class Id
     128, 3, 30, 31, 32, // Vendor specific
     254, 3, 40, 41, 42, // Reserved
@@ -177,16 +177,23 @@ public:
         EXPECT_TRUE(pkt->getOption(128));
         EXPECT_TRUE(pkt->getOption(254));
 
-        boost::shared_ptr<Option> x = pkt->getOption(12);
-        ASSERT_TRUE(x); // option 1 should exist
+        // Verify the packet type is correct.
+        ASSERT_EQ(DHCPOFFER, pkt->getType());
+
+        // First option after message type starts at 3.
+        uint8_t *opt_data_ptr = v4_opts + 3;
+
         // Option 12 is represented by the OptionString class so let's do
         // the appropriate conversion.
+        boost::shared_ptr<Option> x = pkt->getOption(12);
+        ASSERT_TRUE(x); // option 1 should exist
         OptionStringPtr option12 = boost::static_pointer_cast<OptionString>(x);
         ASSERT_TRUE(option12);
         EXPECT_EQ(12, option12->getType());  // this should be option 12
         ASSERT_EQ(3, option12->getValue().length()); // it should be of length 3
         EXPECT_EQ(5, option12->len()); // total option length 5
-        EXPECT_EQ(0, memcmp(&option12->getValue()[0], v4_opts + 2, 3)); // data len=3
+        EXPECT_EQ(0, memcmp(&option12->getValue()[0], opt_data_ptr + 2, 3)); // data len=3
+        opt_data_ptr += x->len();
 
         x = pkt->getOption(14);
         ASSERT_TRUE(x); // option 14 should exist
@@ -197,28 +204,31 @@ public:
         EXPECT_EQ(14, option14->getType());  // this should be option 14
         ASSERT_EQ(3, option14->getValue().length()); // it should be of length 3
         EXPECT_EQ(5, option14->len()); // total option length 5
-        EXPECT_EQ(0, memcmp(&option14->getValue()[0], v4_opts + 7, 3)); // data len=3
+        EXPECT_EQ(0, memcmp(&option14->getValue()[0], opt_data_ptr + 2, 3)); // data len=3
+        opt_data_ptr += x->len();
 
         x = pkt->getOption(60);
         ASSERT_TRUE(x); // option 60 should exist
         EXPECT_EQ(60, x->getType());  // this should be option 60
         ASSERT_EQ(3, x->getData().size()); // it should be of length 3
         EXPECT_EQ(5, x->len()); // total option length 5
-        EXPECT_EQ(0, memcmp(&x->getData()[0], v4_opts + 15, 3)); // data len=3
+        EXPECT_EQ(0, memcmp(&x->getData()[0], opt_data_ptr + 2, 3)); // data len=3
+        opt_data_ptr += x->len();
 
         x = pkt->getOption(128);
         ASSERT_TRUE(x); // option 3 should exist
         EXPECT_EQ(128, x->getType());  // this should be option 254
         ASSERT_EQ(3, x->getData().size()); // it should be of length 3
         EXPECT_EQ(5, x->len()); // total option length 5
-        EXPECT_EQ(0, memcmp(&x->getData()[0], v4_opts + 20, 3)); // data len=3
+        EXPECT_EQ(0, memcmp(&x->getData()[0], opt_data_ptr + 2, 3)); // data len=3
+        opt_data_ptr += x->len();
 
         x = pkt->getOption(254);
         ASSERT_TRUE(x); // option 3 should exist
         EXPECT_EQ(254, x->getType());  // this should be option 254
         ASSERT_EQ(3, x->getData().size()); // it should be of length 3
         EXPECT_EQ(5, x->len()); // total option length 5
-        EXPECT_EQ(0, memcmp(&x->getData()[0], v4_opts + 25, 3)); // data len=3
+        EXPECT_EQ(0, memcmp(&x->getData()[0], opt_data_ptr + 2, 3)); // data len=3
     }
 
 };