From: Tomek Mrugalski Date: Thu, 22 Dec 2016 17:43:21 +0000 (+0100) Subject: [5039] Changes after review: X-Git-Tag: trac5021_base~3^2~5 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=65d187c0e6ea11817188b10d1641f6eb7a6caa4b;p=thirdparty%2Fkea.git [5039] Changes after review: - address_family_, default ctor removed from two parsers - copyright year updated - Element::getNonConst is now documented - user's guide is now clear about csv-format default value is now true --- diff --git a/doc/guide/dhcp4-srv.xml b/doc/guide/dhcp4-srv.xml index 4c7c3efa4c..6f58baf6b2 100644 --- a/doc/guide/dhcp4-srv.xml +++ b/doc/guide/dhcp4-srv.xml @@ -1578,22 +1578,12 @@ It is merely echoed by the server - csv-format - if this value is not specified - and the definition for the particular option exists, the server will assume - that the option data is specified as a list of comma separated values to be - assigned to individual fields of the DHCP option. If the definition - does not exist for this option, the server will assume that the data - parameter contains the option payload in the binary format (represented - as a string of hexadecimal digits). Note that not specifying this - parameter doesn't imply that it defaults to a fixed value, but - the configuration data interpretation also depends on the presence - of the option definition. An administrator must be aware if the - definition for the particular option exists when this parameter - is not specified. It is generally recommended to not specify this - parameter only for the options for which the definition exists, e.g. - standard options. Setting csv-format to an explicit - value will cause the server to strictly check the format of the option - data specified. + csv-format - if this value is not + specified the server will assume that the option data is specified as + a list of comma separated values to be assigned to individual fields + of the DHCP option. This behavior has changed in Kea 1.2. Older + versions used additional logic to determined whether the csv-format + should be true or false. That is no longer the case. diff --git a/doc/guide/dhcp6-srv.xml b/doc/guide/dhcp6-srv.xml index 8fb04a40d1..fc0d79c290 100644 --- a/doc/guide/dhcp6-srv.xml +++ b/doc/guide/dhcp6-srv.xml @@ -1721,22 +1721,12 @@ should include options from the isc option space: - csv-format - if this value is not specified - and the definition for the particular option exists, the server will assume - that the option data is specified as a list of comma separated values to be - assigned to individual fields of the DHCP option. If the definition - does not exist for this option, the server will assume that the data - parameter contains the option payload in the binary format (represented - as a string of hexadecimal digits). Note that not specifying this - parameter doesn't imply that it defaults to a fixed value, but - the configuration data interpretation also depends on the presence - of the option definition. An administrator must be aware if the - definition for the particular option exists when this parameter - is not specified. It is generally recommended to not specify this - parameter only for the options for which the definition exists, e.g. - standard options. Setting csv-format to an explicit - value will cause the server to strictly check the format of the option - data specified. + csv-format - if this value is not + specified the server will assume that the option data is specified as + a list of comma separated values to be assigned to individual fields + of the DHCP option. This behavior has changed in Kea 1.2. Older + versions used additional logic to determined whether the csv-format + should be true or false. That is no longer the case. diff --git a/src/bin/dhcp4/json_config_parser.cc b/src/bin/dhcp4/json_config_parser.cc index 2109c7b8f2..37c1262d88 100644 --- a/src/bin/dhcp4/json_config_parser.cc +++ b/src/bin/dhcp4/json_config_parser.cc @@ -575,7 +575,7 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) { // We need definitions first ConstElementPtr option_defs = mutable_cfg->get("option-def"); if (option_defs) { - OptionDefListParser parser(AF_INET); + OptionDefListParser parser; CfgOptionDefPtr cfg_option_def = CfgMgr::instance().getStagingCfg()->getCfgOptionDef(); parser.parse(cfg_option_def, option_defs); } diff --git a/src/bin/dhcp6/json_config_parser.cc b/src/bin/dhcp6/json_config_parser.cc index 9a6c8483eb..98e1c081b0 100644 --- a/src/bin/dhcp6/json_config_parser.cc +++ b/src/bin/dhcp6/json_config_parser.cc @@ -851,7 +851,7 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) { // We need definitions first ConstElementPtr option_defs = mutable_cfg->get("option-def"); if (option_defs) { - OptionDefListParser parser(AF_INET6); + OptionDefListParser parser; CfgOptionDefPtr cfg_option_def = CfgMgr::instance().getStagingCfg()->getCfgOptionDef(); parser.parse(cfg_option_def, option_defs); } diff --git a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc index ac73374dd6..d3fdf665d0 100644 --- a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc +++ b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc @@ -2024,7 +2024,6 @@ TEST_F(Dhcpv6SrvTest, rsooOverride) { " \"option-def\": [ {" " \"name\": \"foo\"," " \"code\": 120," - " \"csv-format\": false," " \"type\": \"binary\"" " } ]," " \"option-data\": [ {" diff --git a/src/bin/dhcp6/tests/simple_parser6_unittest.cc~ b/src/bin/dhcp6/tests/simple_parser6_unittest.cc~ new file mode 100644 index 0000000000..fb7dfc250d --- /dev/null +++ b/src/bin/dhcp6/tests/simple_parser6_unittest.cc~ @@ -0,0 +1,79 @@ +#include +#include +#include + +using namespace isc; +using namespace isc::data; + +namespace { + +/// @brief DHCP Parser test fixture class +class SimpleParser6Test : public ::testing::Test { + + /// @brief Checks if specified map has an integer parameter with expected value + /// + /// @param map map to be checked + /// @param param_name name of the parameter to be checked + /// @param exp_value expected value of the parameter. + void checkIntegerValue(const ConstElementPtr& map, const std::string& param_name, + int64_t exp_value) { + + // First check if the passed element is a map. + ASSERT_EQ(Element::map, map->getType()); + + // Now try to get the element being checked + ConstElementPtr elem = map->get(param_name); + ASSERT_TRUE(elem); + + // Now check if it's indeed integer + ASSERT_EQ(Element::integer, elem->getType()); + + // Finally, check if its value meets expectation. + EXPECT_EQ(exp_value, elem->intValue()); + } +}; + +// This test checks if global defaults are properly set for DHCPv6. +TEST_F(SimpleParser6Test, globalDefaults6) { + + ElementPtr empty = Element::fromJSON("{ }"); + size_t num; + + EXPECT_NO_THROW(num = SimpleParser6::setDefaults(empty)); + + // We expect at least 4 parameters to be inserted. + EXPECT_TRUE(num >= 4); + + checkIntegerValue(empty, "valid-lifetime", 7200); + checkIntegerValue(empty, "preferred-lifetime", 3600); + checkIntegerValue(empty, "rebind-timer", 1800); + checkIntegerValue(empty, "renew-timer", 900); +} + +// This test checks if the parameters can be inherited from the global +// scope to the subnet scope. +TEST_F(SimpleParser6Test, inheritGlobalToSubnet6) { + ElementPtr global = Element::fromJSON("{ \"renew-timer\": 1," + " \"rebind-timer\": 2," + " \"preferred-lifetime\": 3," + " \"valid-lifetime\": 4" + "}"); + ElementPtr subnet = Element::fromJSON("{ \"renew-timer\": 100 }"); + + // we should inherit 3 parameters. Renew-timer should remain intact, + // as it was already defined in the subnet scope. + size_t num; + EXPECT_NO_THROW(num = SimpleParser::deriveParams(global, subnet, + SimpleParser6::INHERIT_GLOBAL_TO_SUBNET6)); + EXPECT_EQ(3, num); + + // Check the values. 3 of them are interited, while the fourth one + // was already defined in the subnet, so should not be inherited. + checkIntegerValue(subnet, "renew-timer", 100); + checkIntegerValue(subnet, "rebind-timer", 2); + checkIntegerValue(subnet, "preferred-lifetime", 3); + checkIntegerValue(subnet, "valid-lifetime", 4); +} + +}; + diff --git a/src/lib/cc/data.cc b/src/lib/cc/data.cc index 6430fb6dee..fde27e7571 100644 --- a/src/lib/cc/data.cc +++ b/src/lib/cc/data.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2010-2015 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2010-2016 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this diff --git a/src/lib/cc/data.h b/src/lib/cc/data.h index c4b985689c..41c6ee3013 100644 --- a/src/lib/cc/data.h +++ b/src/lib/cc/data.h @@ -1,4 +1,4 @@ -// Copyright (C) 2010-2015 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2010-2016 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -277,6 +277,10 @@ public: /// \param i The position of the ElementPtr to return virtual ConstElementPtr get(const int i) const; + /// \brief returns element as non-const pointer + /// + /// \param i The position of the ElementPtr to retrieve + /// \return specified element pointer virtual ElementPtr getNonConst(const int i); /// Sets the ElementPtr at the given index. If the index is out diff --git a/src/lib/dhcpsrv/parsers/dhcp_parsers.cc b/src/lib/dhcpsrv/parsers/dhcp_parsers.cc index f0376013e7..54dfaff06a 100644 --- a/src/lib/dhcpsrv/parsers/dhcp_parsers.cc +++ b/src/lib/dhcpsrv/parsers/dhcp_parsers.cc @@ -693,9 +693,6 @@ void OptionDataListParser::parse(const CfgOptionPtr& cfg, } // ******************************** OptionDefParser **************************** -OptionDefParser::OptionDefParser(const uint16_t address_family) - : address_family_(address_family) { -} std::pair OptionDefParser::parse(ConstElementPtr option_def) { @@ -779,10 +776,6 @@ OptionDefParser::parse(ConstElementPtr option_def) { } // ******************************** OptionDefListParser ************************ -OptionDefListParser::OptionDefListParser(const uint16_t address_family) - : address_family_(address_family) { -} - void OptionDefListParser::parse(CfgOptionDefPtr storage, ConstElementPtr option_def_list) { if (!option_def_list) { @@ -791,7 +784,7 @@ OptionDefListParser::parse(CfgOptionDefPtr storage, ConstElementPtr option_def_l << option_def_list->getPosition() << ")"); } - OptionDefParser parser(address_family_); + OptionDefParser parser; BOOST_FOREACH(ConstElementPtr option_def, option_def_list->listValue()) { OptionDefinitionTuple def; diff --git a/src/lib/dhcpsrv/parsers/dhcp_parsers.h b/src/lib/dhcpsrv/parsers/dhcp_parsers.h index 365481b47e..9b69dd199e 100644 --- a/src/lib/dhcpsrv/parsers/dhcp_parsers.h +++ b/src/lib/dhcpsrv/parsers/dhcp_parsers.h @@ -681,11 +681,6 @@ typedef std::pair OptionDefinitionT /// This parser creates an instance of a single option definition. class OptionDefParser : public isc::data::SimpleParser { public: - /// @brief Constructor. - /// - /// @param address_family Address family: @c AF_INET or AF_INET6 - OptionDefParser(const uint16_t address_family); - /// @brief Parses an entry that describes single option definition. /// /// @param option_def a configuration entry to be parsed. @@ -694,10 +689,6 @@ public: /// @throw DhcpConfigError if parsing was unsuccessful. OptionDefinitionTuple parse(isc::data::ConstElementPtr option_def); - -private: - /// @brief Address family: @c AF_INET or @c AF_INET6 - uint16_t address_family_; }; /// @brief Parser for a list of option definitions. @@ -708,14 +699,6 @@ private: /// is put into the provided storage. class OptionDefListParser : public isc::data::SimpleParser { public: - /// @brief Constructor. - /// - /// Stores address family that will be used to make certain decisions - /// during parsing. - /// - /// @param address_family @c AF_INET or @c AF_INET6 - OptionDefListParser(const uint16_t address_family); - /// @brief Parses a list of option defintions, create them and store in cfg /// /// This method iterates over def_list, which is a JSON list of option defintions, @@ -725,11 +708,6 @@ public: /// @param def_list JSON list describing option definitions /// @param cfg parsed option definitions will be stored here void parse(CfgOptionDefPtr cfg, isc::data::ConstElementPtr def_list); - -protected: - - /// @brief Address family: @c AF_INET or @c AF_INET6 - uint16_t address_family_; }; /// @brief a collection of pools diff --git a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc index 52e63bf782..c29475521a 100644 --- a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc +++ b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc @@ -352,7 +352,7 @@ public: if (def_config != values_map.end()) { CfgOptionDefPtr cfg_def = CfgMgr::instance().getStagingCfg()->getCfgOptionDef(); - OptionDefListParser def_list_parser(family); + OptionDefListParser def_list_parser; def_list_parser.parse(cfg_def, def_config->second); }