[2491] Changes as a result of the review.

src/bin/dhcp6/dhcp6_srv.cc

@@ -368,7 +368,7 @@ OptionPtr Dhcpv6Srv::createStatusCode(uint16_t code, const std::string& text) {
     assert(option_status);
 
     // Set status code to 'code' (0 - means data field #0).
-    option_status->writeInteger<uint16_t>(code, 0);
+    option_status->writeInteger(code, 0);
     // Set a message (1 - means data field #1).
     option_status->writeString(text, 1);
     return (option_status);

src/lib/dhcp/libdhcp++.cc

@@ -340,28 +340,29 @@ LibDHCP::initStdOptionDefs6() {
         // Some of the options comprise a "record" of data fields so
         // we have to add those fields here.
         switch(params[i].code) {
-        case D6O_IA_NA:
-        case D6O_IA_PD:
-            for (int j = 0; j < 3; ++j) {
-                definition->addRecordField(OPT_UINT32_TYPE);
-            }
+        case D6O_CLIENT_FQDN:
+            definition->addRecordField(OPT_UINT8_TYPE);
+            definition->addRecordField(OPT_FQDN_TYPE);
+            break;
+        case D6O_GEOCONF_CIVIC:
+            definition->addRecordField(OPT_UINT8_TYPE);
+            definition->addRecordField(OPT_UINT16_TYPE);
+            definition->addRecordField(OPT_BINARY_TYPE);
             break;
         case D6O_IAADDR:
             definition->addRecordField(OPT_IPV6_ADDRESS_TYPE);
             definition->addRecordField(OPT_UINT32_TYPE);
             definition->addRecordField(OPT_UINT32_TYPE);
             break;
-        case D6O_STATUS_CODE:
-            definition->addRecordField(OPT_UINT16_TYPE);
-            definition->addRecordField(OPT_STRING_TYPE);
-            break;
-        case D6O_VENDOR_CLASS:
+        case D6O_IA_NA:
+            definition->addRecordField(OPT_UINT32_TYPE);
+            definition->addRecordField(OPT_UINT32_TYPE);
             definition->addRecordField(OPT_UINT32_TYPE);
-            definition->addRecordField(OPT_BINARY_TYPE);
             break;
-        case D6O_VENDOR_OPTS:
+        case D6O_IA_PD:
+            definition->addRecordField(OPT_UINT32_TYPE);
+            definition->addRecordField(OPT_UINT32_TYPE);
             definition->addRecordField(OPT_UINT32_TYPE);
-            definition->addRecordField(OPT_BINARY_TYPE);
             break;
         case D6O_IAPREFIX:
             definition->addRecordField(OPT_UINT32_TYPE);
@@ -369,25 +370,28 @@ LibDHCP::initStdOptionDefs6() {
             definition->addRecordField(OPT_UINT8_TYPE);
             definition->addRecordField(OPT_BINARY_TYPE);
             break;
-        case D6O_GEOCONF_CIVIC:
+        case D6O_LQ_QUERY:
             definition->addRecordField(OPT_UINT8_TYPE);
-            definition->addRecordField(OPT_UINT16_TYPE);
+            definition->addRecordField(OPT_IPV6_ADDRESS_TYPE);
+            break;
+        case D6O_LQ_RELAY_DATA:
+            definition->addRecordField(OPT_IPV6_ADDRESS_TYPE);
             definition->addRecordField(OPT_BINARY_TYPE);
             break;
         case D6O_REMOTE_ID:
             definition->addRecordField(OPT_UINT32_TYPE);
             definition->addRecordField(OPT_BINARY_TYPE);
             break;
-        case D6O_CLIENT_FQDN:
-            definition->addRecordField(OPT_UINT8_TYPE);
-            definition->addRecordField(OPT_FQDN_TYPE);
+        case D6O_STATUS_CODE:
+            definition->addRecordField(OPT_UINT16_TYPE);
+            definition->addRecordField(OPT_STRING_TYPE);
             break;
-        case D6O_LQ_QUERY:
-            definition->addRecordField(OPT_UINT8_TYPE);
-            definition->addRecordField(OPT_IPV6_ADDRESS_TYPE);
+        case D6O_VENDOR_CLASS:
+            definition->addRecordField(OPT_UINT32_TYPE);
+            definition->addRecordField(OPT_BINARY_TYPE);
             break;
-        case D6O_LQ_RELAY_DATA:
-            definition->addRecordField(OPT_IPV6_ADDRESS_TYPE);
+        case D6O_VENDOR_OPTS:
+            definition->addRecordField(OPT_UINT32_TYPE);
             definition->addRecordField(OPT_BINARY_TYPE);
             break;
         default:

src/lib/dhcp/option_custom.cc

@@ -127,7 +127,16 @@ OptionCustom::createBuffers() {
     std::vector<OptionBuffer> buffers;
 
     OptionDataType data_type = definition_.getType();
+    // This function is called when an empty data buffer has been
+    // passed to the constructor. In such cases values for particular
+    // data fields will be set using modifier functions but for now
+    // we need to initialize a set of buffers that are specified
+    // for an option by its definition. Since there is no data yet,
+    // we are going to fill these buffers with default values.
     if (data_type == OPT_RECORD_TYPE) {
+        // For record types we need to iterate over all data fields
+        // specified in option definition and create corresponding
+        // buffers for each of them.
         const OptionDefinition::RecordFieldsCollection fields =
             definition_.getRecordFields();
 
@@ -135,28 +144,52 @@ OptionCustom::createBuffers() {
              field != fields.end(); ++field) {
             OptionBuffer buf;
 
+            // For data types that have a fixed size we can use the
+            // utility function to get the buffer's size.
             size_t data_size = OptionDataTypeUtil::getDataTypeLen(*field);
 
+            // For variable data sizes the utility function returns zero.
+            // It is ok for string values because the default string
+            // is 'empty'. However for FQDN the empty value is not valid
+            // so we initialize it to '.'.
             if (data_size == 0 &&
                 *field == OPT_FQDN_TYPE) {
                 OptionDataTypeUtil::writeFqdn(".", buf);
             } else {
+                // At this point we can resize the buffer. Note that
+                // for string values we are setting the empty buffer
+                // here.
                 buf.resize(data_size);
             }
+            // We have the buffer with default value prepared so we
+            // add it to the set of buffers.
             buffers.push_back(buf);
         }
     } else if (!definition_.getArrayType() &&
                data_type != OPT_EMPTY_TYPE) {
+        // For either 'empty' options we don't have to create any buffers
+        // for obvious reason. For arrays we also don't create any buffers
+        // yet because the set of fields that belong to the array is open
+        // ended so we can't allocate required buffers until we know how
+        // many of them are needed.
+        // For non-arrays we have a single value being held by the option
+        // so we have to allocate exactly one buffer.
         OptionBuffer buf;
         size_t data_size = OptionDataTypeUtil::getDataTypeLen(data_type);
         if (data_size == 0 &&
             data_type == OPT_FQDN_TYPE) {
             OptionDataTypeUtil::writeFqdn(".", buf);
         } else {
+            // Note that if our option holds a string value then
+            // we are making empty buffer here.
             buf.resize(data_size);
         }
+        // Add a buffer that we have created and leave.
         buffers.push_back(buf);
     }
+    // The 'swap' is used here because we want to make sure that we
+    // don't touch buffers_ until we successfully allocate all
+    // buffers to be stored there.
     std::swap(buffers, buffers_);
 }
 
@@ -193,8 +226,12 @@ OptionCustom::createBuffers(const OptionBuffer& data_buf) {
                 // to obtain the length of the data is to read the FQDN. The
                 // utility function will return the size of the buffer on success.
                 if (*field == OPT_FQDN_TYPE) {
-                    OptionDataTypeUtil::readFqdn(OptionBuffer(data, data_buf.end()),
-                                                 data_size);
+                    std::string fqdn =
+                        OptionDataTypeUtil::readFqdn(OptionBuffer(data, data_buf.end()));
+                    // The size of the buffer holding an FQDN is always
+                    // 1 byte larger than the size of the string
+                    // representation of this FQDN.
+                    data_size = fqdn.size() + 1;
                 } else {
                     // In other case we are dealing with string or binary value
                     // which size can't be determined. Thus we consume the
@@ -248,8 +285,12 @@ OptionCustom::createBuffers(const OptionBuffer& data_buf) {
                 // a buffer so we have to actually read the FQDN from a buffer
                 // to get it.
                 if (data_type == OPT_FQDN_TYPE) {
-                    OptionDataTypeUtil::readFqdn(OptionBuffer(data, data_buf.end()),
-                                                 data_size);
+                    std::string fqdn =
+                        OptionDataTypeUtil::readFqdn(OptionBuffer(data, data_buf.end()));
+                    // The size of the buffer holding an FQDN is always
+                    // 1 byte larger than the size of the string
+                    // representation of this FQDN.
+                    data_size = fqdn.size() + 1;
                 }
                 // We don't perform other checks for data types that can't be
                 // used together with array indicator such as strings, empty field
@@ -275,8 +316,12 @@ OptionCustom::createBuffers(const OptionBuffer& data_buf) {
             if (data_size == 0) {
                 // For FQDN we get the size by actually reading the FQDN.
                 if (data_type == OPT_FQDN_TYPE) {
-                    OptionDataTypeUtil::readFqdn(OptionBuffer(data, data_buf.end()),
-                                                 data_size);
+                    std::string fqdn =
+                        OptionDataTypeUtil::readFqdn(OptionBuffer(data, data_buf.end()));
+                    // The size of the buffer holding an FQDN is always
+                    // 1 bytes larger than the size of the string
+                    // representation of this FQDN.
+                    data_size = fqdn.size() + 1;
                 } else {
                     data_size = std::distance(data, data_buf.end());
                 }
@@ -454,8 +499,7 @@ OptionCustom::writeBoolean(const bool value, const uint32_t index) {
 std::string
 OptionCustom::readFqdn(const uint32_t index) const {
     checkIndex(index);
-    size_t len = 0;
-    return (OptionDataTypeUtil::readFqdn(buffers_[index], len));
+    return (OptionDataTypeUtil::readFqdn(buffers_[index]));
 }
 
 void

src/lib/dhcp/option_custom.h

@@ -200,7 +200,7 @@ public:
     /// @return read integer value.
     template<typename T>
     T readInteger(const uint32_t index = 0) const {
-        // Check thet tha index is not out of range.
+        // Check that the index is not out of range.
         checkIndex(index);
         // Check that T points to a valid integer type and this type
         // is consistent with an option definition.

src/lib/dhcp/option_data_types.cc

@@ -209,26 +209,20 @@ OptionDataTypeUtil::writeBool(const bool value,
 }
 
 std::string
-OptionDataTypeUtil::readFqdn(const std::vector<uint8_t>& buf,
-                             size_t& len) {
-    len = 0;
-
+OptionDataTypeUtil::readFqdn(const std::vector<uint8_t>& buf) {
     // If buffer is empty emit an error.
     if (buf.empty()) {
         isc_throw(BadDataTypeCast, "unable to read FQDN from a buffer."
                   << " The buffer is empty.");
     }
     // Copy the data from a buffer to InputBuffer so as we can use
-    // isc::dns::Name object to get the FQDN. This is not the most
-    // efficient way to do it but currently there is no construtor
-    // in Name that would use std::vector directly.
+    // isc::dns::Name object to get the FQDN.
     isc::util::InputBuffer in_buf(static_cast<const void*>(&buf[0]), buf.size());
     try {
         // Try to create an object from the buffer. If exception is thrown
         // it means that the buffer doesn't hold a valid domain name (invalid
         // syntax).
         isc::dns::Name name(in_buf);
-        len = name.getLength();
         return (name.toText());
     } catch (const isc::Exception& ex) {
         // Unable to convert the data in the buffer into FQDN.
@@ -246,7 +240,7 @@ OptionDataTypeUtil::writeFqdn(const std::string& fqdn,
             buf.resize(buf.size() + labels.getDataLength());
             size_t read_len = 0;
             const uint8_t* data = labels.getData(&read_len);
-            memcpy(static_cast<void*>(&buf[buf.size() - labels.getDataLength()]),
+            memcpy(static_cast<void*>(&buf[buf.size() - read_len]),
                    data, read_len);
         }
     } catch (const isc::Exception& ex) {

src/lib/dhcp/option_data_types.h

@@ -351,13 +351,11 @@ public:
     /// section 3.1.
     ///
     /// @param buf input buffer holding a FQDN.
-    /// @param [out] len number of bytes read from a buffer.
     ///
     /// @throw BadDataTypeCast if a FQDN stored within a buffer is
     /// invalid (e.g. empty, contains invalid characters, truncated).
     /// @return fully qualified domain name in a text form.
-    static std::string readFqdn(const std::vector<uint8_t>& buf,
-                                size_t& len);
+    static std::string readFqdn(const std::vector<uint8_t>& buf);
 
     /// @brief Append FQDN into a buffer.
     ///

src/lib/dhcp/option_definition.cc

@@ -90,7 +90,7 @@ OptionDefinition::optionFactory(Option::Universe u, uint16_t type,
 
         case OPT_UINT8_TYPE:
             return (array_type_ ? factoryGeneric(u, type, begin, end) :
-                    factoryInteger<uint8_t>(u, type, begin, end));;
+                    factoryInteger<uint8_t>(u, type, begin, end));
 
         case OPT_INT8_TYPE:
             return (array_type_ ? factoryGeneric(u, type, begin, end) :
@@ -113,27 +113,44 @@ OptionDefinition::optionFactory(Option::Universe u, uint16_t type,
                     factoryInteger<int32_t>(u, type, begin, end));
 
         case OPT_IPV4_ADDRESS_TYPE:
+            // If definition specifies that an option is an array
+            // of IPv4 addresses we return an instance of specialized
+            // class (OptionAddrLst4). For non-array types there is no
+            // specialized class yet implemented so we drop through
+            // to return an instance of OptionCustom.
             if (array_type_) {
                 return (factoryAddrList4(type, begin, end));
             }
             break;
 
         case OPT_IPV6_ADDRESS_TYPE:
+            // Handle array type only here (see comments for
+            // OPT_IPV4_ADDRESS_TYPE case).
             if (array_type_) {
                 return (factoryAddrList6(type, begin, end));
             }
             break;
 
         default:
-            if (u == Option::V6 &&
-                (code_ == D6O_IA_NA || code_ == D6O_IA_PD) &&
-                haveIA6Format()) {
-                return (factoryIA6(type, begin, end));
-
-            } else if (u == Option::V6 &&
-                       code_ == D6O_IAADDR &&
-                       haveIAAddr6Format()) {
-                return (factoryIAAddr6(type, begin, end));
+            if (u == Option::V6) {
+                if ((code_ == D6O_IA_NA || code_ == D6O_IA_PD) &&
+                    haveIA6Format()) {
+                    // Return Option6IA instance for IA_PD and IA_NA option
+                    // types only. We don't want to return Option6IA for other
+                    // options that comprise 3 UINT32 data fields because
+                    // Option6IA accessors' and modifiers' names are derived
+                    // from the IA_NA and IA_PD options' field names: IAID,
+                    // T1, T2. Using functions such as getIAID, getT1 etc. for
+                    // options other than IA_NA and IA_PD would be bad practice
+                    // and cause confusion.
+                    return (factoryIA6(type, begin, end));
+
+                } else if (code_ == D6O_IAADDR && haveIAAddr6Format()) {
+                    // Rerurn Option6IAAddr option instance for the IAADDR
+                    // option only for the same reasons as described in
+                    // for IA_NA and IA_PD above.
+                    return (factoryIAAddr6(type, begin, end));
+                }
             }
         }
         return (OptionPtr(new OptionCustom(*this, u, OptionBuffer(begin, end))));

src/lib/dhcp/tests/libdhcp++_unittest.cc

@@ -69,8 +69,8 @@ public:
                              const std::type_info& expected_type) {
         // Get all option definitions, we will use them to extract
         // the definition for a particular option code.
-        // We don't have to initialize option deinitions here because they
-        // are initialized in the class'es constructor.
+        // We don't have to initialize option definitions here because they
+        // are initialized in the class's constructor.
         OptionDefContainer options = LibDHCP::getOptionDefs(Option::V6);
         // Get the container index #1. This one allows for searching
         // option definitions using option code.

src/lib/dhcp/tests/option_custom_unittest.cc

@@ -686,11 +686,11 @@ TEST_F(OptionCustomTest, recordData) {
     };
 
     OptionBuffer buf;
-    // Initialize field 0.
+    // Initialize field 0 to 8712.
     writeInt<uint16_t>(8712, buf);
     // Initialize field 1 to 'true'
     buf.push_back(static_cast<unsigned short>(1));
-    // Initialize field 2.
+    // Initialize field 2 to 'mydomain.example.com'.
     buf.insert(buf.end(), fqdn_data, fqdn_data + sizeof(fqdn_data));
     // Initialize field 3 to IPv4 address.
     writeAddress(IOAddress("192.168.0.1"), buf);
@@ -700,18 +700,12 @@ TEST_F(OptionCustomTest, recordData) {
     writeString("ABCD", buf);
 
     boost::scoped_ptr<OptionCustom> option;
-    try {
-        option.reset(new OptionCustom(opt_def, Option::V6, buf.begin(), buf.end()));
-    } catch (const Exception& ex) {
-        std::cout << ex.what() << std::endl;
-    }
-
     ASSERT_NO_THROW(
          option.reset(new OptionCustom(opt_def, Option::V6, buf.begin(), buf.end()));
     );
     ASSERT_TRUE(option);
 
-    // We should have 5 data fields.
+    // We should have 6 data fields.
     ASSERT_EQ(6, option->getDataFieldsNum());
 
     // Verify value in the field 0.
@@ -739,7 +733,7 @@ TEST_F(OptionCustomTest, recordData) {
     ASSERT_NO_THROW(value4 = option->readAddress(4));
     EXPECT_EQ("2001:db8:1::1", value4.toText());
 
-    // Verify value in the field 4.
+    // Verify value in the field 5.
     std::string value5;
     ASSERT_NO_THROW(value5 = option->readString(5));
     EXPECT_EQ("ABCD", value5);
@@ -880,7 +874,7 @@ TEST_F(OptionCustomTest, setUint32Data) {
     EXPECT_EQ(1234, value);
 }
 
-// The purpose of this test is to verify that an opton comprising
+// The purpose of this test is to verify that an option comprising
 // single IPv4 address can be created and that this address can
 // be overriden by a new value.
 TEST_F(OptionCustomTest, setIpv4AddressData) {

src/lib/dhcp/tests/option_data_types_unittest.cc

@@ -216,6 +216,9 @@ TEST_F(OptionDataTypesTest, writeBool) {
     // 'true' value being written.
     ASSERT_NO_THROW(OptionDataTypeUtil::writeBool(false, buf));
     ASSERT_EQ(2, buf.size());
+    // Check that the first value has not changed.
+    EXPECT_EQ(buf[0], 1);
+    // Check the the second value is correct.
     EXPECT_EQ(buf[1], 0);
 }
 
@@ -330,17 +333,11 @@ TEST_F(OptionDataTypesTest, writeInt) {
     // integer value. Eventually the buffer holds all values and should
     // match with the reference buffer.
     std::vector<uint8_t> buf;
-    // Write uint8_t
     ASSERT_NO_THROW(OptionDataTypeUtil::writeInt<uint8_t>(127, buf));
-    // Write uint16_t
     ASSERT_NO_THROW(OptionDataTypeUtil::writeInt<uint16_t>(1023, buf));
-    // Write uint32_t
     ASSERT_NO_THROW(OptionDataTypeUtil::writeInt<uint32_t>(4096, buf));
-    // Write int32_t
     ASSERT_NO_THROW(OptionDataTypeUtil::writeInt<int32_t>(-1024, buf));
-    // Write int16_t
     ASSERT_NO_THROW(OptionDataTypeUtil::writeInt<int16_t>(512, buf));
-    // Write int8_t
     ASSERT_NO_THROW(OptionDataTypeUtil::writeInt<int8_t>(-127, buf));
 
     // Make sure that the buffer has the same size as the reference
@@ -372,10 +369,8 @@ TEST_F(OptionDataTypesTest, readFqdn) {
 
     // Read the buffer as FQDN and verify its correctness.
     std::string fqdn;
-    size_t len = 0;
-    EXPECT_NO_THROW(fqdn = OptionDataTypeUtil::readFqdn(buf, len));
+    EXPECT_NO_THROW(fqdn = OptionDataTypeUtil::readFqdn(buf));
     EXPECT_EQ("mydomain.example.com.", fqdn);
-    EXPECT_EQ(len, buf.size());
 
     // By resizing the buffer we simulate truncation. The first
     // length field (8) indicate that the first label's size is
@@ -383,14 +378,14 @@ TEST_F(OptionDataTypesTest, readFqdn) {
     // fails.
     buf.resize(5);
     EXPECT_THROW(
-        OptionDataTypeUtil::readFqdn(buf, len),
+        OptionDataTypeUtil::readFqdn(buf),
         isc::dhcp::BadDataTypeCast
     );
 
     // Another special case: provide an empty buffer.
     buf.clear();
     EXPECT_THROW(
-        OptionDataTypeUtil::readFqdn(buf, len),
+        OptionDataTypeUtil::readFqdn(buf),
         isc::dhcp::BadDataTypeCast
     );
 }