From: Marcin Siodelski Date: Wed, 28 Nov 2012 16:04:26 +0000 (+0100) Subject: [2312] Changes to the code as a result of review. X-Git-Tag: bind10-1.0.0-beta-release~46^2~18^2~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d85d48eee9608db9f4c912e5cfe3ae1f2f726c05;p=thirdparty%2Fkea.git [2312] Changes to the code as a result of review. --- diff --git a/src/lib/dhcp/option_custom.cc b/src/lib/dhcp/option_custom.cc index bffadd2371..6bc871411a 100644 --- a/src/lib/dhcp/option_custom.cc +++ b/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(index); + text << readInteger(index); break; case OPT_INT16_TYPE: - tmp << readInteger(index); + text << readInteger(index); break; case OPT_INT32_TYPE: - tmp << readInteger(index); + text << readInteger(index); break; case OPT_UINT8_TYPE: - tmp << readInteger(index); + text << readInteger(index); break; case OPT_UINT16_TYPE: - tmp << readInteger(index); + text << readInteger(index); break; case OPT_UINT32_TYPE: - tmp << readInteger(index); + text << readInteger(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::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) diff --git a/src/lib/dhcp/option_custom.h b/src/lib/dhcp/option_custom.h index 8cefcd0cc0..ad248c5f38 100644 --- a/src/lib/dhcp/option_custom.h +++ b/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. /// diff --git a/src/lib/dhcp/option_data_types.cc b/src/lib/dhcp/option_data_types.cc index c469d2bf88..46aa663f38 100644 --- a/src/lib/dhcp/option_data_types.cc +++ b/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& 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& buf, void OptionDataTypeUtil::writeAddress(const asiolink::IOAddress& address, std::vector& 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& buf) { void OptionDataTypeUtil::writeBool(const bool value, std::vector& buf) { - if (value) { - buf.push_back(static_cast(1)); - } else { - buf.push_back(static_cast(0)); - } + buf.push_back(static_cast(value ? 1 : 0)); } -void -OptionDataTypeUtil::readString(const std::vector& buf, - std::string& value) { - value.insert(value.end(), buf.begin(), buf.end()); +std::string +OptionDataTypeUtil::readString(const std::vector& 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& 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()); } } diff --git a/src/lib/dhcp/option_data_types.h b/src/lib/dhcp/option_data_types.h index cd8176d85f..afc6d9352f 100644 --- a/src/lib/dhcp/option_data_types.h +++ b/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 { }; /// @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& buf, - const short family, - asiolink::IOAddress& address); + /// + /// @return address being read. + static asiolink::IOAddress readAddress(const std::vector& 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& 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& 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& buf) { if (!OptionDataTypeTraits::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::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& buf, - std::string& value); + /// + /// @return string value being read. + static std::string readString(const std::vector& buf); /// @brief Write UTF8-encoded string into a buffer. /// diff --git a/src/lib/dhcp/option_definition.cc b/src/lib/dhcp/option_definition.cc index 20dabfa514..58d0c4bae8 100644 --- a/src/lib/dhcp/option_definition.cc +++ b/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") diff --git a/src/lib/dhcp/tests/option_custom_unittest.cc b/src/lib/dhcp/tests/option_custom_unittest.cc index abfa9ecad5..da3644b6f4 100644 --- a/src/lib/dhcp/tests/option_custom_unittest.cc +++ b/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()); } }