BIND 10 trac2544, updated. fdce9eb73133bb52609ed6ad51fdb77b28447eb3 [2544] Changes as a result of a code review.
BIND 10 source code commits
bind10-changes at lists.isc.org
Wed Dec 19 10:28:30 UTC 2012
The branch, trac2544 has been updated
via fdce9eb73133bb52609ed6ad51fdb77b28447eb3 (commit)
from 393c70b7a308f5d816a87cf21e017351dbf09260 (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 fdce9eb73133bb52609ed6ad51fdb77b28447eb3
Author: Marcin Siodelski <marcin at isc.org>
Date: Wed Dec 19 11:28:21 2012 +0100
[2544] Changes as a result of a code review.
-----------------------------------------------------------------------
Summary of changes:
src/bin/dhcp4/config_parser.cc | 39 +++++++++++--------------
src/bin/dhcp4/dhcp4_messages.mes | 4 +--
src/bin/dhcp4/tests/config_parser_unittest.cc | 11 +++----
src/bin/dhcp6/config_parser.cc | 39 +++++++++++--------------
src/bin/dhcp6/dhcp6_messages.mes | 4 +--
src/bin/dhcp6/tests/config_parser_unittest.cc | 9 +++---
6 files changed, 49 insertions(+), 57 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/bin/dhcp4/config_parser.cc b/src/bin/dhcp4/config_parser.cc
index ca0e639..76181c2 100644
--- a/src/bin/dhcp4/config_parser.cc
+++ b/src/bin/dhcp4/config_parser.cc
@@ -466,12 +466,12 @@ private:
///
/// This parser parses configuration entries that specify value of
/// a single option. These entries include option name, option code
-/// and data carried by the option. If parsing is successful than an
+/// and data carried by the option. If parsing is successful then an
/// instance of an option is created and added to the storage provided
/// by the calling class.
///
/// @todo This class parses and validates the option name. However it is
-/// not used anywhere util support for option spaces is implemented
+/// not used anywhere until support for option spaces is implemented
/// (see tickets #2319, #2314). When option spaces are implemented
/// there will be a way to reference the particular option using
/// its type (code) or option name.
@@ -827,26 +827,21 @@ public:
// a setStorage and build methods are invoked.
// Try uint32 type parser.
- if (buildParser<Uint32Parser, Uint32Storage >(parser, uint32_values_,
- param.second)) {
- // Storage set, build invoked on the parser, proceed with
- // next configuration element.
- continue;
- }
- // Try string type parser.
- if (buildParser<StringParser, StringStorage >(parser, string_values_,
- param.second)) {
- continue;
- }
- // Try pools parser.
- if (buildParser<PoolParser, PoolStorage >(parser, pools_,
- param.second)) {
- continue;
- }
- // Try option data parser.
- if (buildParser<OptionDataListParser, OptionStorage >(parser, options_,
- param.second)) {
- continue;
+ if (!buildParser<Uint32Parser, Uint32Storage >(parser, uint32_values_,
+ param.second) &&
+ // Try string type parser.
+ !buildParser<StringParser, StringStorage >(parser, string_values_,
+ param.second) &&
+ // Try pool parser.
+ !buildParser<PoolParser, PoolStorage >(parser, pools_,
+ param.second) &&
+ // Try option data parser.
+ !buildParser<OptionDataListParser, OptionStorage >(parser, options_,
+ param.second)) {
+ // Appropriate parsers are created in the createSubnet6ConfigParser
+ // and they should be limited to those that we check here for. Thus,
+ // if we fail to find a matching parser here it is a programming error.
+ isc_throw(Dhcp4ConfigError, "failed to find suitable parser");
}
}
// Ok, we now have subnet parsed
diff --git a/src/bin/dhcp4/dhcp4_messages.mes b/src/bin/dhcp4/dhcp4_messages.mes
index 3e46a73..f258430 100644
--- a/src/bin/dhcp4/dhcp4_messages.mes
+++ b/src/bin/dhcp4/dhcp4_messages.mes
@@ -45,9 +45,9 @@ This is an informational message reporting that the configuration has
been extended to include the specified IPv4 subnet.
% DHCP4_CONFIG_OPTION_DUPLICATE multiple options with the code: %1 added to the subnet: %2
-This warning message is issued on attempt to configure multiple options with the
+This warning message is issued on an attempt to configure multiple options with the
same option code for the particular subnet. Adding multiple options is uncommon
-for DHCPv4, yet it is not prohibited.
+for DHCPv4, but it is not prohibited.
% DHCP4_CONFIG_COMPLETE DHCPv4 server has completed configuration: %1
This is an informational message announcing the successful processing of a
diff --git a/src/bin/dhcp4/tests/config_parser_unittest.cc b/src/bin/dhcp4/tests/config_parser_unittest.cc
index 2f6df58..b88cdcc 100644
--- a/src/bin/dhcp4/tests/config_parser_unittest.cc
+++ b/src/bin/dhcp4/tests/config_parser_unittest.cc
@@ -1,4 +1,3 @@
-
// Copyright (C) 2012 Internet Systems Consortium, Inc. ("ISC")
//
// Permission to use, copy, modify, and/or distribute this software for any
@@ -205,9 +204,10 @@ public:
ASSERT_EQ(buf.getLength() - option_desc.option->getHeaderLen(),
expected_data_len);
}
- // Verify that the data is correct. However do not verify suboptions.
+ // Verify that the data is correct. Do not verify suboptions and a header.
const uint8_t* data = static_cast<const uint8_t*>(buf.getData());
- EXPECT_TRUE(memcmp(expected_data, data, expected_data_len));
+ EXPECT_EQ(0, memcmp(expected_data, data + option_desc.option->getHeaderLen(),
+ expected_data_len));
}
/// @brief Reset configuration database.
@@ -239,14 +239,15 @@ public:
<< ex.what() << std::endl;
}
-
- // returned value should be 0 (configuration success)
+ // status object must not be NULL
if (!status) {
FAIL() << "Fatal error: unable to reset configuration database"
<< " after the test. Configuration function returned"
<< " NULL pointer" << std::endl;
}
+
comment_ = parseAnswer(rcode_, status);
+ // returned value should be 0 (configuration success)
if (rcode_ != 0) {
FAIL() << "Fatal error: unable to reset configuration database"
<< " after the test. Configuration function returned"
diff --git a/src/bin/dhcp6/config_parser.cc b/src/bin/dhcp6/config_parser.cc
index 4c6fab1..edda312 100644
--- a/src/bin/dhcp6/config_parser.cc
+++ b/src/bin/dhcp6/config_parser.cc
@@ -496,12 +496,12 @@ private:
///
/// This parser parses configuration entries that specify value of
/// a single option. These entries include option name, option code
-/// and data carried by the option. If parsing is successful than an
+/// and data carried by the option. If parsing is successful then an
/// instance of an option is created and added to the storage provided
/// by the calling class.
///
/// @todo This class parses and validates the option name. However it is
-/// not used anywhere util support for option spaces is implemented
+/// not used anywhere until support for option spaces is implemented
/// (see tickets #2319, #2314). When option spaces are implemented
/// there will be a way to reference the particular option using
/// its type (code) or option name.
@@ -857,26 +857,21 @@ public:
// a setStorage and build methods are invoked.
// Try uint32 type parser.
- if (buildParser<Uint32Parser, Uint32Storage >(parser, uint32_values_,
- param.second)) {
- // Storage set, build invoked on the parser, proceed with
- // next configuration element.
- continue;
- }
- // Try string type parser.
- if (buildParser<StringParser, StringStorage >(parser, string_values_,
- param.second)) {
- continue;
- }
- // Try pools parser.
- if (buildParser<PoolParser, PoolStorage >(parser, pools_,
- param.second)) {
- continue;
- }
- // Try option data parser.
- if (buildParser<OptionDataListParser, OptionStorage >(parser, options_,
- param.second)) {
- continue;
+ if (!buildParser<Uint32Parser, Uint32Storage >(parser, uint32_values_,
+ param.second) ||
+ // Try string type parser.
+ !buildParser<StringParser, StringStorage >(parser, string_values_,
+ param.second) ||
+ // Try pool parser.
+ !buildParser<PoolParser, PoolStorage >(parser, pools_,
+ param.second) ||
+ // Try option data parser.
+ !buildParser<OptionDataListParser, OptionStorage >(parser, options_,
+ param.second)) {
+ // Appropriate parsers are created in the createSubnet6ConfigParser
+ // and they should be limited to those that we check here for. Thus,
+ // if we fail to find a matching parser here it is a programming error.
+ isc_throw(Dhcp6ConfigError, "failed to find suitable parser");
}
}
// Ok, we now have subnet parsed
diff --git a/src/bin/dhcp6/dhcp6_messages.mes b/src/bin/dhcp6/dhcp6_messages.mes
index 6ab42b3..19e9c7e 100644
--- a/src/bin/dhcp6/dhcp6_messages.mes
+++ b/src/bin/dhcp6/dhcp6_messages.mes
@@ -47,9 +47,9 @@ This is an informational message reporting that the configuration has
been extended to include the specified subnet.
% DHCP6_CONFIG_OPTION_DUPLICATE multiple options with the code: %1 added to the subnet: %2
-This warning message is issued on attempt to configure multiple options with the
+This warning message is issued on an attempt to configure multiple options with the
same option code for the particular subnet. Adding multiple options is uncommon
-for DHCPv6, yet it is not prohibited.
+for DHCPv6, but it is not prohibited.
% DHCP6_CONFIG_START DHCPv6 server is processing the following configuration: %1
This is a debug message that is issued every time the server receives a
diff --git a/src/bin/dhcp6/tests/config_parser_unittest.cc b/src/bin/dhcp6/tests/config_parser_unittest.cc
index 4fd6baa..1440338 100644
--- a/src/bin/dhcp6/tests/config_parser_unittest.cc
+++ b/src/bin/dhcp6/tests/config_parser_unittest.cc
@@ -144,14 +144,14 @@ public:
<< ex.what() << std::endl;
}
-
- // returned value should be 0 (configuration success)
+ // status object must not be NULL
if (!status) {
FAIL() << "Fatal error: unable to reset configuration database"
<< " after the test. Configuration function returned"
<< " NULL pointer" << std::endl;
}
comment_ = parseAnswer(rcode_, status);
+ // returned value should be 0 (configuration success)
if (rcode_ != 0) {
FAIL() << "Fatal error: unable to reset configuration database"
<< " after the test. Configuration function returned"
@@ -215,9 +215,10 @@ public:
ASSERT_EQ(buf.getLength() - option_desc.option->getHeaderLen(),
expected_data_len);
}
- // Verify that the data is correct. However do not verify suboptions.
+ // Verify that the data is correct. Do not verify suboptions and a header.
const uint8_t* data = static_cast<const uint8_t*>(buf.getData());
- EXPECT_TRUE(memcmp(expected_data, data, expected_data_len));
+ EXPECT_EQ(0, memcmp(expected_data, data + option_desc.option->getHeaderLen(),
+ expected_data_len));
}
Dhcpv6Srv srv_;
More information about the bind10-changes
mailing list