[2312] Changes to the code as a result of review.

src/lib/dhcp/option_custom.cc

@@ -99,9 +99,12 @@ OptionCustom::createBuffers() {
             // Proceed to the next data field.
             data += data_size;
         }
-    } else {
-        // If option definition type is not a 'record' than definition
-        // type itself indicates what type of data is being held there.
+    } else if (data_type != OPT_EMPTY_TYPE) {
+        // If data_type value is other than OPT_RECORD_TYPE, our option is
+        // empty (have no data at all) or it comprises one or more
+        // data fields of the same type. The type of those fields
+        // is held in the data_type variable so let's use it to determine
+        // a size of buffers.
         int data_size = OptionDataTypeUtil::getDataTypeLen(data_type);
         // The check below will fail if the input buffer is too short
         // for the data size being held by this option.
@@ -122,7 +125,9 @@ OptionCustom::createBuffers() {
             assert(data_size > 0);
             // Get equal chunks of data and store as collection of buffers.
             // Truncate any remaining part which length is not divisible by
-            // data_size.
+            // data_size. Note that it is ok to truncate the data if and only
+            // if the data buffer is long enough to keep at least one value.
+            // This has been checked above already.
             do {
                 buffers.push_back(OptionBuffer(data, data + data_size));
                 data += data_size;
@@ -134,9 +139,9 @@ OptionCustom::createBuffers() {
             if (data_size == 0) {
                 data_size = std::distance(data, data_.end());
             }
-            if (data_size > 0 && data_type != OPT_EMPTY_TYPE) {
+            if (data_size > 0) {
                 buffers.push_back(OptionBuffer(data, data + data_size));
-            } else if (data_type != OPT_EMPTY_TYPE) {
+            } else {
                 isc_throw(OutOfRange, "option buffer truncated");
             }
         }
@@ -148,57 +153,49 @@ OptionCustom::createBuffers() {
 std::string
 OptionCustom::dataFieldToText(const OptionDataType data_type,
                               const uint32_t index) const {
-    std::ostringstream tmp;
+    std::ostringstream text;
 
     // Get the value of the data field.
     switch (data_type) {
     case OPT_BINARY_TYPE:
-        tmp << util::encode::encodeHex(readBinary(index));
+        text << util::encode::encodeHex(readBinary(index));
         break;
     case OPT_BOOLEAN_TYPE:
-        tmp << (readBoolean(index) ? "true" : "false");
+        text << (readBoolean(index) ? "true" : "false");
         break;
     case OPT_INT8_TYPE:
-        tmp << readInteger<int8_t>(index);
+        text << readInteger<int8_t>(index);
         break;
     case OPT_INT16_TYPE:
-        tmp << readInteger<int16_t>(index);
+        text << readInteger<int16_t>(index);
         break;
     case OPT_INT32_TYPE:
-        tmp << readInteger<int32_t>(index);
+        text << readInteger<int32_t>(index);
         break;
     case OPT_UINT8_TYPE:
-        tmp << readInteger<uint8_t>(index);
+        text << readInteger<uint8_t>(index);
         break;
     case OPT_UINT16_TYPE:
-        tmp << readInteger<uint16_t>(index);
+        text << readInteger<uint16_t>(index);
         break;
     case OPT_UINT32_TYPE:
-        tmp << readInteger<uint32_t>(index);
+        text << readInteger<uint32_t>(index);
         break;
     case OPT_IPV4_ADDRESS_TYPE:
     case OPT_IPV6_ADDRESS_TYPE:
-        {
-            asiolink::IOAddress address("127.0.0.1");
-            readAddress(index, address);
-            tmp << address.toText();
-            break;
-        }
+        text << readAddress(index).toText();
+        break;
     case OPT_STRING_TYPE:
-        {
-            std::string s;
-            readString(index, s);
-            tmp << s;
-            break;
-        }
+        text << readString(index);
+        break;
     default:
         ;
     }
 
     // Append data field type in brackets.
-    tmp << " ( " << OptionDataTypeUtil::getDataTypeName(data_type) << " ) ";
+    text << " ( " << OptionDataTypeUtil::getDataTypeName(data_type) << " ) ";
 
-    return (tmp.str());
+    return (text.str());
 }
 
 void
@@ -214,7 +211,12 @@ OptionCustom::pack4(isc::util::OutputBuffer& buf) {
     // Write data from buffers.
     for (std::vector<OptionBuffer>::const_iterator it = buffers_.begin();
          it != buffers_.end(); ++it) {
-        buf.writeData(&(*it)[0], it->size());
+        // In theory the createBuffers function should have taken
+        // care that there are no empty buffers added to the
+        // collection but it is almost always good to make sure.
+        if (it->size() > 0) {
+            buf.writeData(&(*it)[0], it->size());
+        }
     }
 
     // Write suboptions.
@@ -235,17 +237,17 @@ OptionCustom::pack6(isc::util::OutputBuffer& buf) {
     LibDHCP::packOptions(buf, options_);
 }
 
-void
-OptionCustom::readAddress(const uint32_t index, asiolink::IOAddress& address) const {
+asiolink::IOAddress
+OptionCustom::readAddress(const uint32_t index) const {
     checkIndex(index);
 
     // The address being read can be either IPv4 or IPv6. The decision
     // is made based on the buffer length. If it holds 4 bytes it is IPv4
     // address, if it holds 16 bytes it is IPv6.
     if (buffers_[index].size() == asiolink::V4ADDRESS_LEN) {
-        OptionDataTypeUtil::readAddress(buffers_[index], AF_INET, address);
+        return (OptionDataTypeUtil::readAddress(buffers_[index], AF_INET));
     } else if (buffers_[index].size() == asiolink::V6ADDRESS_LEN) {
-        OptionDataTypeUtil::readAddress(buffers_[index], AF_INET6, address);
+        return (OptionDataTypeUtil::readAddress(buffers_[index], AF_INET6));
     } else {
         isc_throw(BadDataTypeCast, "unable to read data from the buffer as"
                   << " IP address. Invalid buffer length " << buffers_[index].size());
@@ -264,10 +266,10 @@ OptionCustom::readBoolean(const uint32_t index) const {
     return (OptionDataTypeUtil::readBool(buffers_[index]));
 }
 
-void
-OptionCustom::readString(const uint32_t index, std::string& value) const {
+std::string
+OptionCustom::readString(const uint32_t index) const {
     checkIndex(index);
-    OptionDataTypeUtil::readString(buffers_[index], value);
+    return (OptionDataTypeUtil::readString(buffers_[index]));
 }
 
 void
@@ -310,7 +312,7 @@ void OptionCustom::setData(const OptionBufferConstIter first,
     createBuffers();
 }
 
-std::string OptionCustom::toText(int indent /* = 0 */ ) {
+std::string OptionCustom::toText(int indent) {
     std::stringstream tmp;
 
     for (int i = 0; i < indent; ++i)

src/lib/dhcp/option_custom.h

@@ -40,6 +40,12 @@ public:
 
     /// @brief Constructor, used for options to be sent.
     ///
+    /// This constructor creates an instance of an option from the whole
+    /// supplied buffer. This constructor is mainly used to create an
+    /// instances of options to be stored in outgoing DHCP packets.
+    /// The buffer used to create the instance of an option can be
+    /// created from the option data specified in server's configuration.
+    ///
     /// @param def option definition.
     /// @param u specifies universe (V4 or V6).
     /// @param data content of the option.
@@ -51,6 +57,13 @@ public:
 
     /// @brief Constructor, used for received options.
     ///
+    /// This constructor creates an instance an option from the portion
+    /// of the buffer specified by iterators. This is mainly useful when
+    /// parsing received packets. Such packets are represented by a single
+    /// buffer holding option data and all sub options. Methods that are
+    /// parsing a packet, supply relevant portions of the packet buffer
+    /// to this constructor to create option instances out of it.
+    ///
     /// @param def option definition.
     /// @param u specifies universe (V4 or V6).
     /// @param first iterator to the first element that should be copied.
@@ -71,10 +84,10 @@ public:
     /// @brief Read a buffer as IP address.
     ///
     /// @param index buffer index.
-    /// @param [out] address read IP address.
     ///
+    /// @return IP address read from a buffer.
     /// @throw isc::OutOfRange if index is out of range.
-    void readAddress(const uint32_t index, asiolink::IOAddress& address) const;
+    asiolink::IOAddress readAddress(const uint32_t index) const;
 
     /// @brief Read a buffer as binary data.
     ///
@@ -140,10 +153,10 @@ public:
     /// @brief Read a buffer as string value.
     ///
     /// @param index buffer index.
-    /// @param [out] value read string value.
     ///
+    /// @return string value read from buffer.
     /// @throw isc::OutOfRange if index is out of range.
-    void readString(const uint32_t index, std::string& value) const;
+    std::string readString(const uint32_t index) const;
 
     /// @brief Parses received buffer.
     ///

src/lib/dhcp/option_data_types.cc

@@ -48,6 +48,10 @@ OptionDataTypeUtil::OptionDataTypeUtil() {
     data_type_names_[OPT_STRING_TYPE] = "string";
     data_type_names_[OPT_FQDN_TYPE] = "fqdn";
     data_type_names_[OPT_RECORD_TYPE] = "record";
+    // The "unknown" data type is declared here so as
+    // it can be returned by reference by a getDataTypeName
+    // function it no other type is suitable. Other than that
+    // this is unused.
     data_type_names_[OPT_UNKNOWN_TYPE] = "unknown";
 }
 
@@ -73,16 +77,21 @@ OptionDataTypeUtil::getDataTypeLen(const OptionDataType data_type) {
     case OPT_INT8_TYPE:
     case OPT_UINT8_TYPE:
         return (1);
+
     case OPT_INT16_TYPE:
     case OPT_UINT16_TYPE:
         return (2);
+
     case OPT_INT32_TYPE:
     case OPT_UINT32_TYPE:
         return (4);
+
     case OPT_IPV4_ADDRESS_TYPE:
         return (asiolink::V4ADDRESS_LEN);
+
     case OPT_IPV6_ADDRESS_TYPE:
         return (asiolink::V6ADDRESS_LEN);
+
     default:
         ;
     }
@@ -110,23 +119,22 @@ OptionDataTypeUtil::instance() {
     return (instance);
 }
 
-void
+asiolink::IOAddress
 OptionDataTypeUtil::readAddress(const std::vector<uint8_t>& buf,
-                            const short family,
-                            asiolink::IOAddress& address) {
+                                const short family) {
     using namespace isc::asiolink;
     if (family == AF_INET) {
         if (buf.size() < V4ADDRESS_LEN) {
             isc_throw(BadDataTypeCast, "unable to read data from the buffer as"
                       << " IPv4 address. Invalid buffer size: " << buf.size());
         }
-        address = IOAddress::fromBytes(AF_INET, &buf[0]);
-    } else if (buf.size() == V6ADDRESS_LEN) {
+        return (IOAddress::fromBytes(AF_INET, &buf[0]));
+    } else if (family == AF_INET6) {
         if (buf.size() < V6ADDRESS_LEN) {
             isc_throw(BadDataTypeCast, "unable to read data from the buffer as"
                       << " IPv6 address. Invalid buffer size: " << buf.size());
         }
-        address = IOAddress::fromBytes(AF_INET6, &buf[0]);
+        return (IOAddress::fromBytes(AF_INET6, &buf[0]));
     } else {
         isc_throw(BadDataTypeCast, "unable to read data from the buffer as"
                   "IP address. Invalid family: " << family);
@@ -136,6 +144,9 @@ OptionDataTypeUtil::readAddress(const std::vector<uint8_t>& buf,
 void
 OptionDataTypeUtil::writeAddress(const asiolink::IOAddress& address,
                                  std::vector<uint8_t>& buf) {
+    // @todo There is a ticket 2396 submitted, which adds the
+    // functionality to return a buffer representation of
+    // IOAddress. If so, this function can be simplified.
     if (address.getAddress().is_v4()) {
         asio::ip::address_v4::bytes_type addr_bytes =
             address.getAddress().to_v4().to_bytes();
@@ -192,28 +203,23 @@ OptionDataTypeUtil::readBool(const std::vector<uint8_t>& buf) {
 void
 OptionDataTypeUtil::writeBool(const bool value,
                               std::vector<uint8_t>& buf) {
-    if (value) {
-        buf.push_back(static_cast<uint8_t>(1));
-    } else {
-        buf.push_back(static_cast<uint8_t>(0));
-    }
+    buf.push_back(static_cast<uint8_t>(value ? 1 : 0));
 }
 
-void
-OptionDataTypeUtil::readString(const std::vector<uint8_t>& buf,
-                               std::string& value) {
-    value.insert(value.end(), buf.begin(), buf.end());
+std::string
+OptionDataTypeUtil::readString(const std::vector<uint8_t>& buf) {
+    std::string value;
+    if (buf.size() > 0) {
+        value.insert(value.end(), buf.begin(), buf.end());
+    }
+    return (value);
 }
 
 void
 OptionDataTypeUtil::writeString(const std::string& value,
                                 std::vector<uint8_t>& buf) {
     if (value.size() > 0) {
-        // 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.size(),
-                           buf.end());
+        buf.insert(buf.end(), value.begin(), value.end());
     }
 }
 

src/lib/dhcp/option_data_types.h

@@ -41,6 +41,13 @@ public:
 
 
 /// @brief Data types of DHCP option fields.
+///
+/// @warning The order of data types matters: OPT_UNKNOWN_TYPE
+/// must always be the last position. Also, OPT_RECORD_TYPE
+/// must be at last but one position. This is because some
+/// functions perform sanity checks on data type values using
+/// '>' operators, assuming that all values beyond the
+/// OPT_RECORD_TYPE are invalid.
 enum OptionDataType {
     OPT_EMPTY_TYPE,
     OPT_BINARY_TYPE,
@@ -175,6 +182,16 @@ struct OptionDataTypeTraits<std::string> {
 };
 
 /// @brief Utility class for option data types.
+///
+/// This class provides a set of utility functions to operate on
+/// supported DHCP option data types. It includes conversion
+/// between enumerator values representing data types and data
+/// type names. It also includes a set of functions that write
+/// data into option buffers and read data from option buffers.
+/// The data being written and read are converted from/to actual
+/// data types.
+/// @note This is a singleton class but it can be accessed via
+/// static methods only.
 class OptionDataTypeUtil {
 public:
 
@@ -212,19 +229,19 @@ public:
     ///
     /// @param buf input buffer.
     /// @param family address family: AF_INET or AF_INET6.
-    /// @param [out] address being read.
-    static void readAddress(const std::vector<uint8_t>& buf,
-                            const short family,
-                            asiolink::IOAddress& address);
+    /// 
+    /// @return address being read.
+    static asiolink::IOAddress readAddress(const std::vector<uint8_t>& buf,
+                                           const short family);
 
-    /// @brief Write IPv4 or IPv6 address into a buffer.
+    /// @brief Append IPv4 or IPv6 address to a buffer.
     ///
     /// @param address IPv4 or IPv6 address.
     /// @param [out] buf output buffer.
     static void writeAddress(const asiolink::IOAddress& address,
                              std::vector<uint8_t>& buf);
 
-    /// @brief Write hex-encoded binary values into a buffer.
+    /// @brief Append hex-encoded binary values to a buffer.
     ///
     /// @param hex_str string representing a binary value encoded
     /// with hexadecimal digits (without 0x prefix).
@@ -238,7 +255,7 @@ public:
     /// @return boolean value read from a buffer.
     static bool readBool(const std::vector<uint8_t>& buf);
 
-    /// @brief Write boolean value into a buffer.
+    /// @brief Append boolean value into a buffer.
     ///
     /// The bool value is encoded in a buffer in such a way that
     /// "1" means "true" and "0" means "false".
@@ -256,7 +273,7 @@ public:
     static T readInt(const std::vector<uint8_t>& buf) {
         if (!OptionDataTypeTraits<T>::integer_type) {
             isc_throw(isc::dhcp::InvalidDataType, "specified data type to be returned"
-                      " by readInteger is not supported integer type");
+                      " by readInteger is unsupported integer type");
         }
 
         assert(buf.size() == OptionDataTypeTraits<T>::len);
@@ -266,9 +283,13 @@ public:
             value = *(buf.begin());
             break;
         case 2:
+            // Calling readUint16 works either for unsigned
+            // or signed types.
             value = isc::util::readUint16(&(*buf.begin()));
             break;
         case 4:
+            // Calling readUint32 works either for unsigned
+            // or signed types.
             value = isc::util::readUint32(&(*buf.begin()));
             break;
         default:
@@ -280,7 +301,7 @@ public:
         return (value);
     }
 
-    /// @brief Write integer or unsigned integer value into a buffer.
+    /// @brief Append integer or unsigned integer value to a buffer.
     ///
     /// @param value an integer value to be written into a buffer.
     /// @param [out] buf output buffer.
@@ -314,9 +335,9 @@ public:
     /// @brief Read string value from a buffer.
     ///
     /// @param buf input buffer.
-    /// @param [out] value string value being read.
-    static void readString(const std::vector<uint8_t>& buf,
-                           std::string& value);
+    ///
+    /// @return string value being read.
+    static std::string readString(const std::vector<uint8_t>& buf);
 
     /// @brief Write UTF8-encoded string into a buffer.
     ///

src/lib/dhcp/option_definition.cc

@@ -350,8 +350,8 @@ OptionDefinition::writeToBuffer(const std::string& value,
     case OPT_IPV6_ADDRESS_TYPE:
         {
             asiolink::IOAddress address(value);
-            if (!address.getAddress().is_v4() &&
-                !address.getAddress().is_v6()) {
+            if (address.getFamily() != AF_INET &&
+                address.getFamily() != AF_INET6) {
                 isc_throw(BadDataTypeCast, "provided address " << address.toText()
                           << " is not a valid "
                           << (address.getAddress().is_v4() ? "IPv4" : "IPv6")

src/lib/dhcp/tests/option_custom_unittest.cc

@@ -278,7 +278,7 @@ TEST_F(OptionCustomTest, ipv4AddressData) {
 
     IOAddress address("127.0.0.1");
     // Read IPv4 address from using index 0.
-    ASSERT_NO_THROW(option->readAddress(0, address));
+    ASSERT_NO_THROW(address = option->readAddress(0));
 
     EXPECT_EQ("192.168.100.50", address.toText());
 }
@@ -306,7 +306,7 @@ TEST_F(OptionCustomTest, ipv6AddressData) {
     // IPv6 address.
     IOAddress address("::1");
     // Read an address from buffer #0.
-    ASSERT_NO_THROW(option->readAddress(0, address));
+    ASSERT_NO_THROW(address = option->readAddress(0));
 
     EXPECT_EQ("2001:db8:1::100", address.toText());
 }
@@ -333,7 +333,7 @@ TEST_F(OptionCustomTest, stringData) {
     // Custom option should now comprise single string value that
     // can be accessed using index 0.
     std::string value;
-    ASSERT_NO_THROW(option->readString(0, value));
+    ASSERT_NO_THROW(value = option->readString(0));
 
     EXPECT_EQ("hello world!", value);
 }
@@ -494,7 +494,7 @@ TEST_F(OptionCustomTest, ipv4AddressDataArray) {
     // We expect 3 IPv4 addresses being stored in the option.
     for (int i = 0; i < 3; ++i) {
         IOAddress address("10.10.10.10");
-        ASSERT_NO_THROW(option->readAddress(i, address));
+        ASSERT_NO_THROW(address = option->readAddress(i));
         EXPECT_EQ(addresses[i].toText(), address.toText());
     }
 }
@@ -529,7 +529,7 @@ TEST_F(OptionCustomTest, ipv6AddressDataArray) {
     // We expect 3 IPv6 addresses being stored in the option.
     for (int i = 0; i < 3; ++i) {
         IOAddress address("fe80::4");
-        ASSERT_NO_THROW(option->readAddress(i, address));
+        ASSERT_NO_THROW(address = option->readAddress(i));
         EXPECT_EQ(addresses[i].toText(), address.toText());
     }
 }
@@ -580,17 +580,17 @@ TEST_F(OptionCustomTest, recordData) {
 
     // Verify value in the field 2.
     IOAddress value2("127.0.0.1");
-    ASSERT_NO_THROW(option->readAddress(2, value2));
+    ASSERT_NO_THROW(value2 = option->readAddress(2));
     EXPECT_EQ("192.168.0.1", value2.toText());
 
     // Verify value in the field 3.
     IOAddress value3("::1");
-    ASSERT_NO_THROW(option->readAddress(3, value3));
+    ASSERT_NO_THROW(value3 = option->readAddress(3));
     EXPECT_EQ("2001:db8:1::1", value3.toText());
 
     // Verify value in the field 4.
     std::string value4;
-    ASSERT_NO_THROW(option->readString(4, value4));
+    ASSERT_NO_THROW(value4 = option->readString(4));
     EXPECT_EQ("ABCD", value4);
 }
 
@@ -740,7 +740,7 @@ TEST_F(OptionCustomTest, unpack) {
     // We expect 3 IPv4 addresses being stored in the option.
     for (int i = 0; i < 3; ++i) {
         IOAddress address("10.10.10.10");
-        ASSERT_NO_THROW(option->readAddress(i, address));
+        ASSERT_NO_THROW(address = option->readAddress(i));
         EXPECT_EQ(addresses[i].toText(), address.toText());
     }
 
@@ -767,7 +767,7 @@ TEST_F(OptionCustomTest, unpack) {
     // Verify that the addresses have been overwritten.
     for (int i = 0; i < 2; ++i) {
         IOAddress address("10.10.10.10");
-        ASSERT_NO_THROW(option->readAddress(i, address));
+        ASSERT_NO_THROW(address = option->readAddress(i));
         EXPECT_EQ(addresses[i].toText(), address.toText());
     }
 }
@@ -802,7 +802,7 @@ TEST_F(OptionCustomTest, setData) {
     // We expect 3 IPv6 addresses being stored in the option.
     for (int i = 0; i < 3; ++i) {
         IOAddress address("fe80::4");
-        ASSERT_NO_THROW(option->readAddress(i, address));
+        ASSERT_NO_THROW(address = option->readAddress(i));
         EXPECT_EQ(addresses[i].toText(), address.toText());
     }
 
@@ -828,7 +828,7 @@ TEST_F(OptionCustomTest, setData) {
     // Check that it has been replaced.
     for (int i = 0; i < 2; ++i) {
         IOAddress address("10.10.10.10");
-        ASSERT_NO_THROW(option->readAddress(i, address));
+        ASSERT_NO_THROW(address = option->readAddress(i));
         EXPECT_EQ(addresses[i].toText(), address.toText());
     }
 }