From: Marcin Siodelski Date: Wed, 17 Oct 2012 19:29:08 +0000 (+0200) Subject: [2316] Changes according to code review. X-Git-Tag: trac2402_base^2~2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=9bb3d27efdf09dd2e429086767cdee275f3df115;p=thirdparty%2Fkea.git [2316] Changes according to code review. --- diff --git a/src/lib/dhcp/subnet.cc b/src/lib/dhcp/subnet.cc index ada274bbbb..9853884e58 100644 --- a/src/lib/dhcp/subnet.cc +++ b/src/lib/dhcp/subnet.cc @@ -42,7 +42,7 @@ bool Subnet::inRange(const isc::asiolink::IOAddress& addr) const { } void -Subnet::addOption(OptionPtr option, bool persistent /* = false */) { +Subnet::addOption(OptionPtr& option, bool persistent /* = false */) { validateOption(option); options_.push_back(OptionDescriptor(option, persistent)); } @@ -97,11 +97,11 @@ Pool4Ptr Subnet4::getPool4(const isc::asiolink::IOAddress& hint /* = IOAddress(" } void -Subnet4::validateOption(OptionPtr option) { +Subnet4::validateOption(OptionPtr option) const { if (!option) { isc_throw(isc::BadValue, "option configured for subnet must not be NULL"); } else if (option->getUniverse() != Option::V4) { - isc_throw(isc::BadValue, "epected V4 option to be added to the subnet"); + isc_throw(isc::BadValue, "expected V4 option to be added to the subnet"); } } @@ -152,11 +152,11 @@ Pool6Ptr Subnet6::getPool6(const isc::asiolink::IOAddress& hint /* = IOAddress(" } void -Subnet6::validateOption(OptionPtr option) { +Subnet6::validateOption(OptionPtr option) const { if (!option) { isc_throw(isc::BadValue, "option configured for subnet must not be NULL"); } else if (option->getUniverse() != Option::V6) { - isc_throw(isc::BadValue, "epected V6 option to be added to the subnet"); + isc_throw(isc::BadValue, "expected V6 option to be added to the subnet"); } } } // end of isc::dhcp namespace diff --git a/src/lib/dhcp/subnet.h b/src/lib/dhcp/subnet.h index d6a1ccb7dd..21708c53de 100644 --- a/src/lib/dhcp/subnet.h +++ b/src/lib/dhcp/subnet.h @@ -58,30 +58,47 @@ public: /// @brief Constructor. /// - /// @opt option - /// persist if true option is always sent. + /// @param opt option + /// @param persist if true option is always sent. OptionDescriptor(OptionPtr& opt, bool persist) : option(opt), persistent(persist) {}; }; /// @brief Extractor class to extract key with another key. /// - /// This class extracts one multi_index_container key with - /// another key. Within Subnet class it is used to access - /// data inside the OptionDescriptor::option object and use - /// this data as a index key. The standard key extractor such - /// as mem_fun is not sufficient because it can't operate on - /// objects wrapped with structure. + /// This class solves the problem of accessing index key values + /// that are stored in objects nested in other objects. + /// Each OptionDescriptor structure contains the OptionPtr object. + /// The value retured by one of its accessors (getType) is used + /// as an indexing value in the multi_index_container defined below. + /// There is no easy way to mark that value returned by Option::getType + /// should be an index of this multi_index_container. There are standard + /// key extractors such as 'member' or 'mem_fun' but they are not + /// sufficient here. The former can be used to mark that member of + /// the structure that is held in the container should be used as an + /// indexing value. The latter can be used if the indexing value is + /// a product of the class being held in the container. In this complex + /// scenario when the indexing value is a product of the function that + /// is wrapped by the structure, this new extractor template has to be + /// defined. The template class provides a 'chain' of two extractors + /// to access the value returned by nested object and to use it as + /// indexing value. + /// For some more examples of complex keys see: + /// http://www.cs.brown.edu/~jwicks/boost/libs/multi_index/doc/index.html /// /// @tparam KeyExtractor1 extractor used to access data in /// OptionDescriptor::option /// @tparam KeyExtractor2 extractor used to access /// OptionDescriptor::option member. template - struct KeyFromKey { + class KeyFromKey { + public: typedef typename KeyExtractor1::result_type result_type; /// @brief Constructor. + /// + /// @param key1 first key extractor + /// @param key2 second key extractor KeyFromKey(const KeyExtractor1 key1 = KeyExtractor1(), const KeyExtractor2 key2 = KeyExtractor2()) : key1_(key1), key2_(key2) { }; @@ -134,6 +151,7 @@ public: boost::multi_index::indexed_by< // Sequenced index allows accessing elements in the same way // as elements in std::list. + // Sequenced is an index #0. boost::multi_index::sequenced<>, // Start definition of index #1. boost::multi_index::hashed_non_unique< @@ -185,7 +203,7 @@ public: /// requested it or not. /// /// @throw isc::BadValue if invalid option provided. - void addOption(OptionPtr option, bool persistent = false); + void addOption(OptionPtr& option, bool persistent = false); /// @brief Delete all options configured for the subnet. void delOptions(); @@ -207,7 +225,9 @@ public: /// @brief Return a collection of options. /// - /// @return collection of options configured for a subnet. + /// @return reference to collection of options configured for a subnet. + /// The returned reference is valid as long as the Subnet object which + /// returned it still exists. const OptionContainer& getOptions() { return (options_); } @@ -239,7 +259,7 @@ protected: /// @brief Check if option is valid and can be added to a subnet. /// /// @param option option to be validated. - virtual void validateOption(OptionPtr option) = 0; + virtual void validateOption(OptionPtr option) const = 0; /// @brief subnet-id /// @@ -312,7 +332,7 @@ protected: /// @param option option to be validated. /// /// @throw isc::BadValue if provided option is invalid. - virtual void validateOption(OptionPtr option); + virtual void validateOption(OptionPtr option) const; /// @brief collection of pools in that list Pool4Collection pools_; @@ -380,7 +400,7 @@ protected: /// @param option option to be validated. /// /// @throw isc::BadValue if provided option is invalid. - virtual void validateOption(OptionPtr option); + virtual void validateOption(OptionPtr option) const; /// @brief collection of pools in that list Pool6Collection pools_; diff --git a/src/lib/dhcp/tests/subnet_unittest.cc b/src/lib/dhcp/tests/subnet_unittest.cc index e5f2a3e46e..0e2e846141 100644 --- a/src/lib/dhcp/tests/subnet_unittest.cc +++ b/src/lib/dhcp/tests/subnet_unittest.cc @@ -310,10 +310,15 @@ TEST(Subnet6Test, addPersistentOption) { // Add 10 options to the subnet with option codes 100 - 109. for (uint16_t code = 100; code < 110; ++code) { OptionPtr option(new Option(Option::V6, code, OptionBuffer(10, 0xFF))); - // Options with even codes will be flagged persistent. Persistent - // options are those that server sends to clients regardless if - // they ask for them or not. - bool persistent = (code % 2) ? true : false; + // We create 10 options and want some of them to be flagged + // persistent and some non-persistent. Persistent options are + // those that server sends to clients regardless if they ask + // for them or not. We pick 3 out of 10 options and mark them + // non-persistent and 7 other options persistent. + // Code values: 102, 105 and 108 are divisable by 3 + // and options with these codes will be flagged non-persistent. + // Options with other codes will be flagged persistent. + bool persistent = (code % 3) ? true : false; ASSERT_NO_THROW(subnet->addOption(option, persistent)); } @@ -328,15 +333,15 @@ TEST(Subnet6Test, addPersistentOption) { std::pair range_persistent = idx.equal_range(true); - // 5 out of 10 options have been flagged persistent. - ASSERT_EQ(5, distance(range_persistent.first, range_persistent.second)); + // 3 out of 10 options have been flagged persistent. + ASSERT_EQ(7, distance(range_persistent.first, range_persistent.second)); // Get all non-persistent options. std::pair range_non_persistent = idx.equal_range(false); - // 5 out of 10 options have been flagged persistent. - ASSERT_EQ(5, distance(range_non_persistent.first, range_non_persistent.second)); + // 7 out of 10 options have been flagged persistent. + ASSERT_EQ(3, distance(range_non_persistent.first, range_non_persistent.second)); subnet->delOptions();