[2304] Code cleanup.

diff --git a/src/lib/dhcp/option6_int.h b/src/lib/dhcp/option6_int.h
index e0df4c5f4ea10c9dfdba85f6f8497ea96de1a010..3401fac9a1c71abc89e89b5a79e51203dbce407d 100644 (file)
--- 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 <dhcp/libdhcp++.h>
+#include <dhcp/option.h>
+#include <dhcp/option_data_types.h>
+#include <util/io_utilities.h>
+
 #include <stdint.h>
 #include <limits>
-#include <util/io_utilities.h>
-#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 b87f538ca57e243ebb34b1771cf01ba4f88208e5..2447dac92afc2c14813ba44bb3bb81c0040f58d1 100644 (file)
--- 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 <dhcp/libdhcp++.h>
+#include <dhcp/option.h>
+#include <dhcp/option_data_types.h>
+#include <util/io_utilities.h>
+
 #include <stdint.h>
 #include <limits>
-#include <util/io_utilities.h>
-#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<T>& 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 52be59f97ab90bae9d95f235e483bdd1538e6b66..4e8d8a6faa1e37f658224682f2d845aab18c817a 100644 (file)
--- 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 <stdint.h>
 #include <exceptions/exceptions.h>
 
+#include <stdint.h>
+
 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<typename T>
 struct OptionDataTypes {
     static const bool valid = false;
     static const int len = 0;
 };
 
+/// int8_t type is supported.
 template<>
 struct OptionDataTypes<int8_t> {
     static const bool valid = true;
     static const int len = 1;
 };
 
+/// int16_t type is supported.
 template<>
 struct OptionDataTypes<int16_t> {
     static const bool valid = true;
     static const int len = 2;
 };
 
+/// int32_t type is supported.
 template<>
 struct OptionDataTypes<int32_t> {
     static const bool valid = true;
     static const int len = 4;
 };
 
+/// uint8_t type is supported.
 template<>
 struct OptionDataTypes<uint8_t> {
     static const bool valid = true;
     static const int len = 1;
 };
 
+/// uint16_t type is supported.
 template<>
 struct OptionDataTypes<uint16_t> {
     static const bool valid = true;
     static const int len = 2;
 };
 
+/// uint32_t type is supported.
 template<>
 struct OptionDataTypes<uint32_t> {
     static const bool valid = true;

diff --git a/src/lib/dhcp/option_definition.cc b/src/lib/dhcp/option_definition.cc
index 07305b9efff766cdcae2edca5ac576876c35a08d..49c5642baf6bde9e84663a36ecb279e02437d0c8 100644 (file)
--- 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 <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>
 
 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<uint32_t>);
         }
     }
+    // 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 764fe85b0eefc7a2c4ddc35ed2058856db2fa760..ad0598cd2a50de0e49e5d48b8cc1b3f8fd2e96b6 100644 (file)
--- 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 <dhcp/option_data_types.h>
+#include <dhcp/option6_int.h>
+#include <dhcp/option6_int_array.h>
+#include <dhcp/option.h>
 
 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<typename T>
     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<typename T>
     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 26af72608eef9e1256c68a2fc3f9a591b14454d1..4e58c3d6ec552575eb2ac496f8c5ff67eee50950 100644 (file)
--- 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 <dhcp/option6_iaaddr.h>
 #include <util/buffer.h>
 
-#include <iostream>
-#include <sstream>
-#include <arpa/inet.h>
 #include <boost/pointer_cast.hpp>
 #include <gtest/gtest.h>
 

diff --git a/src/lib/dhcp/tests/option6_int_unittest.cc b/src/lib/dhcp/tests/option6_int_unittest.cc
index 906b954077748d865305a1bd78dec7981a47f949..1dfc20595a75ab287439d36669cc0e7a695c5090 100644 (file)
--- a/src/lib/dhcp/tests/option6_int_unittest.cc
+++ b/src/lib/dhcp/tests/option6_int_unittest.cc
@@ -19,9 +19,6 @@
 #include <dhcp/option6_iaaddr.h>
 #include <util/buffer.h>
 
-#include <iostream>
-#include <sstream>
-#include <arpa/inet.h>
 #include <boost/pointer_cast.hpp>
 #include <gtest/gtest.h>
 

diff --git a/src/lib/dhcp/tests/option_definition_unittest.cc b/src/lib/dhcp/tests/option_definition_unittest.cc
index 229771b68f4360078293fad9c948bc9a39b8fcbc..c8df21d0a8f0d33e87768ba936479b77b6e2bd62 100644 (file)
--- 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 <config.h>
-#include <iostream>
-#include <sstream>
+
+#include <exceptions/exceptions.h>
+#include <asiolink/io_address.h>
+#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>
 
 #include <gtest/gtest.h>
 #include <boost/shared_ptr.hpp>
 #include <boost/pointer_cast.hpp>
 
-#include <exceptions/exceptions.h>
-#include <asiolink/io_address.h>
-#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::DataType>(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,