From 2f883d4afb9b27c6c59d993692370685b206b6c2 Mon Sep 17 00:00:00 2001 From: Tomek Mrugalski Date: Tue, 1 Dec 2015 19:15:43 +0100 Subject: [PATCH] [4121] added packOptions{4,6}, removed packOptions(), added unit-test --- src/lib/dhcp/libdhcp++.cc | 48 ++++++++++++------------ src/lib/dhcp/libdhcp++.h | 26 +++++++++---- src/lib/dhcp/option.cc | 11 +++++- src/lib/dhcp/tests/libdhcp++_unittest.cc | 36 ++++++++++++++++++ 4 files changed, 87 insertions(+), 34 deletions(-) diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc index bbc7f82367..07b32828be 100644 --- a/src/lib/dhcp/libdhcp++.cc +++ b/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); } } diff --git a/src/lib/dhcp/libdhcp++.h b/src/lib/dhcp/libdhcp++.h index eaac60d7a4..55c5992ec8 100644 --- a/src/lib/dhcp/libdhcp++.h +++ b/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); diff --git a/src/lib/dhcp/option.cc b/src/lib/dhcp/option.cc index 063884ad34..ba8c4f798b 100644 --- a/src/lib/dhcp/option.cc +++ b/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, diff --git a/src/lib/dhcp/tests/libdhcp++_unittest.cc b/src/lib/dhcp/tests/libdhcp++_unittest.cc index b80931818a..566f64c579 100644 --- a/src/lib/dhcp/tests/libdhcp++_unittest.cc +++ b/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 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 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 v4packed(v4_opts, v4_opts + sizeof(v4_opts)); -- 2.47.3