From: Marcin Siodelski Date: Fri, 29 Jun 2018 11:37:30 +0000 (+0200) Subject: [5664] Implemented wrapper for CalloutHandle. X-Git-Tag: trac5555_base~1^2~6 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=04af46cc2f3ec0e8678d257093a27031f660fe8a;p=thirdparty%2Fkea.git [5664] Implemented wrapper for CalloutHandle. --- diff --git a/src/lib/hooks/callout_handle.cc b/src/lib/hooks/callout_handle.cc index 8e83334f52..8a8e50dd41 100644 --- a/src/lib/hooks/callout_handle.cc +++ b/src/lib/hooks/callout_handle.cc @@ -158,5 +158,26 @@ CalloutHandle::getHookName() const { return (hook); } -} // namespace util +ScopedCalloutHandleState::ScopedCalloutHandleState(CalloutHandlePtr& callout_handle) + : callout_handle_(callout_handle) { + if (!callout_handle_) { + isc_throw(BadValue, "callout_handle argument must not be null"); + } + + resetState(); +} + +ScopedCalloutHandleState::~ScopedCalloutHandleState() { + resetState(); +} + +void +ScopedCalloutHandleState::resetState() { + // No need to check if the handle is null because the constructor + // already checked that. + callout_handle_->deleteAllArguments(); + callout_handle_->setStatus(CalloutHandle::NEXT_STEP_CONTINUE); +} + +} // namespace hooks } // namespace isc diff --git a/src/lib/hooks/callout_handle.h b/src/lib/hooks/callout_handle.h index 206c3b6db7..f421009f5e 100644 --- a/src/lib/hooks/callout_handle.h +++ b/src/lib/hooks/callout_handle.h @@ -415,6 +415,71 @@ private: /// A shared pointer to a CalloutHandle object. typedef boost::shared_ptr CalloutHandlePtr; +/// @brief Wrapper class around callout handle which automatically +/// resets handle's state. +/// +/// The Kea servers often require to associate processed packets with +/// @c CalloutHandle instances. This is to faciliate the case when the +/// hooks library passes information between the callouts using the +/// 'context' stored in the callout handle. The callouts invoked throughout +/// the packet lifetime have access to the context information for the +/// given packet. +/// +/// The association between the packets and the callout handles is +/// achived by giving the ownership of the @c CalloutHandle objects to +/// the @c Pkt objects. When the @c Pkt object goes out of scope, it should +/// also release the pointer to the owned @c CalloutHandle object. +/// However, this causes a risk of circular dependency between the shared +/// pointer to the @c Pkt object and the shared pointer to the +/// @c CalloutHandle it owns, because the pointer to the packet is often +/// set as an argument of the callout handle prior to invoking a callout. +/// +/// In order to break the circular dependency, the arguments of the +/// callout handle must be deleted as soon as they are not needed +/// anymore. This class is a wrapper around the callout handle object, +/// which resets its state during construction and destruction. All +/// Kea hook points must use this class within the scope where the +/// @c HooksManager::callCallouts is invoked to reset the state of the +/// callout handle. The state is reset when this object goes out of +/// scope. +/// +/// Currently, the following operations are performed during the reset: +/// - all arguments of the callout handle are deleted, +/// - the next step status is set to @c CalloutHandle::NEXT_STEP CONTINUE +/// +/// This class must never be modified to also delete the context +/// information from the callout handle, because retaining this information +/// is one of the primary reasons why this class is created. Otherwise, +/// we could simply re-create the callout handle for each hook point +/// without the need to reset the state like we do. +class ScopedCalloutHandleState { +public: + + /// @brief Constructor. + /// + /// Resets state of the callout handle. + /// + /// @param callout_handle reference to the pointer to the callout + /// handle which state should be reset. + /// @throw isc::BadValue if the callout handle is null. + explicit ScopedCalloutHandleState(CalloutHandlePtr& callout_handle); + + /// @brief Destructor. + /// + /// Resets state of the callout handle. + ~ScopedCalloutHandleState(); + +private: + + /// @brief Resets the callout handle state. + /// + /// It is used internally by the constructor and destructor. + void resetState(); + + /// @brief Holds pointer to the wrapped callout handle. + CalloutHandlePtr callout_handle_; +}; + } // namespace hooks } // namespace isc diff --git a/src/lib/hooks/tests/callout_handle_unittest.cc b/src/lib/hooks/tests/callout_handle_unittest.cc index 3e3dcba181..80de99446a 100644 --- a/src/lib/hooks/tests/callout_handle_unittest.cc +++ b/src/lib/hooks/tests/callout_handle_unittest.cc @@ -318,6 +318,46 @@ TEST_F(CalloutHandleTest, StatusField) { EXPECT_EQ(CalloutHandle::NEXT_STEP_CONTINUE, handle.getStatus()); } +// Tests that ScopedCalloutHandleState object resets CalloutHandle state +// during construction and destruction. +TEST_F(CalloutHandleTest, scopedState) { + // Create pointer to the handle to be wrapped. + CalloutHandlePtr handle(new CalloutHandle(getCalloutManager())); + + // Set two arguments and the non-default status. + int one = 1; + int two = 2; + handle->setArgument("one", one); + handle->setArgument("two", two); + handle->setStatus(CalloutHandle::NEXT_STEP_DROP); + + int value = 0; + + { + // Wrap the callout handle with the scoped state object, which should + // reset the state of the handle. + ScopedCalloutHandleState scoped_state(handle); + + // When state is reset, all arguments should be removed and the + // default status should be set. + EXPECT_THROW(handle->getArgument("one", value), NoSuchArgument); + EXPECT_THROW(handle->getArgument("two", value), NoSuchArgument); + EXPECT_EQ(CalloutHandle::NEXT_STEP_CONTINUE, handle->getStatus()); + + // Set the arguments and status again prior to the destruction of + // the wrapper. + handle->setArgument("one", one); + handle->setArgument("two", two); + handle->setStatus(CalloutHandle::NEXT_STEP_DROP); + } + + // Arguments should be gone again and the status should be set to + // a default value. + EXPECT_THROW(handle->getArgument("one", value), NoSuchArgument); + EXPECT_THROW(handle->getArgument("two", value), NoSuchArgument); + EXPECT_EQ(CalloutHandle::NEXT_STEP_CONTINUE, handle->getStatus()); +} + // Further tests of the "skip" flag and tests of getting the name of the // hook to which the current callout is attached is in the "handles_unittest" // module.