From: Tomek Mrugalski Date: Fri, 12 Jul 2013 12:28:01 +0000 (+0200) Subject: [2995] Changes after review: X-Git-Tag: bind10-1.2.0beta1-release~340^2~1 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=14018a4e7e240d9770dfd565284abdf2f82f9b30;p=thirdparty%2Fkea.git [2995] Changes after review: - callouts called using HooksManager::callCallouts - removed Dhcp6Srv::getServerHooks() and getHooksManager() declarations. - updated sendPacket comment (reception->transmission) - documented getCalloutHandle() - indentation fixed for getSubnets6() --- diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 33898374e4..4491a85d87 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -141,8 +141,6 @@ Dhcpv6Srv::Dhcpv6Srv(uint16_t port) hook_index_pkt6_send_ = Hooks.hook_index_pkt6_send_; /// @todo call loadLibraries() when handling configuration changes - vector libraries; // no libraries at this time - HooksManager::loadLibraries(libraries); } catch (const std::exception &e) { LOG_ERROR(dhcp6_logger, DHCP6_SRV_CONSTRUCT_ERROR).arg(e.what()); @@ -217,8 +215,7 @@ bool Dhcpv6Srv::run() { callout_handle->setArgument("query6", query); // Call callouts - HooksManager::getHooksManager().callCallouts(hook_index_pkt6_receive_, - *callout_handle); + HooksManager::callCallouts(hook_index_pkt6_receive_, *callout_handle); // Callouts decided to skip the next processing step. The next // processing step would to process the packet, so skip at this @@ -308,15 +305,11 @@ bool Dhcpv6Srv::run() { // Delete all previous arguments callout_handle->deleteAllArguments(); - // Clear skip flag if it was set in previous callouts - callout_handle->setSkip(false); - // Set our response callout_handle->setArgument("response6", rsp); // Call all installed callouts - HooksManager::getHooksManager().callCallouts(hook_index_pkt6_send_, - *callout_handle); + HooksManager::callCallouts(hook_index_pkt6_send_, *callout_handle); // Callouts decided to skip the next processing step. The next // processing step would to send the packet, so skip at this @@ -670,8 +663,7 @@ Dhcpv6Srv::selectSubnet(const Pkt6Ptr& question) { callout_handle->setArgument("subnet6collection", CfgMgr::instance().getSubnets6()); // Call user (and server-side) callouts - HooksManager::getHooksManager().callCallouts(hook_index_subnet6_select_, - *callout_handle); + HooksManager::callCallouts(hook_index_subnet6_select_, *callout_handle); // Callouts decided to skip this step. This means that no subnet will be // selected. Packet processing will continue, but it will be severly limited diff --git a/src/bin/dhcp6/dhcp6_srv.h b/src/bin/dhcp6/dhcp6_srv.h index d1c7bb6bf6..3954581793 100644 --- a/src/bin/dhcp6/dhcp6_srv.h +++ b/src/bin/dhcp6/dhcp6_srv.h @@ -23,7 +23,7 @@ #include #include #include -#include +#include #include @@ -88,20 +88,6 @@ public: /// @brief Instructs the server to shut down. void shutdown(); - /// @brief returns ServerHooks object - /// @todo: remove this as soon as ServerHooks object is converted - /// to a signleton. - //static boost::shared_ptr getServerHooks(); - - /// @brief returns Callout Manager object - /// - /// This manager is used to manage callouts registered on various hook - /// points. @todo exact access method for HooksManager manager will change - /// when it will be converted to a singleton. - /// - /// @return CalloutManager instance - //static boost::shared_ptr getHooksManager(); - protected: /// @brief verifies if specified packet meets RFC requirements @@ -347,7 +333,7 @@ protected: /// @brief dummy wrapper around IfaceMgr::send() /// /// This method is useful for testing purposes, where its replacement - /// simulates reception of a packet. For that purpose it is protected. + /// simulates transmission of a packet. For that purpose it is protected. virtual void sendPacket(const Pkt6Ptr& pkt); private: @@ -364,6 +350,11 @@ private: /// initiate server shutdown procedure. volatile bool shutdown_; + /// @brief returns callout handle for specified packet + /// + /// @param pkt packet for which the handle should be returned + /// + /// @return a callout handle to be used in hooks related to said packet isc::hooks::CalloutHandlePtr getCalloutHandle(const Pkt6Ptr& pkt); /// Indexes for registered hook points diff --git a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc index 5c7f5b679f..f9239078ac 100644 --- a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc +++ b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc @@ -295,6 +295,13 @@ public: virtual ~NakedDhcpv6SrvTest() { // Let's clean up if there is such a file. unlink(DUID_FILE); + HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts( + "pkt6_receive"); + HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts( + "pkt6_send"); + HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts( + "subnet6_select"); + }; // A DUID used in most tests (typically as client-id) @@ -1897,8 +1904,8 @@ Pkt6* captureSimpleSolicit() { /// @brief a class dedicated to Hooks testing in DHCPv6 server /// -/// This class has a some static members, because each non-static -/// method has implicit this parameter, so it does not match callout +/// This class has a number of static members, because each non-static +/// method has implicit 'this' parameter, so it does not match callout /// signature and couldn't be registered. Furthermore, static methods /// can't modify non-static members (for obvious reasons), so many /// fields are declared static. It is still better to keep them as diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 9967ac6353..e94201d6e0 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -509,8 +509,7 @@ Lease6Ptr AllocEngine::reuseExpiredLease(Lease6Ptr& expired, callout_handle->setArgument("lease6", expired); // Call the callouts - HooksManager::getHooksManager().callCallouts(hook_index_lease6_select_, - *callout_handle); + HooksManager::callCallouts(hook_index_lease6_select_, *callout_handle); // Callouts decided to skip the action. This means that the lease is not // assigned, so the client will get NoAddrAvail as a result. The lease @@ -608,8 +607,7 @@ Lease6Ptr AllocEngine::createLease6(const Subnet6Ptr& subnet, callout_handle->setArgument("lease6", lease); // This is the first callout, so no need to clear any arguments - HooksManager::getHooksManager().callCallouts(hook_index_lease6_select_, - *callout_handle); + HooksManager::callCallouts(hook_index_lease6_select_, *callout_handle); // Callouts decided to skip the action. This means that the lease is not // assigned, so the client will get NoAddrAvail as a result. The lease diff --git a/src/lib/dhcpsrv/cfgmgr.h b/src/lib/dhcpsrv/cfgmgr.h index 9d4edcebd1..cbf3df6b22 100644 --- a/src/lib/dhcpsrv/cfgmgr.h +++ b/src/lib/dhcpsrv/cfgmgr.h @@ -209,8 +209,8 @@ public: /// of possible choices (i.e. all subnets). /// @return const reference to Subnet6 collection inline const Subnet6Collection& getSubnets6() { - return (subnets6_); -} + return (subnets6_); + } /// @brief get IPv4 subnet by address