From: Marcin Siodelski Date: Mon, 3 Nov 2014 18:56:41 +0000 (+0100) Subject: [3467] CSV format now defaults to true. X-Git-Tag: kea-eng-20141219~12^2~8 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=b4f0929aef870928b8212497b2445522ebc17c40;p=thirdparty%2Fkea.git [3467] CSV format now defaults to true. Also, changed internals of the OptionDataParser: - option parameters are validated in distnict functions to make code more readable - option data values are optional --- diff --git a/src/lib/dhcpsrv/dhcp_parsers.cc b/src/lib/dhcpsrv/dhcp_parsers.cc index ab083b97e2..f1a647bfc0 100644 --- a/src/lib/dhcpsrv/dhcp_parsers.cc +++ b/src/lib/dhcpsrv/dhcp_parsers.cc @@ -33,6 +33,7 @@ using namespace std; using namespace isc::asiolink; using namespace isc::data; using namespace isc::hooks; +using namespace isc::util; namespace isc { namespace dhcp { @@ -342,6 +343,108 @@ OptionDataParser::commit() { // Does nothing } +OptionalValue +OptionDataParser::extractCode() const { + uint32_t code; + try { + code = uint32_values_->getParam("code"); + + } catch (const exception& ex) { + return (OptionalValue(code)); + } + + if (code == 0) { + isc_throw(DhcpConfigError, "option code must not be zero " + "(" << uint32_values_->getPosition("code") << ")"); + + } else if (address_family_ == AF_INET && + code > std::numeric_limits::max()) { + isc_throw(DhcpConfigError, "invalid option code '" << code + << "', it must not be greater than '" + << static_cast(std::numeric_limits::max()) + << "' (" << uint32_values_->getPosition("code") << ")"); + + } else if (address_family_ == AF_INET6 && + code > std::numeric_limits::max()) { + isc_throw(DhcpConfigError, "invalid option code '" << code + << "', it must not exceed '" + << std::numeric_limits::max() + << "' (" << uint32_values_->getPosition("code") << ")"); + + } + + return (OptionalValue(code, OptionalValueState(true))); +} + +OptionalValue +OptionDataParser::extractName() const { + std::string name; + try { + name = string_values_->getParam("name"); + + } catch (...) { + return (OptionalValue(name)); + } + + if (name.empty()) { + isc_throw(DhcpConfigError, "option name is empty (" + << string_values_->getPosition("name") << ")"); + + } else if (name.find(" ") != std::string::npos) { + isc_throw(DhcpConfigError, "invalid option name '" << name + << "', space character is not allowed (" + << string_values_->getPosition("name") << ")"); + } + + return (OptionalValue(name, OptionalValueState(true))); +} + +std::string +OptionDataParser::extractData() const { + std::string data; + try { + data = string_values_->getParam("data"); + + } catch (...) { + return (data); + } + + return (data); +} + +OptionalValue +OptionDataParser::extractCSVFormat() const { + bool csv_format = true; + try { + csv_format = boolean_values_->getParam("csv-format"); + + } catch (...) { + return (OptionalValue(csv_format)); + } + + return (OptionalValue(csv_format, OptionalValueState(true))); +} + +std::string +OptionDataParser::extractSpace() const { + std::string space = address_family_ == AF_INET ? "dhcp4" : "dhcp6"; + try { + space = string_values_->getParam("space"); + + } catch (...) { + return (space); + } + + if (!OptionSpace::validateName(space)) { + isc_throw(DhcpConfigError, "invalid option space name '" + << space << "' (" + << string_values_->getPosition("space") << ")"); + } + + return (space); +} + + OptionDefinitionPtr OptionDataParser::findServerSpaceOptionDefinition(const std::string& option_space, const uint32_t option_code) const { @@ -377,64 +480,12 @@ void OptionDataParser::createOption(ConstElementPtr option_data) { const Option::Universe universe = address_family_ == AF_INET ? Option::V4 : Option::V6; - // Check if mandatory parameters are specified. - uint32_t code; - std::string name; - std::string data; - try { - code = uint32_values_->getParam("code"); - name = string_values_->getParam("name"); - data = string_values_->getParam("data"); - } catch (const std::exception& ex) { - isc_throw(DhcpConfigError, - ex.what() << "(" << option_data->getPosition() << ")"); - } - // Check parameters having default values. - std::string space = string_values_->getOptionalParam("space", universe == Option::V4 ? - "dhcp4" : "dhcp6"); - bool csv_format = boolean_values_->getOptionalParam("csv-format", false); - - // Option code is held in the uint32_t storage but is supposed to - // be uint16_t value. We need to check that value in the configuration - // does not exceed range of uint8_t for DHCPv4, uint16_t for DHCPv6 and - // is not zero. - if (code == 0) { - isc_throw(DhcpConfigError, "option code must not be zero " - "(" << uint32_values_->getPosition("code") << ")"); - - } else if (universe == Option::V4 && - code > std::numeric_limits::max()) { - isc_throw(DhcpConfigError, "invalid option code '" << code - << "', it must not exceed '" - << static_cast(std::numeric_limits::max()) - << "' (" << uint32_values_->getPosition("code") << ")"); - } else if (universe == Option::V6 && - code > std::numeric_limits::max()) { - isc_throw(DhcpConfigError, "invalid option code '" << code - << "', it must not exceed '" - << std::numeric_limits::max() - << "' (" << uint32_values_->getPosition("code") << ")"); - - } - - // Check that the option name is non-empty and does not contain spaces - if (name.empty()) { - isc_throw(DhcpConfigError, "name of the option with code '" - << code << "' is empty (" - << string_values_->getPosition("name") << ")"); - } else if (name.find(" ") != std::string::npos) { - isc_throw(DhcpConfigError, "invalid option name '" << name - << "', space character is not allowed (" - << string_values_->getPosition("name") << ")"); - } - - if (!OptionSpace::validateName(space)) { - isc_throw(DhcpConfigError, "invalid option space name '" - << space << "' specified for option '" - << name << "', code '" << code - << "' (" << string_values_->getPosition("space") << ")"); - } + OptionalValue code_param = extractCode(); + OptionalValue name_param = extractName(); + OptionalValue csv_format_param = extractCSVFormat(); + std::string data_param = extractData(); + std::string space_param = extractSpace(); // Find the Option Definition for the option by its option code. // findOptionDefinition will throw if not found, no need to test. @@ -443,7 +494,7 @@ OptionDataParser::createOption(ConstElementPtr option_data) { // configuration. OptionDefinitionPtr def; try { - def = findServerSpaceOptionDefinition(space, code); + def = findServerSpaceOptionDefinition(space_param, code_param); } catch (const std::exception& ex) { isc_throw(DhcpConfigError, ex.what() @@ -454,14 +505,15 @@ OptionDataParser::createOption(ConstElementPtr option_data) { // need to search for its definition among user-configured // options. They are expected to be in the global storage // already. - def = CfgMgr::instance().getStagingCfg()->getCfgOptionDef()->get(space, code); + def = CfgMgr::instance().getStagingCfg()->getCfgOptionDef() + ->get(space_param, code_param); // It's ok if we don't have option format if the option is // specified as hex - if (!def && csv_format) { + if (!def && csv_format_param) { isc_throw(DhcpConfigError, "definition for the option '" - << space << "." << name - << "' having code '" << code + << space_param << "." << name_param + << "' having code '" << code_param << "' does not exist (" << string_values_->getPosition("name") << ")"); } @@ -471,12 +523,12 @@ OptionDataParser::createOption(ConstElementPtr option_data) { std::vector binary; std::vector data_tokens; - if (csv_format) { + if (!csv_format_param.isSpecified() || csv_format_param) { // If the option data is specified as a string of comma // separated values then we need to split this string into // individual values - each value will be used to initialize // one data field of an option. - data_tokens = isc::util::str::tokens(data, ","); + data_tokens = isc::util::str::tokens(data_param, ","); } else { // Otherwise, the option data is specified as a string of // hexadecimal digits that we have to turn into binary format. @@ -484,23 +536,23 @@ OptionDataParser::createOption(ConstElementPtr option_data) { // The decodeHex function expects that the string contains an // even number of digits. If we don't meet this requirement, // we have to insert a leading 0. - if (!data.empty() && data.length() % 2) { - data = data.insert(0, "0"); + if (!data_param.empty() && data_param.length() % 2) { + data_param = data_param.insert(0, "0"); } - util::encode::decodeHex(data, binary); + util::encode::decodeHex(data_param, binary); } catch (...) { isc_throw(DhcpConfigError, "option data is not a valid" - << " string of hexadecimal digits: " << data + << " string of hexadecimal digits: " << data_param << " (" << string_values_->getPosition("data") << ")"); } } OptionPtr option; if (!def) { - if (csv_format) { + if (csv_format_param.isSpecified() && csv_format_param) { isc_throw(DhcpConfigError, "the CSV option data format can be" " used to specify values for an option that has a" - " definition. The option with code " << code + " definition. The option with code " << code_param << " does not have a definition (" << boolean_values_->getPosition("csv-format") << ")"); } @@ -511,13 +563,14 @@ OptionDataParser::createOption(ConstElementPtr option_data) { // definition does not exist for a particular option code. For now it is // ok to create generic option if definition does not exist. OptionPtr option(new Option(universe, - static_cast(code), binary)); + static_cast(code_param), binary)); // The created option is stored in option_descriptor_ class member // until the commit stage when it is inserted into the main storage. // If an option with the same code exists in main storage already the // old option is replaced. option_descriptor_.option_ = option; option_descriptor_.persistent_ = false; + } else { // Option name should match the definition. The option name @@ -525,10 +578,10 @@ OptionDataParser::createOption(ConstElementPtr option_data) { // to reference options and definitions using their names // and/or option codes so keeping the option name in the // definition of option value makes sense. - if (def->getName() != name) { + if (def->getName() != name_param.get()) { isc_throw(DhcpConfigError, "specified option name '" - << name << "' does not match the " - << "option definition: '" << space + << name_param << "' does not match the " + << "option definition: '" << space_param << "." << def->getName() << "' (" << string_values_->getPosition("name") << ")"); } @@ -536,24 +589,24 @@ OptionDataParser::createOption(ConstElementPtr option_data) { // Option definition has been found so let's use it to create // an instance of our option. try { - OptionPtr option = csv_format ? - def->optionFactory(universe, code, data_tokens) : - def->optionFactory(universe, code, binary); + OptionPtr option = csv_format_param ? + def->optionFactory(universe, code_param, data_tokens) : + def->optionFactory(universe, code_param, binary); OptionDescriptor desc(option, false); option_descriptor_.option_ = option; option_descriptor_.persistent_ = false; } catch (const isc::Exception& ex) { isc_throw(DhcpConfigError, "option data does not match" - << " option definition (space: " << space - << ", code: " << code << "): " + << " option definition (space: " << space_param + << ", code: " << code_param << "): " << ex.what() << " (" << string_values_->getPosition("data") << ")"); } } // All went good, so we can set the option space name. - option_space_ = space; + option_space_ = space_param; } // **************************** OptionDataListParser ************************* diff --git a/src/lib/dhcpsrv/dhcp_parsers.h b/src/lib/dhcpsrv/dhcp_parsers.h index 92af0b9b76..6ea69ed9d3 100644 --- a/src/lib/dhcpsrv/dhcp_parsers.h +++ b/src/lib/dhcpsrv/dhcp_parsers.h @@ -25,6 +25,7 @@ #include #include #include +#include #include @@ -574,6 +575,16 @@ private: /// are invalid. void createOption(isc::data::ConstElementPtr option_data); + util::OptionalValue extractCode() const; + + util::OptionalValue extractName() const; + + util::OptionalValue extractCSVFormat() const; + + std::string extractData() const; + + std::string extractSpace() const; + /// Storage for boolean values. BooleanStoragePtr boolean_values_; diff --git a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc index d92886fd72..58bc8bfa2a 100644 --- a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc +++ b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc @@ -551,6 +551,25 @@ TEST_F(ParseConfigTest, basicOptionDataTest) { EXPECT_EQ(val, opt_ptr->toText()); } +TEST_F(ParseConfigTest, defaultCSVFormat) { + std::string config = + "{ \"option-data\": [ {" + " \"name\": \"swap-server\"," + " \"space\": \"dhcp4\"," + " \"code\": 16," + " \"data\": \"192.0.2.0\"" + " } ]" + "}"; + + int rcode = 0; + ASSERT_NO_THROW(rcode = parseConfiguration(config)); + ASSERT_TRUE(rcode == 0); + + OptionPtr opt_ptr = getOptionPtr("dhcp4", 16); + OptionCustomPtr addr_opt = boost::dynamic_pointer_cast< + OptionCustom>(opt_ptr); +} + }; // Anonymous namespace /// These tests check basic operation of the HooksLibrariesParser. diff --git a/src/lib/util/optional_value.h b/src/lib/util/optional_value.h index 1ecabce28c..eb194fcc08 100644 --- a/src/lib/util/optional_value.h +++ b/src/lib/util/optional_value.h @@ -15,9 +15,29 @@ #ifndef OPTIONAL_VALUE_H #define OPTIONAL_VALUE_H +#include + namespace isc { namespace util { +/// @brief Indicate if an @c OptionalValue is is specified or not. +/// +/// This is a simple wrapper class which holds a boolean value to indicate +/// if the @c OptionalValue is specified or not. By using this class in the +/// @c OptionalValue class constructor we avoid the ambiguity when the +/// @c OptionalValue encapsulates a bool type. +struct OptionalValueState { + + /// @brief Constructor. + /// + /// @param specified A boolean value to be assigned. + OptionalValueState(const bool specified) + : specified_(specified) { + } + /// @brief A bool value encapsulated by this structure. + bool specified_; +}; + /// @brief Simple class representing an optional value. /// /// This template class encapsulates a value of any type. An additional flag @@ -43,16 +63,24 @@ template class OptionalValue { public: + /// @brief Default constructor. + /// + /// Note that the type @c T must have a default constructor to use this + /// constructor. + OptionalValue() + : value_(T()), specified_(false) { + } + /// @brief Constructor /// /// Creates optional value. The value defaults to "unspecified". /// /// @param value Default explicit value. - /// @param specified Boolean value which determines if the value is - /// initially specified or not (default is false). - OptionalValue(const T& value, const bool specified = false) - : value_(value), - specified_(specified) { + /// @param specified Value which determines if the value is initially + // specified or not (default is false). + explicit OptionalValue(const T& value, const OptionalValueState& state = + OptionalValueState(false)) + : value_(value), specified_(state.specified_) { } /// @brief Retrieves the actual value. @@ -75,6 +103,10 @@ public: specify(true); } + void specify(const OptionalValueState& state) { + specified_ = state.specified_; + } + /// @brief Sets the value to "specified" or "unspecified". /// /// It does not alter the actual value. It only marks it "specified" or @@ -131,6 +163,25 @@ private: bool specified_; ///< Flag which indicates if the value is specified. }; +/// @brief Inserts an optional value to a stream. +/// +/// This function overloads the global operator<< to behave as in +/// @c ostream::operator<< but applied to @c OptionalValue objects. +/// +/// @param os A @c std::ostream object to which the value is inserted. +/// @param optional_value An @c OptionalValue object to be inserted into +/// a stream. +/// @tparam Type of the value encapsulated by the @c OptionalValue object. +/// +/// @return A reference to the stream after insertion. +template +std::ostream& +operator<<(std::ostream& os, const OptionalValue& optional_value) { + os << optional_value.get(); + return (os); +} + + } // end of namespace isc::util } // end of namespace isc