From: Marcin Siodelski Date: Wed, 10 Oct 2012 15:21:01 +0000 (+0200) Subject: [2304] Code cleanup. X-Git-Tag: trac2402_base~5^2~6 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=85627bca2677b4e68d67c8ecd35fbfe409eda251;p=thirdparty%2Fkea.git [2304] Code cleanup. --- diff --git a/src/lib/dhcp/option6_int.h b/src/lib/dhcp/option6_int.h index e0df4c5f4e..3401fac9a1 100644 --- a/src/lib/dhcp/option6_int.h +++ b/src/lib/dhcp/option6_int.h @@ -15,12 +15,13 @@ #ifndef OPTION6_INT_H_ #define OPTION6_INT_H_ +#include +#include +#include +#include + #include #include -#include -#include "dhcp/libdhcp++.h" -#include "dhcp/option.h" -#include "dhcp/option_data_types.h" namespace isc { namespace dhcp { diff --git a/src/lib/dhcp/option6_int_array.h b/src/lib/dhcp/option6_int_array.h index b87f538ca5..2447dac92a 100644 --- a/src/lib/dhcp/option6_int_array.h +++ b/src/lib/dhcp/option6_int_array.h @@ -15,12 +15,13 @@ #ifndef OPTION6_INT_ARRAY_H_ #define OPTION6_INT_ARRAY_H_ +#include +#include +#include +#include + #include #include -#include -#include "dhcp/libdhcp++.h" -#include "dhcp/option.h" -#include "dhcp/option_data_types.h" namespace isc { namespace dhcp { @@ -149,7 +150,7 @@ public: /// @brief Set option values. /// - /// @param collection of values to be set. + /// @param values collection of values to be set for option. void setValues(const std::vector& values) { values_ = values; } /// @brief returns complete length of option diff --git a/src/lib/dhcp/option_data_types.h b/src/lib/dhcp/option_data_types.h index 52be59f97a..4e8d8a6faa 100644 --- a/src/lib/dhcp/option_data_types.h +++ b/src/lib/dhcp/option_data_types.h @@ -15,9 +15,10 @@ #ifndef OPTION_DATA_TYPES_H_ #define OPTION_DATA_TYPES_H_ -#include #include +#include + namespace isc { namespace dhcp { @@ -28,43 +29,53 @@ public: isc::Exception(file, line, what) { }; }; -/// @brief Numeric data types supported by DHCP option defintions. +/// @brief Trait class for integer data types supported in DHCP option definitions. +/// +/// This is useful to check whether the type specified as template parameter +/// is supported by classes like Option6Int, Option6IntArray and some template +/// factory functions in OptionDefinition class. template struct OptionDataTypes { static const bool valid = false; static const int len = 0; }; +/// int8_t type is supported. template<> struct OptionDataTypes { static const bool valid = true; static const int len = 1; }; +/// int16_t type is supported. template<> struct OptionDataTypes { static const bool valid = true; static const int len = 2; }; +/// int32_t type is supported. template<> struct OptionDataTypes { static const bool valid = true; static const int len = 4; }; +/// uint8_t type is supported. template<> struct OptionDataTypes { static const bool valid = true; static const int len = 1; }; +/// uint16_t type is supported. template<> struct OptionDataTypes { static const bool valid = true; static const int len = 2; }; +/// uint32_t type is supported. template<> struct OptionDataTypes { static const bool valid = true; diff --git a/src/lib/dhcp/option_definition.cc b/src/lib/dhcp/option_definition.cc index 07305b9eff..49c5642baf 100644 --- a/src/lib/dhcp/option_definition.cc +++ b/src/lib/dhcp/option_definition.cc @@ -12,14 +12,14 @@ // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR // PERFORMANCE OF THIS SOFTWARE. -#include "dhcp/dhcp6.h" -#include "dhcp/option_definition.h" -#include "dhcp/option4_addrlst.h" -#include "dhcp/option6_addrlst.h" -#include "dhcp/option6_ia.h" -#include "dhcp/option6_iaaddr.h" -#include "dhcp/option6_int.h" -#include "dhcp/option6_int_array.h" +#include +#include +#include +#include +#include +#include +#include +#include using namespace std; using namespace isc::util; @@ -97,6 +97,10 @@ OptionDefinition::addRecordField(const DataType data_type) { Option::Factory* OptionDefinition::getFactory() const { + // @todo This function must be extended to return more factory + // functions that create instances of more specialized options. + // This requires us to first implement those more specialized + // options that will be derived from Option class. if (type_ == IPV6_ADDRESS_TYPE && array_type_) { return (factoryAddrList6); } else if (type_ == IPV4_ADDRESS_TYPE && array_type_) { @@ -126,6 +130,11 @@ OptionDefinition::getFactory() const { return (factoryInteger); } } + // Factory generic returns instance of Option class. However, once we + // implement CustomOption class we may want to return factory function + // that will create instance of CustomOption rather than Option. + // CustomOption will allow to access particular data fields within the + // option rather than raw data buffer. return (factoryGeneric); } @@ -139,6 +148,15 @@ OptionDefinition::sanityCheckUniverse(const Option::Universe expected_universe, void OptionDefinition::validate() const { + // Option name must not be empty. + if (name_.empty()) { + isc_throw(isc::BadValue, "option name must not be empty"); + } + // Option name must not contain spaces. + if (name_.find(" ") != string::npos) { + isc_throw(isc::BadValue, "option name must not contain spaces"); + } + // Unsupported option types are not allowed. if (type_ >= UNKNOWN_TYPE) { isc_throw(isc::OutOfRange, "option type value " << type_ << " is out of range"); diff --git a/src/lib/dhcp/option_definition.h b/src/lib/dhcp/option_definition.h index 764fe85b0e..ad0598cd2a 100644 --- a/src/lib/dhcp/option_definition.h +++ b/src/lib/dhcp/option_definition.h @@ -15,10 +15,10 @@ #ifndef OPTION_DEFINITION_H_ #define OPTION_DEFINITION_H_ -#include "dhcp/option_data_types.h" -#include "dhcp/option6_int.h" -#include "dhcp/option6_int_array.h" -#include "dhcp/option.h" +#include +#include +#include +#include namespace isc { namespace dhcp { @@ -51,7 +51,7 @@ namespace dhcp { /// limited by the maximal DHCPv6 option length). /// Should option comprise data fields of different types the "record" /// option type is used. In such cases the data field types within the -/// record are specified using \ref OptionDefinition::AddRecordField. +/// record are specified using \ref OptionDefinition::addRecordField. /// When OptionDefinition object has been sucessfully created it /// can be queried to return the appropriate option factory function /// for the specified option format. There is a number of "standard" @@ -76,6 +76,7 @@ namespace dhcp { /// /// @todo Extend the comment to describe "generic factories". /// @todo Extend this class to use custom namespaces. +/// @todo Extend this class with more factory functions. class OptionDefinition { public: @@ -105,6 +106,13 @@ public: private: /// @brief Utility class for operations on DataTypes. + /// + /// This class is implemented as the singleton because the list of + /// supported data types is in this case loaded only once + /// into the memory and persists for all option definitions. + /// + /// @todo This class can be extended to return the string value + /// representing the data type from the enum value. class DataTypeUtil { public: @@ -208,6 +216,13 @@ public: /// @return option data type. DataType getType() const { return (type_); }; + /// @brief Check if the option definition is valid. + /// + /// @throw isc::OutOfRange if invalid option type was specified. + /// @throw isc::BadValue if invalid option name was specified, + /// e.g. empty or containing spaces. + void validate() const; + /// @brief Check if specified format is IA_NA option format. /// /// @return true if specified format is IA_NA option format. @@ -223,6 +238,9 @@ public: /// @param u universe (must be V4). /// @param type option type. /// @param buf option buffer with a list of IPv4 addresses. + /// + /// @throw isc::OutOfRange if length of the provided option buffer + /// is not multiple of IPV4 address length. static OptionPtr factoryAddrList4(Option::Universe u, uint16_t type, const OptionBuffer& buf); @@ -231,6 +249,9 @@ public: /// @param u universe (must be V6). /// @param type option type. /// @param buf option buffer with a list of IPv6 addresses. + /// + /// @throw isc::OutOfaRange if length of provided option buffer + /// is not multiple of IPV6 address length. static OptionPtr factoryAddrList6(Option::Universe u, uint16_t type, const OptionBuffer& buf); @@ -242,7 +263,7 @@ public: static OptionPtr factoryEmpty(Option::Universe u, uint16_t type, const OptionBuffer& buf); - /// @brief Factory to create generic option.. + /// @brief Factory to create generic option. /// /// @param u universe (V6 or V4). /// @param type option type. @@ -276,10 +297,11 @@ public: /// @brief Factory function to create option with integer value. /// - /// @param u universe (V6 or V4). /// @param type option type. /// @param buf option buffer. - /// @param T type of the data field (must be one of the uintX_t or intX_t). + /// @tparam T type of the data field (must be one of the uintX_t or intX_t). + /// + /// @throw isc::OutOfRange if provided option buffer length is invalid. template static OptionPtr factoryInteger(Option::Universe, uint16_t type, const OptionBuffer& buf) { if (buf.size() > sizeof(T)) { @@ -292,10 +314,11 @@ public: /// @brief Factory function to create option with array of integer values. /// - /// @param u universe (V6 or V4). /// @param type option type. /// @param buf option buffer. - /// @param T type of the data field (must be one of the uintX_t or intX_t). + /// @tparam T type of the data field (must be one of the uintX_t or intX_t). + /// + /// @throw isc::OutOfRange if provided option buffer length is invalid. template static OptionPtr factoryIntegerArray(Option::Universe, uint16_t type, const OptionBuffer& buf) { if (buf.size() == 0) { @@ -323,13 +346,8 @@ private: /// @param actual_universe actual universe value. /// /// @throw isc::BadValue if expected universe and actual universe don't match. - static inline void sanityCheckUniverse(const Option::Universe expected_universe, - const Option::Universe actual_universe); - - /// @brief Check if the option definition is valid. - /// - /// @todo: list exceptions it throws. - void validate() const; + static inline void sanityCheckUniverse(const Option::Universe expected_universe, + const Option::Universe actual_universe); /// Option name. std::string name_; diff --git a/src/lib/dhcp/tests/option6_int_array_unittest.cc b/src/lib/dhcp/tests/option6_int_array_unittest.cc index 26af72608e..4e58c3d6ec 100644 --- a/src/lib/dhcp/tests/option6_int_array_unittest.cc +++ b/src/lib/dhcp/tests/option6_int_array_unittest.cc @@ -19,9 +19,6 @@ #include #include -#include -#include -#include #include #include diff --git a/src/lib/dhcp/tests/option6_int_unittest.cc b/src/lib/dhcp/tests/option6_int_unittest.cc index 906b954077..1dfc20595a 100644 --- a/src/lib/dhcp/tests/option6_int_unittest.cc +++ b/src/lib/dhcp/tests/option6_int_unittest.cc @@ -19,9 +19,6 @@ #include #include -#include -#include -#include #include #include diff --git a/src/lib/dhcp/tests/option_definition_unittest.cc b/src/lib/dhcp/tests/option_definition_unittest.cc index 229771b68f..c8df21d0a8 100644 --- a/src/lib/dhcp/tests/option_definition_unittest.cc +++ b/src/lib/dhcp/tests/option_definition_unittest.cc @@ -13,25 +13,23 @@ // PERFORMANCE OF THIS SOFTWARE. #include -#include -#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include #include #include #include -#include -#include -#include "dhcp/dhcp4.h" -#include "dhcp/dhcp6.h" -#include "dhcp/option4_addrlst.h" -#include "dhcp/option6_addrlst.h" -#include "dhcp/option6_ia.h" -#include "dhcp/option6_iaaddr.h" -#include "dhcp/option6_int.h" -#include "dhcp/option6_int_array.h" -#include "dhcp/option_definition.h" - using namespace std; using namespace isc; using namespace isc::dhcp; @@ -57,6 +55,7 @@ TEST_F(OptionDefinitionTest, constructor) { EXPECT_EQ(1, opt_def1.getCode()); EXPECT_EQ(OptionDefinition::STRING_TYPE, opt_def1.getType()); EXPECT_FALSE(opt_def1.getArrayType()); + EXPECT_NO_THROW(opt_def1.validate()); // Specify the option data type as an enum value. OptionDefinition opt_def2("OPTION_RAPID_COMMIT", 14, @@ -65,6 +64,7 @@ TEST_F(OptionDefinitionTest, constructor) { EXPECT_EQ(14, opt_def2.getCode()); EXPECT_EQ(OptionDefinition::EMPTY_TYPE, opt_def2.getType()); EXPECT_FALSE(opt_def2.getArrayType()); + EXPECT_NO_THROW(opt_def1.validate()); // Check if it is possible to set that option is an array. OptionDefinition opt_def3("OPTION_NIS_SERVERS", 27, @@ -74,6 +74,7 @@ TEST_F(OptionDefinitionTest, constructor) { EXPECT_EQ(27, opt_def3.getCode()); EXPECT_EQ(OptionDefinition::IPV6_ADDRESS_TYPE, opt_def3.getType()); EXPECT_TRUE(opt_def3.getArrayType()); + EXPECT_NO_THROW(opt_def3.validate()); // The created object is invalid if invalid data type is specified but // constructor shouldn't throw exception. The object is validated after @@ -121,6 +122,31 @@ TEST_F(OptionDefinitionTest, addRecordField) { EXPECT_THROW(opt_def.addRecordField(invalid_type), isc::BadValue); } +TEST_F(OptionDefinitionTest, validate) { + // Not supported option type string is not allowed. + OptionDefinition opt_def1("OPTION_CLIENTID", D6O_CLIENTID, "non-existent-type"); + EXPECT_THROW(opt_def1.validate(), isc::OutOfRange); + + // Not supported option type enum value is not allowed. + OptionDefinition opt_def2("OPTION_CLIENTID", D6O_CLIENTID, OptionDefinition::UNKNOWN_TYPE); + EXPECT_THROW(opt_def2.validate(), isc::OutOfRange); + + OptionDefinition opt_def3("OPTION_CLIENTID", D6O_CLIENTID, + static_cast(OptionDefinition::UNKNOWN_TYPE + + 2)); + EXPECT_THROW(opt_def3.validate(), isc::OutOfRange); + + // Empty option name is not allowed. + OptionDefinition opt_def4("", D6O_CLIENTID, "string"); + EXPECT_THROW(opt_def4.validate(), isc::BadValue); + + // Option name must not contain spaces. + OptionDefinition opt_def5(" OPTION_CLIENTID", D6O_CLIENTID, "string"); + EXPECT_THROW(opt_def5.validate(), isc::BadValue); + + OptionDefinition opt_def6("OPTION CLIENTID", D6O_CLIENTID, "string"); + EXPECT_THROW(opt_def6.validate(), isc::BadValue); +} TEST_F(OptionDefinitionTest, factoryAddrList6) { OptionDefinition opt_def("OPTION_NIS_SERVERS", D6O_NIS_SERVERS,