From: Thomas Markwalder Date: Fri, 19 Feb 2021 13:42:38 +0000 (-0500) Subject: [#1635] Addressed review comments X-Git-Tag: Kea-1.9.5~24 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e3b16f4cbf014c9e32c3130d258c1cf550fd032d;p=thirdparty%2Fkea.git [#1635] Addressed review comments Changes were cosmetic, no fucntional changes required. Modified: doc/sphinx/arm/dhcp4-srv.rst src/bin/dhcp4/tests/config_parser_unittest.cc src/lib/cc/simple_parser.h src/lib/dhcpsrv/alloc_engine.cc src/lib/dhcpsrv/alloc_engine.h src/lib/dhcpsrv/client_class_def.cc src/lib/dhcpsrv/client_class_def.h src/lib/dhcpsrv/parsers/shared_network_parser.cc src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc --- diff --git a/doc/sphinx/arm/dhcp4-srv.rst b/doc/sphinx/arm/dhcp4-srv.rst index 42dbd048c9..b8c3396ed4 100644 --- a/doc/sphinx/arm/dhcp4-srv.rst +++ b/doc/sphinx/arm/dhcp4-srv.rst @@ -182,33 +182,33 @@ client will begin the renewal and rebind procedures. .. note:: - Beginning with Kea 1.6.0 the lease valid lifetime is extended from a - single value to a triplet with minimum, default and maximum values using - ``min-valid-lifetime``, ``valid-lifetime`` and ``max-valid-lifetime``. - As of Kea 1.9.5, these values may be specified in client classes. The recipe - the server uses to select which lifetime value to use is as follows: - - If the client query is a BOOTP query, the server will always use the - infinite lease time (e.g.0xffffffff). Otherwise the server must next - determine which configured triplet to use by first searching all - classes assigned to the query, and then the subnet selected for - the query. - - Classes are searched in the order they were assigned to the query. The - server will use the triplet from the first class that specifies it. - If no classes specify the triplet then the server will use the triplet - specified by the subnet selected for the client. If the subnet does not - explicitly specify it the server will look next at the subnet's - shared-network (if one), then for a global specification, and - finally the global default. - - If the client requested a lifetime value via DHCP option 51, then the - lifetime value used will be the requested value bounded by the configured - triplet. In other words, If the requested lifetime is less than the - configured minimum the configured minimum will be used; if it is more - than the configured maximum the configured maximum will be used. If - the client did not provide a requested value, the lifetime value used - is the simple be the triplet default value. + Beginning with Kea 1.6.0 the lease valid lifetime is extended from a + single value to a triplet with minimum, default and maximum values using + ``min-valid-lifetime``, ``valid-lifetime`` and ``max-valid-lifetime``. + As of Kea 1.9.5, these values may be specified in client classes. The recipe + the server uses to select which lifetime value to use is as follows: + + If the client query is a BOOTP query, the server will always use the + infinite lease time (e.g. 0xffffffff). Otherwise the server must next + determine which configured triplet to use by first searching all + classes assigned to the query, and then the subnet selected for + the query. + + Classes are searched in the order they were assigned to the query. The + server will use the triplet from the first class that specifies it. + If no classes specify the triplet then the server will use the triplet + specified by the subnet selected for the client. If the subnet does not + explicitly specify it the server will look next at the subnet's + shared-network (if one), then for a global specification, and + finally the global default. + + If the client requested a lifetime value via DHCP option 51, then the + lifetime value used will be the requested value bounded by the configured + triplet. In other words, if the requested lifetime is less than the + configured minimum the configured minimum will be used; if it is more + than the configured maximum the configured maximum will be used. If + the client did not provide a requested value, the lifetime value used + will be the triplet default value. .. note:: diff --git a/src/bin/dhcp4/tests/config_parser_unittest.cc b/src/bin/dhcp4/tests/config_parser_unittest.cc index 22180d9683..bbbb976361 100644 --- a/src/bin/dhcp4/tests/config_parser_unittest.cc +++ b/src/bin/dhcp4/tests/config_parser_unittest.cc @@ -6088,7 +6088,6 @@ TEST_F(Dhcp4ParserTest, clientClassValidLifetime) { class_def = dictionary->findClass("two"); ASSERT_TRUE(class_def); EXPECT_TRUE(class_def->getValid().unspecified()); - } diff --git a/src/lib/cc/simple_parser.h b/src/lib/cc/simple_parser.h index 5bed826345..ce18650735 100644 --- a/src/lib/cc/simple_parser.h +++ b/src/lib/cc/simple_parser.h @@ -321,16 +321,16 @@ public: /// @brief Parses a integer triplet /// - /// Parse an integer triplet parameter of the form: + /// Parses an integer triplet parameter of the form: /// /// min-, , max- /// - /// @param scope Data element holding e.g. shared network configuration + /// @param scope Data element holding e.g. shared network configuration /// to be parsed. /// @param name Base name of the parameter. /// @return A triplet with the parsed value. const dhcp::Triplet parseIntTriplet(const data::ConstElementPtr& scope, - const std::string& name); + const std::string& name); }; }; diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 61e93e1bb5..251133427d 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -3845,7 +3845,7 @@ AllocEngine::getValidLft(const ClientContext4& ctx) { // If it's BOOTP, use infinite valid lifetime. if (ctx.query_->inClass("BOOTP")) { - return(Lease::INFINITY_LFT); + return (Lease::INFINITY_LFT); } // Use the dhcp-lease-time content from the client if it's there. @@ -3858,10 +3858,9 @@ AllocEngine::getValidLft(const ClientContext4& ctx) { } } - // If the triplet is specified in one of our classes use it. // We use the first one we find. - Tripletcandidate_lft; + Triplet candidate_lft; const ClientClasses classes = ctx.query_->getClasses(); if (!classes.empty()) { // Let's get class definitions @@ -3887,7 +3886,7 @@ AllocEngine::getValidLft(const ClientContext4& ctx) { // If client requested a value, use the value bounded by // the candidate triplet. if (requested_lft > 0) { - return(candidate_lft.get(requested_lft)); + return (candidate_lft.get(requested_lft)); } // Use the candidate's default value. diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index 8973b6e4be..d0b7a06d93 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/src/lib/dhcpsrv/alloc_engine.h @@ -1640,7 +1640,7 @@ public: /// it simply returns the candidate triplet's default value. /// /// @param ctx Client context holding various information about the client. - /// @return unsigned integer value of the valid lifetime to use + /// @return unsigned integer value of the valid lifetime to use. static uint32_t getValidLft(const ClientContext4& ctx); private: diff --git a/src/lib/dhcpsrv/client_class_def.cc b/src/lib/dhcpsrv/client_class_def.cc index 1a0e02a5fe..d038a016db 100644 --- a/src/lib/dhcpsrv/client_class_def.cc +++ b/src/lib/dhcpsrv/client_class_def.cc @@ -197,14 +197,16 @@ ClientClassDef:: toElement() const { // Set valid-lifetime if (!valid_.unspecified()) { result->set("valid-lifetime", - Element::create(static_cast(valid_.get()))); + Element::create(static_cast(valid_.get()))); + if (valid_.getMin() < valid_.get()) { result->set("min-valid-lifetime", - Element::create(static_cast(valid_.getMin()))); + Element::create(static_cast(valid_.getMin()))); } + if (valid_.getMax() > valid_.get()) { result->set("max-valid-lifetime", - Element::create(static_cast(valid_.getMax()))); + Element::create(static_cast(valid_.getMax()))); } } @@ -282,7 +284,7 @@ ClientClassDictionary::findClass(const std::string& name) const { return (*it).second; } - return(ClientClassDefPtr()); + return (ClientClassDefPtr()); } void diff --git a/src/lib/dhcpsrv/client_class_def.h b/src/lib/dhcpsrv/client_class_def.h index 990a16d750..e5eb160755 100644 --- a/src/lib/dhcpsrv/client_class_def.h +++ b/src/lib/dhcpsrv/client_class_def.h @@ -192,6 +192,7 @@ public: } /// @brief Return valid-lifetime value + /// @return a triplet containing the valid lifetime. Triplet getValid() const { return(valid_); } diff --git a/src/lib/dhcpsrv/parsers/shared_network_parser.cc b/src/lib/dhcpsrv/parsers/shared_network_parser.cc index ca75264d78..1eb7c87102 100644 --- a/src/lib/dhcpsrv/parsers/shared_network_parser.cc +++ b/src/lib/dhcpsrv/parsers/shared_network_parser.cc @@ -244,7 +244,7 @@ SharedNetwork6Parser::parse(const data::ConstElementPtr& shared_network_data) { // preferred-lifetime shared_network->setPreferred(parseIntTriplet(shared_network_data, - "preferred-lifetime")); + "preferred-lifetime")); // Get interface-id option content. For now we support string // representation only diff --git a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc index 44805fbe1e..7d434a241b 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc @@ -4291,7 +4291,7 @@ TEST_F(AllocEngine4Test, getValidLft4) { ClientClassDictionaryPtr dictionary = CfgMgr::instance().getStagingCfg()->getClientClassDictionary(); ClientClassDefPtr class_def(new ClientClassDef("valid_one", ExpressionPtr())); - Tripletvalid_one(50,100,150); + Triplet valid_one(50, 100, 150); class_def->setValid(valid_one); dictionary->addClass(class_def); @@ -4307,7 +4307,7 @@ TEST_F(AllocEngine4Test, getValidLft4) { CfgMgr::instance().commit(); // Update the subnet's triplet to something more useful. - subnet_->setValid(Triplet(500,1000,1500)); + subnet_->setValid(Triplet(500, 1000, 1500)); // Describes a test scenario. struct Scenario { @@ -4321,7 +4321,7 @@ TEST_F(AllocEngine4Test, getValidLft4) { std::vector scenarios = { { "BOOTP", - {"BOOTP"}, + { "BOOTP" }, 0, Lease::INFINITY_LFT }, @@ -4369,19 +4369,19 @@ TEST_F(AllocEngine4Test, getValidLft4) { }, { "class plus option", - {"valid_one"}, + { "valid_one" }, valid_one.getMin() + 25, valid_one.getMin() + 25 }, { "class plus option too small", - {"valid_one"}, + { "valid_one" }, valid_one.getMin() - 25, valid_one.getMin() }, { "class plus option too big", - {"valid_one"}, + { "valid_one" }, valid_one.getMax() + 25, valid_one.getMax() } diff --git a/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc b/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc index 0e786c5001..f1a87aabce 100644 --- a/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc +++ b/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc @@ -1303,29 +1303,29 @@ TEST_F(ClientClassDefParserTest, validLifetimeTests) { std::vector scenarios = { { - "unspecified", - "", - Triplet() + "unspecified", + "", + Triplet() }, { - "valid only", - "\"valid-lifetime\": 100", - Triplet(100) + "valid only", + "\"valid-lifetime\": 100", + Triplet(100) }, { - "min only", - "\"min-valid-lifetime\": 50", - Triplet(50, 50, 50) + "min only", + "\"min-valid-lifetime\": 50", + Triplet(50, 50, 50) }, { - "max only", - "\"max-valid-lifetime\": 75", - Triplet(75, 75, 75) + "max only", + "\"max-valid-lifetime\": 75", + Triplet(75, 75, 75) }, { - "all three", - "\"min-valid-lifetime\": 25, \"valid-lifetime\": 50, \"max-valid-lifetime\": 75", - Triplet(25, 50, 75) + "all three", + "\"min-valid-lifetime\": 25, \"valid-lifetime\": 50, \"max-valid-lifetime\": 75", + Triplet(25, 50, 75) } }; @@ -1336,7 +1336,7 @@ TEST_F(ClientClassDefParserTest, validLifetimeTests) { if (!scenario.cfg_txt_.empty()) { oss << ",\n" << scenario.cfg_txt_; } - oss << "\n}\n"; + oss << "\n}\n"; ClientClassDefPtr class_def; ASSERT_NO_THROW_LOG(class_def = parseClientClassDef(oss.str(), AF_INET));