BIND 10 trac2317, updated. 016a19585f32b9bdf57fc5aecdc3b073f3d1a482 [2317] Added changes as a result of the review.
BIND 10 source code commits
bind10-changes at lists.isc.org
Mon Jan 14 11:25:12 UTC 2013
The branch, trac2317 has been updated
via 016a19585f32b9bdf57fc5aecdc3b073f3d1a482 (commit)
from c3edda086f948bc9c6bd46c8b9198a2e5261eac2 (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 016a19585f32b9bdf57fc5aecdc3b073f3d1a482
Author: Marcin Siodelski <marcin at isc.org>
Date: Mon Jan 14 12:25:01 2013 +0100
[2317] Added changes as a result of the review.
-----------------------------------------------------------------------
Summary of changes:
src/bin/dhcp4/config_parser.cc | 63 ++++++++++++++++++++---------
src/bin/dhcp6/config_parser.cc | 65 +++++++++++++++++++++---------
src/lib/dhcpsrv/dhcp_config_parser.h | 4 +-
src/lib/dhcpsrv/option_space_container.h | 4 ++
4 files changed, 97 insertions(+), 39 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/bin/dhcp4/config_parser.cc b/src/bin/dhcp4/config_parser.cc
index 82cdc95..8ebbaff 100644
--- a/src/bin/dhcp4/config_parser.cc
+++ b/src/bin/dhcp4/config_parser.cc
@@ -92,7 +92,7 @@ StringStorage string_defaults;
OptionStorage option_defaults;
/// @brief Global storage for option definitions.
-OptionDefStorage option_def_defaults;
+OptionDefStorage option_def_intermediate;
/// @brief a dummy configuration parser
///
@@ -590,9 +590,20 @@ 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 then an
-/// instance of an option is created and added to the storage provided
-/// by the calling class.
+/// and data carried by the option. The option data can be specified
+/// in one of the two available formats: binary value represented as
+/// a string of hexadecimal digits or a list of comma separated values.
+/// The format being used is controlled by csv-format configuration
+/// parameter. When setting this value to True, the latter format is
+/// used. The subsequent values in the CSV format apply to relevant
+/// option data fields in the configured option. For example the
+/// configuration: "data" : "192.168.2.0, 56, hello world" can be
+/// used to set values for the option comprising IPv4 address,
+/// integer and string data field. Note that order matters. If the
+/// order of values does not match the order of data fields within
+/// an option the configuration will not be accepted. If parsing
+/// is successful then an instance of an option is created and
+/// added to the storage provided by the calling class.
class OptionDataParser : public DhcpConfigParser {
public:
@@ -687,6 +698,9 @@ public:
}
uint16_t opt_type = option_descriptor_.option->getType();
Subnet::OptionContainerPtr options = options_->getItems(option_space_);
+ // The getItems() should never return NULL pointer. If there are no
+ // options configured for the particular option space a pointer
+ // to an empty container should be returned.
assert(options);
Subnet::OptionContainerTypeIndex& idx = options->get<1>();
// Try to find options with the particular option code in the main
@@ -769,7 +783,10 @@ private:
// need to search for its definition among user-configured
// options. They are expected to be in the global storage
// already.
- OptionDefContainerPtr defs = option_def_defaults.getItems(option_space);
+ OptionDefContainerPtr defs = option_def_intermediate.getItems(option_space);
+ // The getItems() should never return the NULL pointer. If there are
+ // no option definitions for the particular option space a pointer
+ // to an empty container should be returned.
assert(defs);
const OptionDefContainerTypeIndex& idx = defs->get<1>();
OptionDefContainerTypeRange range = idx.equal_range(option_code);
@@ -1044,12 +1061,12 @@ public:
}
}
- /// @brief Sets a pointer to a storage.
+ /// @brief Sets a pointer to the data store.
///
/// The newly created instance of an option definition will be
- /// added to a storage given by the argument.
+ /// added to the data store given by the argument.
///
- /// @param storage pointer to a storage where the option definition
+ /// @param storage pointer to the data store where the option definition
/// will be added to.
void setStorage(OptionDefStorage* storage) {
storage_ = storage;
@@ -1083,7 +1100,7 @@ private:
// Split the list of record types into tokens.
std::vector<std::string> record_tokens =
isc::util::str::tokens(record_types, ",");
- // Iterate over each token and add a record typy into
+ // Iterate over each token and add a record type into
// option definition.
BOOST_FOREACH(std::string record_type, record_tokens) {
try {
@@ -1103,7 +1120,7 @@ private:
def->validate();
} catch (const isc::Exception ex) {
isc_throw(DhcpConfigError, "invalid option definition"
- << " parameters" << ex.what());
+ << " parameters: " << ex.what());
}
// Option definition has been created successfully.
option_space_name_ = space;
@@ -1155,7 +1172,7 @@ public:
void build(ConstElementPtr option_def_list) {
// Clear existing items in the global storage.
// We are going to replace all of them.
- option_def_defaults.clearItems();
+ option_def_intermediate.clearItems();
if (!option_def_list) {
isc_throw(DhcpConfigError, "parser error: a pointer to a list of"
@@ -1165,7 +1182,7 @@ public:
BOOST_FOREACH(ConstElementPtr option_def, option_def_list->listValue()) {
boost::shared_ptr<OptionDefParser>
parser(new OptionDefParser("single-option-def"));
- parser->setStorage(&option_def_defaults);
+ parser->setStorage(&option_def_intermediate);
parser->build(option_def);
parser->commit();
}
@@ -1181,10 +1198,13 @@ public:
// We need to move option definitions from the temporary
// storage to the global storage.
BOOST_FOREACH(std::string space_name,
- option_def_defaults.getOptionSpaceNames()) {
+ option_def_intermediate.getOptionSpaceNames()) {
BOOST_FOREACH(OptionDefinitionPtr def,
- *option_def_defaults.getItems(space_name)) {
+ *option_def_intermediate.getItems(space_name)) {
+ // All option definitions should be initialized to non-NULL
+ // values. The validation is expected to be made by the
+ // OptionDefParser when creating an option definition.
assert(def);
cfg_mgr.addOptionDef(def, space_name);
}
@@ -1371,7 +1391,9 @@ private:
// Get all options within a particular option space.
BOOST_FOREACH(Subnet::OptionDescriptor desc,
*options_.getItems(option_space)) {
- // In theory, option should be non-NULL.
+ // The pointer should be non-NULL. The validation is expected
+ // to be performed by the OptionDataParser before adding an
+ // option descriptor to the container.
assert(desc.option);
// We want to check whether an option with the particular
// option code has been already added. If so, we want
@@ -1397,6 +1419,9 @@ private:
// Get all global options for the particular option space.
BOOST_FOREACH(Subnet::OptionDescriptor desc,
*option_defaults.getItems(option_space)) {
+ // The pointer should be non-NULL. The validation is expected
+ // to be performed by the OptionDataParser before adding an
+ // option descriptor to the container.
assert(desc.option);
// Check if the particular option has been already added.
// This would mean that it has been configured in the
@@ -1610,7 +1635,9 @@ configureDhcp4Server(Dhcpv4Srv& , ConstElementPtr config_set) {
// other values. Typically, the values in the subnet4 structure
// depend on the global values. Also, option values configuration
// must be performed after the option definitions configurations.
- // Thus we group parsers and will fire them in the right order.
+ // Thus we group parsers and will fire them in the right order:
+ // all parsers other than subnet4 and option-data parser,
+ // option-data parser, subnet4 parser.
ParserCollection independent_parsers;
ParserPtr subnet_parser;
ParserPtr option_parser;
@@ -1625,7 +1652,7 @@ configureDhcp4Server(Dhcpv4Srv& , ConstElementPtr config_set) {
Uint32Storage uint32_local(uint32_defaults);
StringStorage string_local(string_defaults);
OptionStorage option_local(option_defaults);
- OptionDefStorage option_def_local(option_def_defaults);
+ OptionDefStorage option_def_local(option_def_intermediate);
// answer will hold the result.
ConstElementPtr answer;
@@ -1717,7 +1744,7 @@ configureDhcp4Server(Dhcpv4Srv& , ConstElementPtr config_set) {
std::swap(uint32_defaults, uint32_local);
std::swap(string_defaults, string_local);
std::swap(option_defaults, option_local);
- std::swap(option_def_defaults, option_def_local);
+ std::swap(option_def_intermediate, option_def_local);
return (answer);
}
diff --git a/src/bin/dhcp6/config_parser.cc b/src/bin/dhcp6/config_parser.cc
index 2ec9320..c0e6c07 100644
--- a/src/bin/dhcp6/config_parser.cc
+++ b/src/bin/dhcp6/config_parser.cc
@@ -101,7 +101,7 @@ StringStorage string_defaults;
OptionStorage option_defaults;
/// @brief Global storage for option definitions.
-OptionDefStorage option_def_defaults;
+OptionDefStorage option_def_intermediate;
/// @brief a dummy configuration parser
@@ -618,9 +618,20 @@ 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 then an
-/// instance of an option is created and added to the storage provided
-/// by the calling class.
+/// and data carried by the option. The option data can be specified
+/// in one of the two available formats: binary value represented as
+/// a string of hexadecimal digits or a list of comma separated values.
+/// The format being used is controlled by csv-format configuration
+/// parameter. When setting this value to True, the latter format is
+/// used. The subsequent values in the CSV format apply to relevant
+/// option data fields in the configured option. For example the
+/// configuration: "data" : "192.168.2.0, 56, hello world" can be
+/// used to set values for the option comprising IPv4 address,
+/// integer and string data field. Note that order matters. If the
+/// order of values does not match the order of data fields within
+/// an option the configuration will not be accepted. If parsing
+/// is successful then an instance of an option is created and
+/// added to the storage provided by the calling class.
class OptionDataParser : public DhcpConfigParser {
public:
@@ -716,6 +727,9 @@ public:
}
uint16_t opt_type = option_descriptor_.option->getType();
Subnet::OptionContainerPtr options = options_->getItems(option_space_);
+ // The getItems() should never return NULL pointer. If there are no
+ // options configured for the particular option space a pointer
+ // to an empty container should be returned.
assert(options);
Subnet::OptionContainerTypeIndex& idx = options->get<1>();
// Try to find options with the particular option code in the main
@@ -799,7 +813,10 @@ private:
// need to search for its definition among user-configured
// options. They are expected to be in the global storage
// already.
- OptionDefContainerPtr defs = option_def_defaults.getItems(option_space);
+ OptionDefContainerPtr defs = option_def_intermediate.getItems(option_space);
+ // The getItems() should never return the NULL pointer. If there are
+ // no option definitions for the particular option space a pointer
+ // to an empty container should be returned.
assert(defs);
const OptionDefContainerTypeIndex& idx = defs->get<1>();
OptionDefContainerTypeRange range = idx.equal_range(option_code);
@@ -1066,7 +1083,7 @@ public:
}
}
- /// @brief Stores the parsed option definition in a storage.
+ /// @brief Stores the parsed option definition in the data store.
void commit() {
// @todo validate option space name once 2313 is merged.
if (storage_ && option_definition_) {
@@ -1074,12 +1091,12 @@ public:
}
}
- /// @brief Sets a pointer to a storage.
+ /// @brief Sets a pointer to the data store.
///
/// The newly created instance of an option definition will be
- /// added to a storage given by the argument.
+ /// added to the data store given by the argument.
///
- /// @param storage pointer to a storage where the option definition
+ /// @param storage pointer to the data store where the option definition
/// will be added to.
void setStorage(OptionDefStorage* storage) {
storage_ = storage;
@@ -1113,7 +1130,7 @@ private:
// Split the list of record types into tokens.
std::vector<std::string> record_tokens =
isc::util::str::tokens(record_types, ",");
- // Iterate over each token and add a record typy into
+ // Iterate over each token and add a record type into
// option definition.
BOOST_FOREACH(std::string record_type, record_tokens) {
try {
@@ -1133,7 +1150,7 @@ private:
def->validate();
} catch (const isc::Exception ex) {
isc_throw(DhcpConfigError, "invalid option definition"
- << " parameters" << ex.what());
+ << " parameters: " << ex.what());
}
// Option definition has been created successfully.
option_space_name_ = space;
@@ -1185,7 +1202,7 @@ public:
void build(ConstElementPtr option_def_list) {
// Clear existing items in the global storage.
// We are going to replace all of them.
- option_def_defaults.clearItems();
+ option_def_intermediate.clearItems();
if (!option_def_list) {
isc_throw(DhcpConfigError, "parser error: a pointer to a list of"
@@ -1195,7 +1212,7 @@ public:
BOOST_FOREACH(ConstElementPtr option_def, option_def_list->listValue()) {
boost::shared_ptr<OptionDefParser>
parser(new OptionDefParser("single-option-def"));
- parser->setStorage(&option_def_defaults);
+ parser->setStorage(&option_def_intermediate);
parser->build(option_def);
parser->commit();
}
@@ -1211,10 +1228,13 @@ public:
// We need to move option definitions from the temporary
// storage to the global storage.
BOOST_FOREACH(std::string space_name,
- option_def_defaults.getOptionSpaceNames()) {
+ option_def_intermediate.getOptionSpaceNames()) {
BOOST_FOREACH(OptionDefinitionPtr def,
- *option_def_defaults.getItems(space_name)) {
+ *option_def_intermediate.getItems(space_name)) {
+ // All option definitions should be initialized to non-NULL
+ // values. The validation is expected to be made by the
+ // OptionDefParser when creating an option definition.
assert(def);
cfg_mgr.addOptionDef(def, space_name);
}
@@ -1403,7 +1423,9 @@ private:
// Get all options within a particular option space.
BOOST_FOREACH(Subnet::OptionDescriptor desc,
*options_.getItems(option_space)) {
- // In theory, option should be non-NULL.
+ // The pointer should be non-NULL. The validation is expected
+ // to be performed by the OptionDataParser before adding an
+ // option descriptor to the container.
assert(desc.option);
// We want to check whether an option with the particular
// option code has been already added. If so, we want
@@ -1429,6 +1451,9 @@ private:
// Get all global options for the particular option space.
BOOST_FOREACH(Subnet::OptionDescriptor desc,
*option_defaults.getItems(option_space)) {
+ // The pointer should be non-NULL. The validation is expected
+ // to be performed by the OptionDataParser before adding an
+ // option descriptor to the container.
assert(desc.option);
// Check if the particular option has been already added.
// This would mean that it has been configured in the
@@ -1644,7 +1669,9 @@ configureDhcp6Server(Dhcpv6Srv& , ConstElementPtr config_set) {
// other values. Typically, the values in the subnet4 structure
// depend on the global values. Also, option values configuration
// must be performed after the option definitions configurations.
- // Thus we group parsers and will fire them in the right order.
+ // Thus we group parsers and will fire them in the right order:
+ // all parsers other than subnet4 and option-data parser,
+ // option-data parser, subnet4 parser.
ParserCollection independent_parsers;
ParserPtr subnet_parser;
ParserPtr option_parser;
@@ -1659,7 +1686,7 @@ configureDhcp6Server(Dhcpv6Srv& , ConstElementPtr config_set) {
Uint32Storage uint32_local(uint32_defaults);
StringStorage string_local(string_defaults);
OptionStorage option_local(option_defaults);
- OptionDefStorage option_def_local(option_def_defaults);
+ OptionDefStorage option_def_local(option_def_intermediate);
// answer will hold the result.
ConstElementPtr answer;
@@ -1750,7 +1777,7 @@ configureDhcp6Server(Dhcpv6Srv& , ConstElementPtr config_set) {
std::swap(uint32_defaults, uint32_local);
std::swap(string_defaults, string_local);
std::swap(option_defaults, option_local);
- std::swap(option_def_defaults, option_def_local);
+ std::swap(option_def_intermediate, option_def_local);
return (answer);
}
diff --git a/src/lib/dhcpsrv/dhcp_config_parser.h b/src/lib/dhcpsrv/dhcp_config_parser.h
index 6b4dd36..d3d05f6 100644
--- a/src/lib/dhcpsrv/dhcp_config_parser.h
+++ b/src/lib/dhcpsrv/dhcp_config_parser.h
@@ -136,8 +136,8 @@ protected:
/// @throw DhcpConfigError if the entry has not been found
/// in the storage.
template<typename ReturnType, typename StorageType>
- ReturnType getParam(const std::string param_id,
- const StorageType& storage) const {
+ static ReturnType getParam(const std::string& param_id,
+ const StorageType& storage) {
typename StorageType::const_iterator param = storage.find(param_id);
if (param == storage.end()) {
isc_throw(DhcpConfigError, "missing parameter '"
diff --git a/src/lib/dhcpsrv/option_space_container.h b/src/lib/dhcpsrv/option_space_container.h
index 69093c1..f90bedd 100644
--- a/src/lib/dhcpsrv/option_space_container.h
+++ b/src/lib/dhcpsrv/option_space_container.h
@@ -50,6 +50,10 @@ public:
/// @brief Get all items for the particular option space.
///
+ /// @warning when there are no items for the specified option
+ /// space an empty container is created and returned. However
+ /// this container is not added to the list of option spaces.
+ ///
/// @param option_space name of the option space.
///
/// @return pointer to the container holding items.
More information about the bind10-changes
mailing list