From: Marcin Siodelski Date: Tue, 14 Oct 2014 14:23:13 +0000 (+0200) Subject: [3589] Addressed review comments. X-Git-Tag: trac3162a_base~4^2~3 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=3be6cfb9555d17f19871cf51b11bc17ce7c0fbe2;p=thirdparty%2Fkea.git [3589] Addressed review comments. --- diff --git a/src/lib/dhcp/option.h b/src/lib/dhcp/option.h index ad61d28c6d..df69014139 100644 --- a/src/lib/dhcp/option.h +++ b/src/lib/dhcp/option.h @@ -352,7 +352,17 @@ public: /// just to force that every option has virtual dtor virtual ~Option(); - /// @brief Checks if two options are equal + /// @brief Checks if options are equal. + /// + /// This method calls a virtual @c equals function to compare objects. + /// This method is not meant to be overriden in the derived classes. + /// Instead, the other @c equals function must be overriden. + /// + /// @param other Pointer to the option to compare this option to. + /// @return true if both options are equal, false otherwise. + bool equals(const OptionPtr& other) const; + + /// @brief Checks if two options are equal. /// /// Equality verifies option type and option content. Care should /// be taken when using this method. Implementation for derived @@ -361,10 +371,9 @@ public: /// will detect differences between base Option and derived /// objects. /// - /// @param other the other option - /// @return true if both options are equal - bool equals(const OptionPtr& other) const; - + /// @param other Instance of the option to compare to. + /// + /// @return true if options are equal, false otherwise. virtual bool equals(const Option& other) const; protected: diff --git a/src/lib/dhcp/tests/option_unittest.cc b/src/lib/dhcp/tests/option_unittest.cc index 430bb09420..387794c3ea 100644 --- a/src/lib/dhcp/tests/option_unittest.cc +++ b/src/lib/dhcp/tests/option_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2011-2013 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2011-2014 Internet Systems Consortium, Inc. ("ISC") // // Permission to use, copy, modify, and/or distribute this software for any // purpose with or without fee is hereby granted, provided that the above @@ -547,8 +547,9 @@ TEST_F(OptionTest, setData) { buf_.size())); } -// This test verifies that options can be compared using equals() method. -TEST_F(OptionTest, equals) { +// This test verifies that options can be compared using equals(OptionPtr) +// method. +TEST_F(OptionTest, equalsWithPointers) { // Five options with varying lengths OptionPtr opt1(new Option(Option::V6, 258, buf_.begin(), buf_.begin() + 1)); @@ -570,6 +571,29 @@ TEST_F(OptionTest, equals) { EXPECT_TRUE(opt2->equals(opt5)); } +// This test verifies that options can be compared using equals(Option) method. +TEST_F(OptionTest, equals) { + + // Five options with varying lengths + Option opt1(Option::V6, 258, buf_.begin(), buf_.begin() + 1); + Option opt2(Option::V6, 258, buf_.begin(), buf_.begin() + 2); + Option opt3(Option::V6, 258, buf_.begin(), buf_.begin() + 3); + + // The same content as opt2, but different type + Option opt4(Option::V6, 1, buf_.begin(), buf_.begin() + 2); + + // Another instance with the same type and content as opt2 + Option opt5(Option::V6, 258, buf_.begin(), buf_.begin() + 2); + + EXPECT_TRUE(opt1.equals(opt1)); + + EXPECT_FALSE(opt1.equals(opt2)); + EXPECT_FALSE(opt1.equals(opt3)); + EXPECT_FALSE(opt1.equals(opt4)); + + EXPECT_TRUE(opt2.equals(opt5)); +} + // This test verifies that the name of the option space being encapsulated by // the particular option can be set. TEST_F(OptionTest, setEncapsulatedSpace) { diff --git a/src/lib/dhcpsrv/cfg_option.h b/src/lib/dhcpsrv/cfg_option.h index a8a66b2be8..ab6ff3fa46 100644 --- a/src/lib/dhcpsrv/cfg_option.h +++ b/src/lib/dhcpsrv/cfg_option.h @@ -281,6 +281,7 @@ public: /// /// This method will not return vendor options, i.e. having option space /// name in the format of "vendor-X" where X is 32-bit unsiged integer. + /// See @c getAll(uint32_t) for vendor options. /// /// @param option_space Name of the option space. /// @@ -330,6 +331,8 @@ public: /// If the option space name is specified in the following format: /// "vendor-X" where X is an uint32_t number, it is assumed to be /// a vendor space and the uint32_t number is returned by this function. + /// If the option space name is invalid this method will return 0 which + /// is not valid vendor-id. /// /// @todo remove this function once when the conversion is dealt by the /// appropriate functions returning options by option space names. @@ -342,7 +345,14 @@ private: /// @brief Appends encapsulated options to the options in an option space. /// - /// @param option_space Name of the option space containing optionn to + /// This method appends sub-options to the options belonging to the + /// particular option space. For example: if the option space "foo" + /// is specified, this function will go over all options belonging to + /// "foo" and will check which option spaces they encapsulate. For each + /// such option it will retrieve options for these option spaces and append + /// as sub-options to options belonging to "foo". + /// + /// @param option_space Name of the option space containing option to /// which encapsulated options are appended. void encapsulateInternal(const std::string& option_space); diff --git a/src/lib/dhcpsrv/subnet.h b/src/lib/dhcpsrv/subnet.h index 94777e36bc..a3ca08a7f1 100644 --- a/src/lib/dhcpsrv/subnet.h +++ b/src/lib/dhcpsrv/subnet.h @@ -107,6 +107,12 @@ public: return (cfg_option_); } + /// @brief Returns const pointer to the option data configuration for this + /// subnet. + ConstCfgOptionPtr getCfgOption() const { + return (cfg_option_); + } + /// @brief returns the last address that was tried from this pool /// /// This method returns the last address that was attempted to be allocated diff --git a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc index 121a2ccc73..69d952f757 100644 --- a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc @@ -19,6 +19,8 @@ #include #include #include +#include +#include using namespace isc; using namespace isc::dhcp; @@ -246,7 +248,7 @@ TEST(CfgOptionTest, copy) { // to the top level options on request. TEST(CfgOptionTest, encapsulate) { CfgOption cfg; - // Create top-level options. These options encapsulate "foo" option space. + // Create top-level options encapsulating "foo" option space. for (uint16_t code = 1000; code < 1020; ++code) { OptionUint16Ptr option = OptionUint16Ptr(new OptionUint16(Option::V6, code, 1234)); @@ -254,6 +256,14 @@ TEST(CfgOptionTest, encapsulate) { ASSERT_NO_THROW(cfg.add(option, false, DHCP6_OPTION_SPACE)); } + // Create top level options encapsulating "bar" option space. + for (uint16_t code = 1020; code < 1040; ++code) { + OptionUint16Ptr option = OptionUint16Ptr(new OptionUint16(Option::V6, + code, 2345)); + option->setEncapsulatedSpace("bar"); + ASSERT_NO_THROW(cfg.add(option, false, DHCP6_OPTION_SPACE)); + } + // Create sub-options belonging to "foo" option space. for (uint16_t code = 1; code < 20; ++code) { OptionUint8Ptr option = OptionUint8Ptr(new OptionUint8(Option::V6, code, @@ -261,25 +271,36 @@ TEST(CfgOptionTest, encapsulate) { ASSERT_NO_THROW(cfg.add(option, false, "foo")); } - // Append options from "foo" space as sub-options. + // Create sub-options belonging to "bar" option space. + for (uint16_t code = 100; code < 130; ++code) { + OptionUint8Ptr option = OptionUint8Ptr(new OptionUint8(Option::V6, + code, 0x02)); + ASSERT_NO_THROW(cfg.add(option, false, "bar")); + } + + // Append options from "foo" and "bar" space as sub-options. ASSERT_NO_THROW(cfg.encapsulate()); - // Verify that we have 20 top-level options. + // Verify that we have 40 top-level options. OptionContainerPtr options = cfg.getAll(DHCP6_OPTION_SPACE); - ASSERT_EQ(20, options->size()); + ASSERT_EQ(40, options->size()); - // Verify that each of them contains expected sub-options. - for (uint16_t code = 1000; code < 1020; ++code) { + for (uint16_t code = 1000; code < 1040; ++code) { OptionUint16Ptr option = boost::dynamic_pointer_cast< OptionUint16>(cfg.get(DHCP6_OPTION_SPACE, code).option); ASSERT_TRUE(option) << "option with code " << code << " not found"; - EXPECT_EQ(1234, option->getValue()); - for (uint16_t subcode = 1; subcode < 20; ++subcode) { - OptionUint8Ptr suboption = boost::dynamic_pointer_cast< - OptionUint8>(option->getOption(subcode)); - ASSERT_TRUE(suboption) << "suboption with code " << subcode - << " not found"; - EXPECT_EQ(0x01, suboption->getValue()); + const OptionCollection& suboptions = option->getOptions(); + for (OptionCollection::const_iterator suboption = + suboptions.begin(); suboption != suboptions.end(); + ++suboption) { + OptionUint8Ptr opt = boost::dynamic_pointer_cast< + OptionUint8>(suboption->second); + ASSERT_TRUE(opt); + if (code < 1020) { + EXPECT_EQ(0x01, opt->getValue()); + } else { + EXPECT_EQ(0x02, opt->getValue()); + } } } } @@ -410,11 +431,16 @@ TEST(CfgOptionTest, addVendorOptions) { ASSERT_NO_THROW(cfg.add(option, false, "vendor-12345678")); } + // Second option space uses corner case value for vendor id = max uint8. + uint32_t vendor_id = std::numeric_limits::max(); + std::ostringstream option_space; + option_space << "vendor-" << vendor_id; + // Add 7 options to another option space. The option codes partially overlap // with option codes that we have added to dhcp6 option space. for (uint16_t code = 105; code < 112; ++code) { OptionPtr option(new Option(Option::V6, code, OptionBuffer(10, 0xFF))); - ASSERT_NO_THROW(cfg.add(option, false, "vendor-87654321")); + ASSERT_NO_THROW(cfg.add(option, false, option_space.str())); } // Get options from the Subnet and check if all 10 are there. @@ -431,7 +457,7 @@ TEST(CfgOptionTest, addVendorOptions) { ++expected_code; } - options = cfg.getAll(87654321); + options = cfg.getAll(vendor_id); ASSERT_TRUE(options); ASSERT_EQ(7, options->size());