From: Razvan Becheriu Date: Tue, 4 Aug 2020 12:03:41 +0000 (+0300) Subject: [#1065] addresses review X-Git-Tag: Kea-1.8.0~101 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=5a1c9a7fd896e59aa72a0939bb4d7a1ec0d407b1;p=thirdparty%2Fkea.git [#1065] addresses review --- diff --git a/src/hooks/dhcp/lease_cmds/lease_cmds.cc b/src/hooks/dhcp/lease_cmds/lease_cmds.cc index 1d099428d3..688c3b0392 100644 --- a/src/hooks/dhcp/lease_cmds/lease_cmds.cc +++ b/src/hooks/dhcp/lease_cmds/lease_cmds.cc @@ -85,7 +85,9 @@ public: /// Supported values are: "address", hw-address and duid. /// /// @param txt text to be converted + /// /// @return value converted to Parameters::Type + /// /// @throw BadValue if unsupported type is specified static Type txtToType(const std::string& txt) { if (txt == "address") { @@ -131,6 +133,7 @@ public: /// /// @param handle Callout context - which is expected to contain the /// add command JSON text in the "command" argument + /// /// @return 0 upon success, non-zero otherwise int leaseAddHandler(CalloutHandle& handle); @@ -142,6 +145,7 @@ public: /// /// @param handle Callout context - which is expected to contain the /// add command JSON text in the "command" argument + /// /// @return 0 upon success, non-zero otherwise int lease6BulkApplyHandler(CalloutHandle& handle); @@ -152,6 +156,7 @@ public: /// /// @param handle Callout context - which is expected to contain the /// get command JSON text in the "command" argument + /// /// @return 0 upon success, non-zero otherwise int leaseGetHandler(CalloutHandle& handle); @@ -165,6 +170,7 @@ public: /// /// @param handle Callout context - which is expected to contain the /// get command JSON text in the "command" argument + /// /// @return 0 upon success, non-zero otherwise. int leaseGetAllHandler(CalloutHandle& handle); @@ -181,6 +187,7 @@ public: /// /// @param handle Callout context - which is expected to contain the /// get commands JSON text in the "command" argument. + /// /// @return 0 if the handler has been invoked successfully, 1 if an /// error occurs, 3 if no leases are returned. int @@ -192,6 +199,7 @@ public: /// /// @param handle Callout context - which is expected to contain the /// get command JSON text in the "command" argument + /// /// @return 0 if the handler has been invoked successfully, 1 if an /// error occurs, 3 if no leases are returned. int @@ -203,6 +211,7 @@ public: /// /// @param handle Callout context - which is expected to contain the /// get command JSON text in the "command" argument + /// /// @return 0 if the handler has been invoked successfully, 1 if an /// error occurs, 3 if no leases are returned. int @@ -214,6 +223,7 @@ public: /// /// @param handle Callout context - which is expected to contain the /// get command JSON text in the "command" argument + /// /// @return 0 if the handler has been invoked successfully, 1 if an /// error occurs, 3 if no leases are returned. int @@ -226,6 +236,7 @@ public: /// /// @param handle Callout context - which is expected to contain the /// get command JSON text in the "command" argument + /// /// @return 0 if the handler has been invoked successfully, 1 if an /// error occurs, 3 if no leases are returned. int @@ -237,6 +248,7 @@ public: /// /// @param handle Callout context - which is expected to contain the /// delete command JSON text in the "command" argument + /// /// @return 0 upon success, non-zero otherwise int lease4DelHandler(CalloutHandle& handle); @@ -247,6 +259,7 @@ public: /// /// @param handle Callout context - which is expected to contain the /// delete command JSON text in the "command" argument + /// /// @return 0 upon success, non-zero otherwise int lease6DelHandler(CalloutHandle& handle); @@ -257,6 +270,7 @@ public: /// /// @param handle Callout context - which is expected to contain the /// update command JSON text in the "command" argument + /// /// @return 0 upon success, non-zero otherwise int lease4UpdateHandler(CalloutHandle& handle); @@ -267,6 +281,7 @@ public: /// /// @param handle Callout context - which is expected to contain the /// update command JSON text in the "command" argument + /// /// @return 0 upon success, non-zero otherwise int lease6UpdateHandler(CalloutHandle& handle); @@ -277,6 +292,7 @@ public: /// /// @param handle Callout context - which is expected to contain the /// wipe command JSON text in the "command" argument + /// /// @return 0 upon success, non-zero otherwise int lease4WipeHandler(CalloutHandle& handle); @@ -287,6 +303,7 @@ public: /// /// @param handle Callout context - which is expected to contain the /// wipe command JSON text in the "command" argument + /// /// @return 0 upon success, non-zero otherwise int lease6WipeHandler(CalloutHandle& handle); @@ -297,6 +314,7 @@ public: /// /// @param handle Callout context - which is expected to contain the /// lease4-resend-ddns command JSON text in the "command" argument + /// /// @return 0 upon success, non-zero otherwise int lease4ResendDdnsHandler(CalloutHandle& handle); @@ -306,6 +324,7 @@ public: /// /// @param handle Callout context - which is expected to contain the /// lease6-resend-ddns command JSON text in the "command" argument + /// /// @return 0 upon success, non-zero otherwise int lease6ResendDdnsHandler(CalloutHandle& handle); @@ -316,7 +335,9 @@ public: /// /// @param v6 whether addresses allowed are v4 (false) or v6 (true) /// @param args arguments passed to command + /// /// @return parsed parameters + /// /// @throw BadValue if input arguments don't make sense. Parameters getParameters(bool v6, const ConstElementPtr& args); @@ -351,6 +372,8 @@ public: /// @param control_result Control result: "empty" of the lease was /// not found, "error" otherwise. /// @param error_message Error message. + /// + /// @return The JSON map of the failed leases. ElementPtr createFailedLeaseMap(const Lease::Type& lease_type, const IOAddress& lease_address, const DuidPtr& duid, @@ -358,11 +381,14 @@ public: const std::string& error_message) const; /// @brief Fetches an IP address parameter from a map of parameters + /// /// @param map of parameters in which to look /// @name name of the parameter desired /// @param family expected protocol family of the address parameter, /// AF_INET or AF_INET6 + /// /// @return IOAddress containing the value of the parameter. + /// /// @throw BadValue if the parameter is missing or invalid IOAddress getAddressParam(ConstElementPtr params, const std::string name, short family = AF_INET) const; @@ -400,6 +426,26 @@ public: /// /// @param lease Deleted lease. static void updateStatsOnDelete(const Lease6Ptr& lease); + + /// @brief Add or update lease. + /// + /// @param lease The lease to be added or updated (if exists). + /// @param force_create Flag to indicate if the function should try to + /// create the lease if it does not exists, or simply update and fail in + /// such case. + /// + /// @return true if lease has been successfully added, false otherwise. + static bool addOrUpdate4(Lease4Ptr lease, bool force_create); + + /// @brief Add or update lease. + /// + /// @param lease The lease to be added or updated (if exists). + /// @param force_create Flag to indicate if the function should try to + /// create the lease if it does not exists, or simply update and fail in + /// such case. + /// + /// @return true if lease has been successfully added, false otherwise. + static bool addOrUpdate6(Lease6Ptr lease, bool force_create); }; void @@ -597,10 +643,8 @@ LeaseCmdsImpl::updateStatsOnDelete(const Lease6Ptr& lease) { } } -namespace { // anonymous namespace. - bool -addOrUpdate4(Lease4Ptr lease, bool force_create) { +LeaseCmdsImpl::addOrUpdate4(Lease4Ptr lease, bool force_create) { Lease4Ptr existing = LeaseMgrFactory::instance().getLease4(lease->addr_); if (force_create && !existing) { // lease does not exist @@ -617,7 +661,7 @@ addOrUpdate4(Lease4Ptr lease, bool force_create) { } bool -addOrUpdate6(Lease6Ptr lease, bool force_create) { +LeaseCmdsImpl::addOrUpdate6(Lease6Ptr lease, bool force_create) { Lease6Ptr existing = LeaseMgrFactory::instance().getLease6(lease->type_, lease->addr_); if (force_create && !existing) { @@ -634,8 +678,6 @@ addOrUpdate6(Lease6Ptr lease, bool force_create) { return (false); } -} // end of anonymous namespace. - int LeaseCmdsImpl::leaseAddHandler(CalloutHandle& handle) { // Arbitrary defaulting to DHCPv4 or with other words extractCommand @@ -1847,12 +1889,18 @@ LeaseCmdsImpl::lease4WipeHandler(CalloutHandle& handle) { } if (id) { - // Wipe a single subnet + // Wipe a single subnet. num = LeaseMgrFactory::instance().wipeLeases4(id); ids << " " << id; - int64_t previous_declined = StatsMgr::instance().getObservation( - StatsMgr::generateName("subnet", id, "declined-addresses"))->getInteger().first; + auto observation = StatsMgr::instance().getObservation( + StatsMgr::generateName("subnet", id, "declined-addresses")); + + int64_t previous_declined = 0; + + if (observation) { + previous_declined = observation->getInteger().first; + } StatsMgr::instance().setValue( StatsMgr::generateName("subnet", id, "assigned-addresses"), @@ -1925,8 +1973,14 @@ LeaseCmdsImpl::lease6WipeHandler(CalloutHandle& handle) { num = LeaseMgrFactory::instance().wipeLeases6(id); ids << " " << id; - int64_t previous_declined = StatsMgr::instance().getObservation( - StatsMgr::generateName("subnet", id, "declined-addresses"))->getInteger().first; + auto observation = StatsMgr::instance().getObservation( + StatsMgr::generateName("subnet", id, "declined-addresses")); + + int64_t previous_declined = 0; + + if (observation) { + previous_declined = observation->getInteger().first; + } StatsMgr::instance().setValue( StatsMgr::generateName("subnet", id, "assigned-nas" ), diff --git a/src/hooks/dhcp/lease_cmds/tests/lease_cmds_unittest.cc b/src/hooks/dhcp/lease_cmds/tests/lease_cmds_unittest.cc index 85ac4ebb43..8fc5458398 100644 --- a/src/hooks/dhcp/lease_cmds/tests/lease_cmds_unittest.cc +++ b/src/hooks/dhcp/lease_cmds/tests/lease_cmds_unittest.cc @@ -910,7 +910,7 @@ TEST_F(LeaseCmdsTest, Lease4Add) { } // Check that a simple, well formed lease4 can be added. -TEST_F(LeaseCmdsTest, Lease4AddWithStats) { +TEST_F(LeaseCmdsTest, Lease4AddDeclinedLeases) { // Initialize lease manager (false = v4, false = don't add leases) initLeaseMgr(false, false); @@ -1054,7 +1054,7 @@ TEST_F(LeaseCmdsTest, Lease4AddSubnetIdMissing) { // Check that subnet-id is optional. If not specified, Kea should select // it on its own. -TEST_F(LeaseCmdsTest, Lease4AddSubnetIdMissingWithStats) { +TEST_F(LeaseCmdsTest, Lease4AddSubnetIdMissingDeclinedLeases) { // Initialize lease manager (false = v4, false = don't add leases) initLeaseMgr(false, false); @@ -1627,7 +1627,7 @@ TEST_F(LeaseCmdsTest, Lease6Add) { } // Check that a simple, well formed lease6 can be added. -TEST_F(LeaseCmdsTest, Lease6AddWithStats) { +TEST_F(LeaseCmdsTest, Lease6AddDeclinedLeases) { // Initialize lease manager (true = v6, false = don't add leases) initLeaseMgr(true, false); @@ -1802,7 +1802,7 @@ TEST_F(LeaseCmdsTest, Lease6AddSubnetIdMissing) { // Check that subnet-id is optional. If not specified, Kea should select // it on its own. -TEST_F(LeaseCmdsTest, Lease6AddSubnetIdMissingWithStats) { +TEST_F(LeaseCmdsTest, Lease6AddSubnetIdMissingDeclinedLeases) { // Initialize lease manager (true = v6, false = don't add leases) initLeaseMgr(true, false); @@ -4090,7 +4090,7 @@ TEST_F(LeaseCmdsTest, Lease4Update) { // Check that a lease4 can be updated. We're changing hw-address // and a hostname. -TEST_F(LeaseCmdsTest, Lease4UpdateWithStats) { +TEST_F(LeaseCmdsTest, Lease4UpdateDeclinedLeases) { // Initialize lease manager (false = v4, true = add leases) initLeaseMgr(false, true, true); @@ -4195,7 +4195,7 @@ TEST_F(LeaseCmdsTest, Lease4UpdateNoSubnetId) { // Check that a lease4 can be updated. We're changing hw-address // and a hostname. The subnet-id is not specified. -TEST_F(LeaseCmdsTest, Lease4UpdateNoSubnetIdWithStats) { +TEST_F(LeaseCmdsTest, Lease4UpdateNoSubnetIdDeclinedLeases) { // Initialize lease manager (false = v4, true = add leases) initLeaseMgr(false, true, true); @@ -4661,7 +4661,7 @@ TEST_F(LeaseCmdsTest, Lease6Update) { // Check that a lease6 can be updated. We're changing hw-address // and a hostname. -TEST_F(LeaseCmdsTest, Lease6UpdateWithStats) { +TEST_F(LeaseCmdsTest, Lease6UpdateDeclinedLeases) { // Initialize lease manager (true = v6, true = add leases) initLeaseMgr(true, true, true); @@ -4797,7 +4797,7 @@ TEST_F(LeaseCmdsTest, Lease6UpdateNoSubnetId) { // Check that a lease6 can be updated. We're changing hw-address // and a hostname. The subnet-id is not specified. -TEST_F(LeaseCmdsTest, Lease6UpdateNoSubnetIdWithStats) { +TEST_F(LeaseCmdsTest, Lease6UpdateNoSubnetIdDeclinedLeases) { // Initialize lease manager (true = v6, true = add leases) initLeaseMgr(true, true, true); @@ -5316,7 +5316,7 @@ TEST_F(LeaseCmdsTest, Lease4DelByAddr) { } // Checks that lease4-del can return a lease by address. -TEST_F(LeaseCmdsTest, Lease4DelByAddrWithStats) { +TEST_F(LeaseCmdsTest, Lease4DelByAddrDeclinedLeases) { // Initialize lease manager (false = v4, true = add leases) initLeaseMgr(false, true, true); @@ -5745,7 +5745,7 @@ TEST_F(LeaseCmdsTest, Lease6DelByAddr) { // Checks that lease6-del(subnet-id, addr6) can handle a situation when // the query is correctly formed and the lease is returned. -TEST_F(LeaseCmdsTest, Lease6DelByAddrWithStats) { +TEST_F(LeaseCmdsTest, Lease6DelByAddrDeclinedLeases) { initLeaseMgr(true, true, true); // (true = v6, true = create a lease)