BIND 10 trac2637, updated. 4e02045923698cb38b29b22b25e700a41c10aaee [2637] Changes as a result of the review.
BIND 10 source code commits
bind10-changes at lists.isc.org
Mon Jan 21 21:00:21 UTC 2013
The branch, trac2637 has been updated
via 4e02045923698cb38b29b22b25e700a41c10aaee (commit)
from d9421d93e3a65e6025281ece92706f7c2003ae77 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit 4e02045923698cb38b29b22b25e700a41c10aaee
Author: Marcin Siodelski <marcin at isc.org>
Date: Mon Jan 21 22:00:12 2013 +0100
[2637] Changes as a result of the review.
-----------------------------------------------------------------------
Summary of changes:
src/bin/dhcp4/config_parser.cc | 10 +--
src/bin/dhcp4/ctrl_dhcp4_srv.cc | 12 ++--
src/bin/dhcp6/config_parser.cc | 12 ++--
src/lib/dhcp/option.cc | 5 +-
src/lib/dhcp/option.h | 6 --
src/lib/dhcp/option_definition.cc | 40 +++++++-----
src/lib/dhcp/option_definition.h | 11 ++++
src/lib/dhcp/tests/option_definition_unittest.cc | 73 ++++++++++++++++++++++
8 files changed, 128 insertions(+), 41 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/bin/dhcp4/config_parser.cc b/src/bin/dhcp4/config_parser.cc
index 1d19083..861a570 100644
--- a/src/bin/dhcp4/config_parser.cc
+++ b/src/bin/dhcp4/config_parser.cc
@@ -758,14 +758,14 @@ private:
// Check that the option name has been specified, is non-empty and does not
// contain spaces.
std::string option_name = getParam<std::string>("name", string_values_);
- if (option_name.empty()) {
+ if (!OptionDefinition::validateName(option_name)) {
isc_throw(DhcpConfigError, "name of the option with code '"
- << option_code << "' is empty");
- } else if (option_name.find(" ") != std::string::npos) {
- isc_throw(DhcpConfigError, "invalid option name '" << option_name
- << "', space character is not allowed");
+ << option_code << "' is invalid: '"
+ << option_name << "'");
}
+ // Check that the option name has been specified and that it is
+ // valid.
std::string option_space = getParam<std::string>("space", string_values_);
if (!OptionSpace::validateName(option_space)) {
isc_throw(DhcpConfigError, "invalid option space name '"
diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.cc b/src/bin/dhcp4/ctrl_dhcp4_srv.cc
index 8ac49d3..bd5bc4e 100644
--- a/src/bin/dhcp4/ctrl_dhcp4_srv.cc
+++ b/src/bin/dhcp4/ctrl_dhcp4_srv.cc
@@ -97,7 +97,7 @@ ControlledDhcpv4Srv::dhcp4ConfigHandler(ConstElementPtr new_config) {
// Merge an existing and new configuration.
isc::data::merge(merged_config, new_config);
LOG_DEBUG(dhcp4_logger, DBG_DHCP4_COMMAND, DHCP4_CONFIG_UPDATE)
- .arg(full_config->str());
+ .arg(merged_config->str());
}
// Configure the server.
@@ -153,7 +153,7 @@ void ControlledDhcpv4Srv::establishSession() {
.arg(specfile);
cc_session_ = new Session(io_service_.get_io_service());
// Create a session with the dummy configuration handler.
- // Dumy configuration handler is internally invoked by the
+ // Dummy configuration handler is internally invoked by the
// constructor and on success the constructor updates
// the current session with the configuration that had been
// commited in the previous session. If we did not install
@@ -164,10 +164,10 @@ void ControlledDhcpv4Srv::establishSession() {
dhcp4CommandHandler, false);
config_session_->start();
- // We initially create ModuleCCSession() without configHandler, as
- // the session module is too eager to send partial configuration.
- // We want to get the full configuration, so we explicitly call
- // getFullConfig() and then pass it to our configHandler.
+ // Replace the pointer to the dummy configuration handler that we
+ // passed in the ModuleCCSession constructor with the pointer
+ // to the actual config handler that will be parsing future
+ // changes to the configuration.
config_session_->setConfigHandler(dhcp4ConfigHandler);
try {
diff --git a/src/bin/dhcp6/config_parser.cc b/src/bin/dhcp6/config_parser.cc
index 8634325..af81deb 100644
--- a/src/bin/dhcp6/config_parser.cc
+++ b/src/bin/dhcp6/config_parser.cc
@@ -785,15 +785,13 @@ private:
<< "', it must not exceed '"
<< std::numeric_limits<uint16_t>::max() << "'");
}
- // Check that the option name has been specified, is non-empty and does not
- // contain spaces.
+ // Check that the option name has been specified and that it is
+ // valid.
std::string option_name = getParam<std::string>("name", string_values_);
- if (option_name.empty()) {
+ if (!OptionDefinition::validateName(option_name)) {
isc_throw(DhcpConfigError, "name of the option with code '"
- << option_code << "' is empty");
- } else if (option_name.find(" ") != std::string::npos) {
- isc_throw(DhcpConfigError, "invalid option name '" << option_name
- << "', space character is not allowed");
+ << option_code << "' is invalid: '"
+ << option_name << "'");
}
std::string option_space = getParam<std::string>("space", string_values_);
diff --git a/src/lib/dhcp/option.cc b/src/lib/dhcp/option.cc
index e06b163..7e25e8b 100644
--- a/src/lib/dhcp/option.cc
+++ b/src/lib/dhcp/option.cc
@@ -84,9 +84,10 @@ Option::check() {
}
void Option::pack(isc::util::OutputBuffer& buf) {
- // Write a header.
+ // Write a header (protocol dependent:
+ // 2 bytes for DHCPv4 and 4 for DHCPv6)
packHeader(buf);
- // Write data.
+ // Write data (the same for both protocols).
if (!data_.empty()) {
buf.writeData(&data_[0], data_.size());
}
diff --git a/src/lib/dhcp/option.h b/src/lib/dhcp/option.h
index 553e825..77125de 100644
--- a/src/lib/dhcp/option.h
+++ b/src/lib/dhcp/option.h
@@ -156,13 +156,7 @@ public:
/// @brief Writes option in wire-format to a buffer.
///
- /// Writes option in wire-format to buffer, returns pointer to first unused
- /// byte after stored option (that is useful for writing options one after
- /// another).
- ///
/// @param buf pointer to a buffer
- ///
- /// @throw BadValue Universe of the option is neither V4 nor V6.
virtual void pack(isc::util::OutputBuffer& buf);
/// @brief Parses received buffer.
diff --git a/src/lib/dhcp/option_definition.cc b/src/lib/dhcp/option_definition.cc
index 253cdcd..8360e5f 100644
--- a/src/lib/dhcp/option_definition.cc
+++ b/src/lib/dhcp/option_definition.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2012 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC")
//
// Permission to use, copy, modify, and/or distribute this software for any
// purpose with or without fee is hereby granted, provided that the above
@@ -210,22 +210,10 @@ OptionDefinition::sanityCheckUniverse(const Option::Universe expected_universe,
void
OptionDefinition::validate() const {
- using namespace boost::algorithm;
-
std::ostringstream err_str;
- // Allowed characters in the option name are: lower or
- // upper case letters, digits, underscores and hyphens.
- // Empty option spaces are not allowed.
- if (!all(name_, boost::is_from_range('a', 'z') ||
- boost::is_from_range('A', 'Z') ||
- boost::is_digit() ||
- boost::is_any_of(std::string("-_"))) ||
- name_.empty() ||
- // Hyphens and underscores are not allowed at the beginning
- // and at the end of the option name.
- all(find_head(name_, 1), boost::is_any_of(std::string("-_"))) ||
- all(find_tail(name_, 1), boost::is_any_of(std::string("-_")))) {
+ // Option name must be valid.
+ if (!OptionDefinition::validateName(name_)) {
err_str << "invalid option name '" << name_ << "'";
} else if (type_ >= OPT_UNKNOWN_TYPE) {
@@ -292,6 +280,28 @@ OptionDefinition::validate() const {
}
bool
+OptionDefinition::validateName(const std::string& option_name) {
+
+ using namespace boost::algorithm;
+
+ // Allowed characters in the option name are: lower or
+ // upper case letters, digits, underscores and hyphens.
+ // Empty option names are not allowed.
+ if (!all(option_name, boost::is_from_range('a', 'z') ||
+ boost::is_from_range('A', 'Z') ||
+ boost::is_digit() ||
+ boost::is_any_of(std::string("-_"))) ||
+ option_name.empty() ||
+ // Hyphens and underscores are not allowed at the beginning
+ // and at the end of the option name.
+ all(find_head(option_name, 1), boost::is_any_of(std::string("-_"))) ||
+ all(find_tail(option_name, 1), boost::is_any_of(std::string("-_")))) {
+ return (false);
+ }
+ return (true);
+}
+
+bool
OptionDefinition::haveIAx6Format(OptionDataType first_type) const {
return (haveType(OPT_RECORD_TYPE) &&
record_fields_.size() == 3 &&
diff --git a/src/lib/dhcp/option_definition.h b/src/lib/dhcp/option_definition.h
index efcaba0..79d2ac7 100644
--- a/src/lib/dhcp/option_definition.h
+++ b/src/lib/dhcp/option_definition.h
@@ -212,6 +212,17 @@ public:
/// @throw MalformedOptionDefinition option definition is invalid.
void validate() const;
+ /// @brief Check if the option name is valid.
+ ///
+ /// This function checks the the specified option name
+ /// conforms to the following rules: characters in the
+ /// option name are: lower or upper case letters,
+ /// digits, underscores and hyphens. Empty strings are
+ /// not allowed.
+ ///
+ /// @return true if option name is valid.
+ static bool validateName(const std::string& option_name);
+
/// @brief Check if specified format is IA_NA option format.
///
/// @return true if specified format is IA_NA option format.
diff --git a/src/lib/dhcp/tests/option_definition_unittest.cc b/src/lib/dhcp/tests/option_definition_unittest.cc
index 310c7bf..dc0dc67 100644
--- a/src/lib/dhcp/tests/option_definition_unittest.cc
+++ b/src/lib/dhcp/tests/option_definition_unittest.cc
@@ -743,6 +743,42 @@ TEST_F(OptionDefinitionTest, uint32Tokenized) {
// @todo Add more cases for DHCPv4
}
+// The purpose of this test is to verify that the definition for option
+// that comprises a single FQDN value can be created and that this
+// definition can be used to create an option with FQDN.
+TEST_F(OptionDefinitionTest, fqdnTokenized) {
+
+ const uint16_t opt_code = 100;
+
+ // Create option definition.
+ OptionDefinition opt_def("OPTION_FQDN", opt_code, "fqdn");
+
+ // The multiple values can be added but only the first one
+ // is taken. The other one is ignored.
+ std::vector<std::string> values;
+ values.push_back("example.com");
+ values.push_back("foo.example.com.");
+ // Create the option.
+ OptionPtr option_v6;
+ EXPECT_NO_THROW(
+ option_v6 = opt_def.optionFactory(Option::V6, opt_code, values);
+ );
+ // Check that it has been successfully created.
+ ASSERT_TRUE(option_v6);
+ ASSERT_TRUE(typeid(*option_v6) == typeid(OptionCustom));
+ boost::shared_ptr<OptionCustom> option_cast_v6 =
+ boost::static_pointer_cast<OptionCustom>(option_v6);
+ ASSERT_TRUE(option_cast_v6);
+ // It should comprise single FQDN.
+ std::string fqdn;
+ // Get the FQDN using index 0 (the first available).
+ ASSERT_NO_THROW(fqdn = option_cast_v6->readFqdn(0));
+ EXPECT_EQ("example.com.", fqdn);
+ // Attempt to get another FQDN (using index 1) should
+ // result in error.
+ EXPECT_THROW(option_cast_v6->readFqdn(1), isc::OutOfRange);
+}
+
// The purpose of this test is to verify that definition for option that
// comprises array of uint16 values can be created and that this definition
// can be used to create option with an array of uint16 values.
@@ -892,6 +928,43 @@ TEST_F(OptionDefinitionTest, uint32ArrayTokenized) {
EXPECT_EQ(1111, values[3]);
}
+// The purpose of this test is to verify that the definition of option
+// comprising an array of FQDNs can be created and that this definition
+// can be used to create the instance of this option.
+TEST_F(OptionDefinitionTest, fqdnArrayTokenized) {
+
+ const uint16_t opt_code = 100;
+
+ // Create a definition of an option that comprises an array
+ // of FQDNs.
+ OptionDefinition opt_def("OPTION_FQDN_ARRAY", opt_code, "fqdn",
+ true);
+
+ // Add two FQDNs.
+ std::vector<std::string> values;
+ values.push_back("example.com");
+ values.push_back("foo.example.com.");
+ // Create the instance of the option with two FQDNs.
+ OptionPtr option_v6;
+ EXPECT_NO_THROW(
+ option_v6 = opt_def.optionFactory(Option::V6, opt_code, values);
+ );
+ // Check if option has been successfully created.
+ ASSERT_TRUE(option_v6);
+ // It has the expected type: OptionCustom.
+ ASSERT_TRUE(typeid(*option_v6) == typeid(OptionCustom));
+ boost::shared_ptr<OptionCustom> option_cast_v6 =
+ boost::static_pointer_cast<OptionCustom>(option_v6);
+ ASSERT_TRUE(option_cast_v6);
+ // Get the FQDNs and validate them.
+ std::string fqdn;
+ ASSERT_NO_THROW(fqdn = option_cast_v6->readFqdn(0));
+ EXPECT_EQ("example.com.", fqdn);
+ std::string fqdn2;
+ ASSERT_NO_THROW(fqdn2 = option_cast_v6->readFqdn(1));
+ EXPECT_EQ("foo.example.com.", fqdn2);
+}
+
// The purpose of this test is to verify that the definition can be created
// for the option that comprises string value in the UTF8 format.
TEST_F(OptionDefinitionTest, utf8StringTokenized) {
More information about the bind10-changes
mailing list