From: Marcin Siodelski Date: Thu, 6 Dec 2012 15:39:04 +0000 (+0100) Subject: [2491] Changes as a result of the review. X-Git-Tag: bind10-1.0.0-beta-release~19^2~15^2~5 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=9430b1fef5b2cc99e97b4cfaf6ccd518bcd76198;p=thirdparty%2Fkea.git [2491] Changes as a result of the review. --- diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 122345c6e8..33bab99c9b 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/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(code, 0); + option_status->writeInteger(code, 0); // Set a message (1 - means data field #1). option_status->writeString(text, 1); return (option_status); diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc index d326557c11..934638f08b 100644 --- a/src/lib/dhcp/libdhcp++.cc +++ b/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: diff --git a/src/lib/dhcp/option_custom.cc b/src/lib/dhcp/option_custom.cc index dccac8435f..0c3cb818af 100644 --- a/src/lib/dhcp/option_custom.cc +++ b/src/lib/dhcp/option_custom.cc @@ -127,7 +127,16 @@ OptionCustom::createBuffers() { std::vector 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 diff --git a/src/lib/dhcp/option_custom.h b/src/lib/dhcp/option_custom.h index a3d16efa8c..d9a4c18417 100644 --- a/src/lib/dhcp/option_custom.h +++ b/src/lib/dhcp/option_custom.h @@ -200,7 +200,7 @@ public: /// @return read integer value. template 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. diff --git a/src/lib/dhcp/option_data_types.cc b/src/lib/dhcp/option_data_types.cc index 459dea733b..0027cfe8b4 100644 --- a/src/lib/dhcp/option_data_types.cc +++ b/src/lib/dhcp/option_data_types.cc @@ -209,26 +209,20 @@ OptionDataTypeUtil::writeBool(const bool value, } std::string -OptionDataTypeUtil::readFqdn(const std::vector& buf, - size_t& len) { - len = 0; - +OptionDataTypeUtil::readFqdn(const std::vector& 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(&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(&buf[buf.size() - labels.getDataLength()]), + memcpy(static_cast(&buf[buf.size() - read_len]), data, read_len); } } catch (const isc::Exception& ex) { diff --git a/src/lib/dhcp/option_data_types.h b/src/lib/dhcp/option_data_types.h index 1628735986..ff5789d16e 100644 --- a/src/lib/dhcp/option_data_types.h +++ b/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& buf, - size_t& len); + static std::string readFqdn(const std::vector& buf); /// @brief Append FQDN into a buffer. /// diff --git a/src/lib/dhcp/option_definition.cc b/src/lib/dhcp/option_definition.cc index 434c6a2c20..7ed48e4c8d 100644 --- a/src/lib/dhcp/option_definition.cc +++ b/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(u, type, begin, end));; + factoryInteger(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(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)))); diff --git a/src/lib/dhcp/tests/libdhcp++_unittest.cc b/src/lib/dhcp/tests/libdhcp++_unittest.cc index d65d4a8b0b..8ead52126e 100644 --- a/src/lib/dhcp/tests/libdhcp++_unittest.cc +++ b/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. diff --git a/src/lib/dhcp/tests/option_custom_unittest.cc b/src/lib/dhcp/tests/option_custom_unittest.cc index ec6b53d784..044279bf37 100644 --- a/src/lib/dhcp/tests/option_custom_unittest.cc +++ b/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(8712, buf); // Initialize field 1 to 'true' buf.push_back(static_cast(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 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) { diff --git a/src/lib/dhcp/tests/option_data_types_unittest.cc b/src/lib/dhcp/tests/option_data_types_unittest.cc index a185cdf3a1..748a84b566 100644 --- a/src/lib/dhcp/tests/option_data_types_unittest.cc +++ b/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 buf; - // Write uint8_t ASSERT_NO_THROW(OptionDataTypeUtil::writeInt(127, buf)); - // Write uint16_t ASSERT_NO_THROW(OptionDataTypeUtil::writeInt(1023, buf)); - // Write uint32_t ASSERT_NO_THROW(OptionDataTypeUtil::writeInt(4096, buf)); - // Write int32_t ASSERT_NO_THROW(OptionDataTypeUtil::writeInt(-1024, buf)); - // Write int16_t ASSERT_NO_THROW(OptionDataTypeUtil::writeInt(512, buf)); - // Write int8_t ASSERT_NO_THROW(OptionDataTypeUtil::writeInt(-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 ); }