[4121] added packOptions{4,6}, removed packOptions(), added unit-test

src/lib/dhcp/libdhcp++.cc

@@ -671,38 +671,36 @@ size_t LibDHCP::unpackVendorOptions4(const uint32_t vendor_id, const OptionBuffe
     return (offset);
 }
 
-
-
 void
-LibDHCP::packOptions(isc::util::OutputBuffer& buf,
+LibDHCP::packOptions4(isc::util::OutputBuffer& buf,
                      const OptionCollection& options) {
+    OptionPtr agent;
+    OptionPtr end;
     for (OptionCollection::const_iterator it = options.begin();
          it != options.end(); ++it) {
-        it->second->pack(buf);
-    }
-}
 
-void
-LibDHCP::packOptions4(isc::util::OutputBuffer& buf,
-                     const OptionCollection& options) {
-    OptionCollection::const_iterator it = options.begin();
-    for (; it != options.end(); ++it) {
-	// Some options must be last
-	if ((it->first == DHO_DHCP_AGENT_OPTIONS) ||
-	    (it->first == DHO_END)) {
-	    continue;
-	}
-        it->second->pack(buf);
+        // RAI and END options must be last.
+        switch (it->first) {
+            case DHO_DHCP_AGENT_OPTIONS:
+                agent = it->second;
+                break;
+            case DHO_END:
+                end = it->second;
+                break;
+            default:
+                it->second->pack(buf);
+                break;
+        }
     }
-    // Add the RAI option if it exists
-    it = options.find(DHO_DHCP_AGENT_OPTIONS);
-    if (it != options.end()) {
-	it->second->pack(buf);
+
+    // Add the RAI option if it exists.
+    if (agent) {
+       agent->pack(buf);
     }
-    // And at the end the END option
-    it = options.find(DHO_END);
-    if (it != options.end()) {
-	it->second->pack(buf);
+
+    // And at the end the END option.
+    if (end)  {
+       end->pack(buf);
     }
 }
 

src/lib/dhcp/libdhcp++.h

@@ -123,7 +123,7 @@ public:
                                               uint16_t type,
                                               const OptionBuffer& buf);
 
-    /// @brief Stores options in a buffer.
+    /// @brief Stores DHCPv4 options in a buffer.
     ///
     /// Stores all options defined in options containers in a on-wire
     /// format in output buffer specified by buf.
@@ -132,18 +132,28 @@ 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.
+    ///
     /// @param buf output buffer (assembled options will be stored here)
     /// @param options collection of options to store to
-
-    /// Generic version: to be used when there is no specific order
-    static void packOptions(isc::util::OutputBuffer& buf,
-                            const isc::dhcp::OptionCollection& options);
-
-    /// DHCPv4 version: put some options last
     static void packOptions4(isc::util::OutputBuffer& buf,
                              const isc::dhcp::OptionCollection& options);
 
-    /// DHCPv6 version (currently same than the generic one)
+    /// @brief Stores DHCPv6 options in a buffer.
+    ///
+    /// Stores all options defined in options containers in a on-wire
+    /// format in output buffer specified by buf.
+    ///
+    /// May throw different exceptions if option assembly fails. There
+    /// may be different reasons (option too large, option malformed,
+    /// too many options etc.)
+    ///
+    /// Currently there's no special logic in it. Options are stored in
+    /// the order of their option codes.
+    ///
+    /// @param buf output buffer (assembled options will be stored here)
+    /// @param options collection of options to store to
     static void packOptions6(isc::util::OutputBuffer& buf,
                              const isc::dhcp::OptionCollection& options);
 

src/lib/dhcp/option.cc

@@ -117,7 +117,16 @@ Option::packHeader(isc::util::OutputBuffer& buf) {
 
 void
 Option::packOptions(isc::util::OutputBuffer& buf) {
-    LibDHCP::packOptions(buf, options_);
+    switch (universe_) {
+    case V4:
+        LibDHCP::packOptions4(buf, options_);
+        return;
+    case V6:
+        LibDHCP::packOptions6(buf, options_);
+        return;
+    default:
+        isc_throw(isc::BadValue, "Invalid universe type " << universe_);
+    }
 }
 
 void Option::unpack(OptionBufferConstIter begin,

src/lib/dhcp/tests/libdhcp++_unittest.cc

@@ -476,6 +476,7 @@ static uint8_t v4_opts[] = {
     0x01, 0x02, 0x03, 0x00  // Vendor Specific Information continued
 };
 
+// This test verifies that pack options for v4 is working correctly.
 TEST_F(LibDhcpTest, packOptions4) {
 
     vector<uint8_t> payload[5];
@@ -542,6 +543,41 @@ TEST_F(LibDhcpTest, packOptions4) {
     EXPECT_EQ(0, memcmp(v4_opts, buf.getData(), sizeof(v4_opts)));
 }
 
+// This test verifies that pack options for v4 is working correctly
+// and RAI option is packed last.
+TEST_F(LibDhcpTest, packOptions4Order) {
+
+    uint8_t expected[] = {
+        12,  3, 0,   1,  2, // Just a random option
+        99,  3, 10, 11, 12, // Another random option
+        82,  3, 20, 21, 22 // Relay Agent Info option
+    };
+
+    vector<uint8_t> payload[3];
+    for (unsigned i = 0; i < 3; i++) {
+        payload[i].resize(3);
+        payload[i][0] = i*10;
+        payload[i][1] = i*10+1;
+        payload[i][2] = i*10+2;
+    }
+
+    OptionPtr opt12(new Option(Option::V4, 12, payload[0]));
+    OptionPtr opt99(new Option(Option::V4, 99, payload[1]));
+    OptionPtr opt82(new Option(Option::V4, 82, payload[2]));
+
+    // Let's create options. They are added in 82,12,99, but the should be
+    // packed in 12,99,82 order (82, which is RAI, should go last)
+    isc::dhcp::OptionCollection opts;
+    opts.insert(make_pair(opt82->getType(), opt82));
+    opts.insert(make_pair(opt12->getType(), opt12));
+    opts.insert(make_pair(opt99->getType(), opt99));
+
+    OutputBuffer buf(100);
+    EXPECT_NO_THROW(LibDHCP::packOptions4(buf, opts));
+    ASSERT_EQ(buf.getLength(), sizeof(expected));
+    EXPECT_EQ(0, memcmp(expected, buf.getData(), sizeof(expected)));
+}
+
 TEST_F(LibDhcpTest, unpackOptions4) {
 
     vector<uint8_t> v4packed(v4_opts, v4_opts + sizeof(v4_opts));