[2490] Changes as a result of review.

src/lib/dhcp/libdhcp++.cc

@@ -122,7 +122,7 @@ size_t LibDHCP::unpackOptions6(const OptionBuffer& buf,
                       " for the same option code. This will be supported once"
                       " support for option spaces is implemented");
         } else if (num_defs == 0) {
-            // Don't crash if definition does not exist because only a few
+            // @todo Don't crash if definition does not exist because only a few
             // option definitions are initialized right now. In the future
             // we will initialize definitions for all options and we will
             // remove this elseif. For now, return generic option.
@@ -303,11 +303,12 @@ LibDHCP::initStdOptionDefs6() {
         try {
             definition->validate();
         } catch (const Exception& ex) {
-            // Return empty set in an unlikely event of a validation error.
-            // We don't return exception here on error because it should
-            // not happen to the regular user. It is a programming error.
+            // This is unlikely event that validation fails and may
+            // be only caused by programming error. To guarantee the
+            // data consistency we clear all option definitions that
+            // have been added so far and pass the exception forward.
             v6option_defs_.clear();
-            return;
+            throw;
         }
         v6option_defs_.push_back(definition);
     }

src/lib/dhcp/libdhcp++.h

@@ -137,6 +137,8 @@ private:
     /// The method creates option definitions for all DHCPv6 options.
     ///
     /// @throw std::bad_alloc if system went out of memory.
+    /// @throw MalformedOptionDefinition if any of the definitions
+    /// is incorect. This is a programming error.
     static void initStdOptionDefs6();
 
     /// pointers to factories that produce DHCPv6 options

src/lib/dhcp/option_data_types.h

@@ -147,7 +147,14 @@ struct OptionDataTypeTraits<uint32_t> {
 template<>
 struct OptionDataTypeTraits<asiolink::IOAddress> {
     static const bool valid = true;
-    static const int len = sizeof(asiolink::IOAddress);
+    // The len value is used to determine the size of the data
+    // to be written to an option buffer. IOAddress object may
+    // either represent an IPv4 or IPv6 addresses which have
+    // different lengths. Thus we can't put fixed value here.
+    // The length of a data to be written into an option buffer
+    // have to be determined in the runtime for a particular
+    // IOAddress object. Thus setting len to zero.
+    static const int len = 0;
     static const bool integer_type = false;
     static const OptionDataType type = OPT_ANY_ADDRESS_TYPE;
 };
@@ -156,7 +163,11 @@ struct OptionDataTypeTraits<asiolink::IOAddress> {
 template<>
 struct OptionDataTypeTraits<std::string> {
     static const bool valid = true;
-    static const int len = sizeof(std::string);
+    // The len value is used to determine the size of the data
+    // to be written to an option buffer. For strings this
+    // size is unknown until we actually deal with the particular
+    // string to be written. Thus setting it to zero.
+    static const int len = 0;
     static const bool integer_type = false;
     static const OptionDataType type = OPT_STRING_TYPE;
 };

src/lib/dhcp/option_definition.cc

@@ -157,7 +157,7 @@ OptionDefinition::DataTypeUtil::writeToBuffer(const std::string& value,
         }
     case OPT_UINT8_TYPE:
         {
-            buf.push_back(lexicalCastWithRangeCheck<int8_t>(value));
+            buf.push_back(lexicalCastWithRangeCheck<uint8_t>(value));
             return;
         }
     case OPT_UINT16_TYPE:
@@ -212,7 +212,7 @@ OptionDefinition::DataTypeUtil::writeToBuffer(const std::string& value,
             // Increase the size of the storage by the size of the string.
             buf.resize(buf.size() + value.size());
             // Assuming that the string is already UTF8 encoded.
-            std::copy_backward(value.c_str(), value.c_str() + value.length(),
+            std::copy_backward(value.c_str(), value.c_str() + value.size(),
                                buf.end());
             return;
         }
@@ -288,40 +288,50 @@ OptionDefinition::optionFactory(Option::Universe u, uint16_t type,
     try {
         if (type_ == OPT_BINARY_TYPE) {
             return (factoryGeneric(u, type, begin, end));
+
         } else if (type_ == OPT_IPV6_ADDRESS_TYPE && array_type_) {
             return (factoryAddrList6(type, begin, end));
+
         } else if (type_ == OPT_IPV4_ADDRESS_TYPE && array_type_) {
             return (factoryAddrList4(type, begin, end));
+
         } else if (type_ == OPT_EMPTY_TYPE) {
             return (factoryEmpty(u, type));
+
         } else if (u == Option::V6 &&
                    code_ == D6O_IA_NA &&
                    haveIA6Format()) {
             return (factoryIA6(type, begin, end));
+
         } else if (u == Option::V6 &&
                    code_ == D6O_IAADDR &&
                    haveIAAddr6Format()) {
             return (factoryIAAddr6(type, begin, end));
+
         } else if (type_ == OPT_UINT8_TYPE) {
             if (array_type_) {
                 return (factoryGeneric(u, type, begin, end));
             } else {
                 return (factoryInteger<uint8_t>(u, type, begin, end));
             }
+
         } else if (type_ == OPT_UINT16_TYPE) {
             if (array_type_) {
                 return (factoryIntegerArray<uint16_t>(type, begin, end));
             } else {
                 return (factoryInteger<uint16_t>(u, type, begin, end));
             }
+
         } else if (type_ == OPT_UINT32_TYPE) {
             if (array_type_) {
                 return (factoryIntegerArray<uint32_t>(type, begin, end));
             } else {
                 return (factoryInteger<uint32_t>(u, type, begin, end));
             }
+
         }
         return (factoryGeneric(u, type, begin, end));
+
     } catch (const Exception& ex) {
         isc_throw(InvalidOptionValue, ex.what());
     }